rdt_find_domain() finds a domain given a resource and a cache-id.
This is used by both the architecture code and the filesystem code.

After the filesystem code moves to live in /fs/, this helper will no
longer be visible.

Move it to the global header file. As its now globally visible, and
has only a handful of callers, swap the 'rdt' for 'resctrl'.

Signed-off-by: James Morse <james.morse@arm.com>

---
Changes since v5:
 * This patch replaced one that split off the 'new entry to insert'
   behaviour.
---
 arch/x86/kernel/cpu/resctrl/core.c        | 38 +++--------------------
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |  2 --
 include/linux/resctrl.h                   | 34 ++++++++++++++++++++
 4 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 18e5f13bc4ae..49a9ac0dd96c 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -395,36 +395,6 @@ void rdt_ctrl_update(void *arg)
 	hw_res->msr_update(m);
 }
 
-/*
- * rdt_find_domain - Search for a domain id in a resource domain list.
- *
- * Search the domain list to find the domain id. If the domain id is
- * found, return the domain. NULL otherwise.  If the domain id is not
- * found (and NULL returned) then the first domain with id bigger than
- * the input id can be returned to the caller via @pos.
- */
-struct rdt_domain_hdr *rdt_find_domain(struct list_head *h, int id,
-				       struct list_head **pos)
-{
-	struct rdt_domain_hdr *d;
-	struct list_head *l;
-
-	list_for_each(l, h) {
-		d = list_entry(l, struct rdt_domain_hdr, list);
-		/* When id is found, return its domain. */
-		if (id == d->id)
-			return d;
-		/* Stop searching when finding id's position in sorted list. */
-		if (id < d->id)
-			break;
-	}
-
-	if (pos)
-		*pos = l;
-
-	return NULL;
-}
-
 static void setup_default_ctrlval(struct rdt_resource *r, u32 *dc)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
@@ -535,7 +505,7 @@ static void domain_add_cpu_ctrl(int cpu, struct rdt_resource *r)
 		return;
 	}
 
-	hdr = rdt_find_domain(&r->ctrl_domains, id, &add_pos);
+	hdr = resctrl_find_domain(&r->ctrl_domains, id, &add_pos);
 	if (hdr) {
 		if (WARN_ON_ONCE(hdr->type != RESCTRL_CTRL_DOMAIN))
 			return;
@@ -590,7 +560,7 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
 		return;
 	}
 
-	hdr = rdt_find_domain(&r->mon_domains, id, &add_pos);
+	hdr = resctrl_find_domain(&r->mon_domains, id, &add_pos);
 	if (hdr) {
 		if (WARN_ON_ONCE(hdr->type != RESCTRL_MON_DOMAIN))
 			return;
@@ -655,7 +625,7 @@ static void domain_remove_cpu_ctrl(int cpu, struct rdt_resource *r)
 		return;
 	}
 
-	hdr = rdt_find_domain(&r->ctrl_domains, id, NULL);
+	hdr = resctrl_find_domain(&r->ctrl_domains, id, NULL);
 	if (!hdr) {
 		pr_warn("Can't find control domain for id=%d for CPU %d for resource %s\n",
 			id, cpu, r->name);
@@ -701,7 +671,7 @@ static void domain_remove_cpu_mon(int cpu, struct rdt_resource *r)
 		return;
 	}
 
-	hdr = rdt_find_domain(&r->mon_domains, id, NULL);
+	hdr = resctrl_find_domain(&r->mon_domains, id, NULL);
 	if (!hdr) {
 		pr_warn("Can't find monitor domain for id=%d for CPU %d for resource %s\n",
 			id, cpu, r->name);
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 5d87f279085f..7df98fda8a32 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -695,7 +695,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg)
 		 * This file provides data from a single domain. Search
 		 * the resource to find the domain with "domid".
 		 */
-		hdr = rdt_find_domain(&r->mon_domains, domid, NULL);
+		hdr = resctrl_find_domain(&r->mon_domains, domid, NULL);
 		if (!hdr || WARN_ON_ONCE(hdr->type != RESCTRL_MON_DOMAIN)) {
 			ret = -ENOENT;
 			goto out;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 8291f1b59981..da73404183da 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -581,8 +581,6 @@ void rdtgroup_kn_unlock(struct kernfs_node *kn);
 int rdtgroup_kn_mode_restrict(struct rdtgroup *r, const char *name);
 int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name,
 			     umode_t mask);
-struct rdt_domain_hdr *rdt_find_domain(struct list_head *h, int id,
-				       struct list_head **pos);
 ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 				char *buf, size_t nbytes, loff_t off);
 int rdtgroup_schemata_show(struct kernfs_open_file *of,
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index b74853f224f7..6cec088ae0d9 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
 		might_sleep();
 }
 
+/**
+ * resctrl_find_domain() - Search for a domain id in a resource domain list.
+ * @h:		The domain list to search.
+ * @id:		The domain id to search for.
+ * @pos:	A pointer to position in the list id should be inserted.
+ *
+ * Search the domain list to find the domain id. If the domain id is
+ * found, return the domain. NULL otherwise.  If the domain id is not
+ * found (and NULL returned) then the first domain with id bigger than
+ * the input id can be returned to the caller via @pos.
+ */
+static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
+							 int id,
+							 struct list_head **pos)
+{
+	struct rdt_domain_hdr *d;
+	struct list_head *l;
+
+	list_for_each(l, h) {
+		d = list_entry(l, struct rdt_domain_hdr, list);
+		/* When id is found, return its domain. */
+		if (id == d->id)
+			return d;
+		/* Stop searching when finding id's position in sorted list. */
+		if (id < d->id)
+			break;
+	}
+
+	if (pos)
+		*pos = l;
+
+	return NULL;
+}
+
 /**
  * resctrl_arch_reset_rmid() - Reset any private state associated with rmid
  *			       and eventid.
-- 
2.39.2

Hi James,

On 2/7/25 10:17 AM, James Morse wrote:
> rdt_find_domain() finds a domain given a resource and a cache-id.
> This is used by both the architecture code and the filesystem code.
> 
> After the filesystem code moves to live in /fs/, this helper will no
> longer be visible.
> 
> Move it to the global header file. As its now globally visible, and
> has only a handful of callers, swap the 'rdt' for 'resctrl'.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> Changes since v5:
>  * This patch replaced one that split off the 'new entry to insert'
>    behaviour.
> ---

...

> @@ -395,36 +395,6 @@ void rdt_ctrl_update(void *arg)
>  	hw_res->msr_update(m);
>  }
>  
> -/*
> - * rdt_find_domain - Search for a domain id in a resource domain list.
> - *
> - * Search the domain list to find the domain id. If the domain id is
> - * found, return the domain. NULL otherwise.  If the domain id is not
> - * found (and NULL returned) then the first domain with id bigger than
> - * the input id can be returned to the caller via @pos.
> - */
> -struct rdt_domain_hdr *rdt_find_domain(struct list_head *h, int id,
> -				       struct list_head **pos)
> -{
> -	struct rdt_domain_hdr *d;
> -	struct list_head *l;
> -
> -	list_for_each(l, h) {
> -		d = list_entry(l, struct rdt_domain_hdr, list);
> -		/* When id is found, return its domain. */
> -		if (id == d->id)
> -			return d;
> -		/* Stop searching when finding id's position in sorted list. */
> -		if (id < d->id)
> -			break;
> -	}
> -
> -	if (pos)
> -		*pos = l;
> -
> -	return NULL;
> -}
> -
>  static void setup_default_ctrlval(struct rdt_resource *r, u32 *dc)
>  {
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);

...

> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
>  		might_sleep();
>  }
>  
> +/**
> + * resctrl_find_domain() - Search for a domain id in a resource domain list.
> + * @h:		The domain list to search.
> + * @id:		The domain id to search for.
> + * @pos:	A pointer to position in the list id should be inserted.
> + *
> + * Search the domain list to find the domain id. If the domain id is
> + * found, return the domain. NULL otherwise.  If the domain id is not
> + * found (and NULL returned) then the first domain with id bigger than
> + * the input id can be returned to the caller via @pos.
> + */
> +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
> +							 int id,
> +							 struct list_head **pos)

Could you please provide a motivation for why this needs to be inline now?

> +{
> +	struct rdt_domain_hdr *d;
> +	struct list_head *l;
> +
> +	list_for_each(l, h) {
> +		d = list_entry(l, struct rdt_domain_hdr, list);
> +		/* When id is found, return its domain. */
> +		if (id == d->id)
> +			return d;
> +		/* Stop searching when finding id's position in sorted list. */
> +		if (id < d->id)
> +			break;
> +	}
> +
> +	if (pos)
> +		*pos = l;
> +
> +	return NULL;
> +}
> +
>  /**
>   * resctrl_arch_reset_rmid() - Reset any private state associated with rmid
>   *			       and eventid.


Reinette

On Wed, Feb 19, 2025 at 03:24:06PM -0800, Reinette Chatre wrote:
> On 2/7/25 10:17 AM, James Morse wrote:
> > rdt_find_domain() finds a domain given a resource and a cache-id.
> > This is used by both the architecture code and the filesystem code.
> > 
> > After the filesystem code moves to live in /fs/, this helper will no
> > longer be visible.
> > 
> > Move it to the global header file. As its now globally visible, and
> > has only a handful of callers, swap the 'rdt' for 'resctrl'.
[...]
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
> >  		might_sleep();
> >  }
> >  
> > +/**
> > + * resctrl_find_domain() - Search for a domain id in a resource domain list.
> > + * @h:		The domain list to search.
> > + * @id:		The domain id to search for.
> > + * @pos:	A pointer to position in the list id should be inserted.
> > + *
> > + * Search the domain list to find the domain id. If the domain id is
> > + * found, return the domain. NULL otherwise.  If the domain id is not
> > + * found (and NULL returned) then the first domain with id bigger than
> > + * the input id can be returned to the caller via @pos.
> > + */
> > +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
> > +							 int id,
> > +							 struct list_head **pos)
> 
> Could you please provide a motivation for why this needs to be inline now?

It's in a header now, to avoid the compiler complaining about unused
static functions wherever this file is included. The alternative is a
prototype declaration and the actual implementation in a .c file.

(drive-by comment, I don't really understand this subsystem to make a
meaningful contribution)

-- 
Catalin

Hi Catalin,

On 2/20/25 2:58 AM, Catalin Marinas wrote:
> On Wed, Feb 19, 2025 at 03:24:06PM -0800, Reinette Chatre wrote:
>> On 2/7/25 10:17 AM, James Morse wrote:
>>> rdt_find_domain() finds a domain given a resource and a cache-id.
>>> This is used by both the architecture code and the filesystem code.
>>>
>>> After the filesystem code moves to live in /fs/, this helper will no
>>> longer be visible.
>>>
>>> Move it to the global header file. As its now globally visible, and
>>> has only a handful of callers, swap the 'rdt' for 'resctrl'.
> [...]
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
>>>  		might_sleep();
>>>  }
>>>  
>>> +/**
>>> + * resctrl_find_domain() - Search for a domain id in a resource domain list.
>>> + * @h:		The domain list to search.
>>> + * @id:		The domain id to search for.
>>> + * @pos:	A pointer to position in the list id should be inserted.
>>> + *
>>> + * Search the domain list to find the domain id. If the domain id is
>>> + * found, return the domain. NULL otherwise.  If the domain id is not
>>> + * found (and NULL returned) then the first domain with id bigger than
>>> + * the input id can be returned to the caller via @pos.
>>> + */
>>> +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
>>> +							 int id,
>>> +							 struct list_head **pos)
>>
>> Could you please provide a motivation for why this needs to be inline now?
> 
> It's in a header now, to avoid the compiler complaining about unused
> static functions wherever this file is included. The alternative is a
> prototype declaration and the actual implementation in a .c file.

resctrl_find_domain() is currently in a .c file (arch/x86/kernel/cpu/resctrl/core.c)
with a prototype declaration (in arch/x86/kernel/cpu/resctrl/internal.h). This patch
switches resctrl_find_domain() to be inline without a motivation.

After a fresh reading of "The inline disease" in Documentation/process/coding-style.rst 
I do see a few red flags related to making this function inline. The function is certainly
larger than the rule of thumb of "3 lines" and considering the number of call sites I do
not see how bloating the kernel is justified.

> 
> (drive-by comment, I don't really understand this subsystem to make a
> meaningful contribution)
> 

Thanks for taking a look. The idea is not unique to resctrl.

Reinette

Hi Reinette,

On 20/02/2025 16:01, Reinette Chatre wrote:
> On 2/20/25 2:58 AM, Catalin Marinas wrote:
>> On Wed, Feb 19, 2025 at 03:24:06PM -0800, Reinette Chatre wrote:
>>> On 2/7/25 10:17 AM, James Morse wrote:
>>>> rdt_find_domain() finds a domain given a resource and a cache-id.
>>>> This is used by both the architecture code and the filesystem code.
>>>>
>>>> After the filesystem code moves to live in /fs/, this helper will no
>>>> longer be visible.
>>>>
>>>> Move it to the global header file. As its now globally visible, and
>>>> has only a handful of callers, swap the 'rdt' for 'resctrl'.

>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
>>>>  		might_sleep();
>>>>  }
>>>>  
>>>> +/**
>>>> + * resctrl_find_domain() - Search for a domain id in a resource domain list.
>>>> + * @h:		The domain list to search.
>>>> + * @id:		The domain id to search for.
>>>> + * @pos:	A pointer to position in the list id should be inserted.
>>>> + *
>>>> + * Search the domain list to find the domain id. If the domain id is
>>>> + * found, return the domain. NULL otherwise.  If the domain id is not
>>>> + * found (and NULL returned) then the first domain with id bigger than
>>>> + * the input id can be returned to the caller via @pos.
>>>> + */
>>>> +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
>>>> +							 int id,
>>>> +							 struct list_head **pos)
>>>
>>> Could you please provide a motivation for why this needs to be inline now?
>>
>> It's in a header now, to avoid the compiler complaining about unused
>> static functions wherever this file is included. The alternative is a
>> prototype declaration and the actual implementation in a .c file.

(it was this)


> resctrl_find_domain() is currently in a .c file (arch/x86/kernel/cpu/resctrl/core.c)
> with a prototype declaration (in arch/x86/kernel/cpu/resctrl/internal.h). This patch
> switches resctrl_find_domain() to be inline without a motivation.

Its not clear what side should own this function, both the architecture and filesystem
code need to make use of it. The majority of callers are in the arch code - but putting it
here means its duplicated between architectures.
Putting it in the filesystem code means calling out to it, and means the compiler can't
remove that extra NULL argument thing if its unused. Its also a hindrance to having the
arch code standalone - so we can in the future consider making the filesystem parts a
module. (Tony has picked at this and pointed out that kernfs not being exported is a
bigger problem). Its not needed now (and I haven't tried), but it seems a reasonable
direction of travel.


> After a fresh reading of "The inline disease" in Documentation/process/coding-style.rst 
> I do see a few red flags related to making this function inline. The function is certainly
> larger than the rule of thumb of "3 lines"

The thing about rules of thumb ...
The first example I looked at is __task_state_index(), which is equally longer than this
rule of thumb.


> and considering the number of call sites I do not see how bloating the kernel is justified.

I think this is several orders of magnitude below the point that this is something to care
about. If this were inlined in put_user() or something lots of drivers call, I'd agree.

I'll move it to live in the filesystem code - that saves 36 bytes.
(I'm honestly surprised its measurable!)


James

Hi, James, Reinette,

On 2/20/25 08:01, Reinette Chatre wrote:
> Hi Catalin,
>
> On 2/20/25 2:58 AM, Catalin Marinas wrote:
>> On Wed, Feb 19, 2025 at 03:24:06PM -0800, Reinette Chatre wrote:
>>> On 2/7/25 10:17 AM, James Morse wrote:
>>>> rdt_find_domain() finds a domain given a resource and a cache-id.
>>>> This is used by both the architecture code and the filesystem code.
>>>>
>>>> After the filesystem code moves to live in /fs/, this helper will no
>>>> longer be visible.
>>>>
>>>> Move it to the global header file. As its now globally visible, and
>>>> has only a handful of callers, swap the 'rdt' for 'resctrl'.
>> [...]
>>>> --- a/include/linux/resctrl.h
>>>> +++ b/include/linux/resctrl.h
>>>> @@ -372,6 +372,40 @@ static inline void resctrl_arch_rmid_read_context_check(void)
>>>>   		might_sleep();
>>>>   }
>>>>   
>>>> +/**
>>>> + * resctrl_find_domain() - Search for a domain id in a resource domain list.
>>>> + * @h:		The domain list to search.
>>>> + * @id:		The domain id to search for.
>>>> + * @pos:	A pointer to position in the list id should be inserted.
>>>> + *
>>>> + * Search the domain list to find the domain id. If the domain id is
>>>> + * found, return the domain. NULL otherwise.  If the domain id is not
>>>> + * found (and NULL returned) then the first domain with id bigger than
>>>> + * the input id can be returned to the caller via @pos.
>>>> + */
>>>> +static inline struct rdt_domain_hdr *resctrl_find_domain(struct list_head *h,
>>>> +							 int id,
>>>> +							 struct list_head **pos)
>>> Could you please provide a motivation for why this needs to be inline now?
>> It's in a header now, to avoid the compiler complaining about unused
>> static functions wherever this file is included. The alternative is a
>> prototype declaration and the actual implementation in a .c file.
> resctrl_find_domain() is currently in a .c file (arch/x86/kernel/cpu/resctrl/core.c)
> with a prototype declaration (in arch/x86/kernel/cpu/resctrl/internal.h). This patch
> switches resctrl_find_domain() to be inline without a motivation.
>
> After a fresh reading of "The inline disease" in Documentation/process/coding-style.rst
> I do see a few red flags related to making this function inline. The function is certainly
> larger than the rule of thumb of "3 lines" and considering the number of call sites I do
> not see how bloating the kernel is justified.

Agree with Reinette.


Plus, resctrl_find_domain() is only called during setup and CPU hot plug 
which are not run time paths and won't impact run time performance. 
inline doesn't help the performance but makes the kernel bigger.

I can see the function is moved from arch/x86/kernel/cpu/resctrl/core.c 
and there is no corresponding fs/resctrl/core.c.

If your motivation is to avoid fs/resctrl/core.c (which is much small) 
to have one less file and just host the function in .h, please consider 
to create fs/resctrl/core.c and put the function in it and declare it in 
the .h file. So there won't be inline issue any more.

>> (drive-by comment, I don't really understand this subsystem to make a
>> meaningful contribution)
>>
> Thanks for taking a look. The idea is not unique to resctrl.
>
> Reinette
>
Thanks.


-Fenghua