commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
searching closid_num_dirty_rmid") added a Kconfig option that causes
resctrl to search for the CLOSID with the fewest dirty cache lines when
creating a new control group. This depends on the values read from the
llc_occupancy counters.

This support missed that some platforms may not have these counters.
This causes a NULL pointer dereference when creating a new control
group as the array was not allocated by dom_data_init().

As this feature isn't necessary on platforms that don't have cache
occupancy monitors, add this to the check that occurs when a new
control group is allocated.

The existing code is not selected by any upstream platform, it makes
no sense to backport this patch to stable.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 011e17efb1a6..1767c1affa60 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -149,7 +149,8 @@ static int closid_alloc(void)
 
 	lockdep_assert_held(&rdtgroup_mutex);
 
-	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
+	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
+	    is_llc_occupancy_enabled()) {
 		cleanest_closid = resctrl_find_cleanest_closid();
 		if (cleanest_closid < 0)
 			return cleanest_closid;
-- 
2.39.2

On 3/21/24 11:50, James Morse wrote:
> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> searching closid_num_dirty_rmid") added a Kconfig option that causes

This is not true. The Kconfig option is never added. It is added later in
the series. There is also comment
on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/


Shouldn't the Kconfig option added first before doing this change?

> resctrl to search for the CLOSID with the fewest dirty cache lines when
> creating a new control group. This depends on the values read from the
> llc_occupancy counters.
> 
> This support missed that some platforms may not have these counters.
> This causes a NULL pointer dereference when creating a new control
> group as the array was not allocated by dom_data_init().
> 
> As this feature isn't necessary on platforms that don't have cache
> occupancy monitors, add this to the check that occurs when a new
> control group is allocated.
> 
> The existing code is not selected by any upstream platform, it makes
> no sense to backport this patch to stable.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 011e17efb1a6..1767c1affa60 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -149,7 +149,8 @@ static int closid_alloc(void)
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> +	    is_llc_occupancy_enabled()) {
>  		cleanest_closid = resctrl_find_cleanest_closid();
>  		if (cleanest_closid < 0)
>  			return cleanest_closid;

-- 
Thanks
Babu Moger

On Mon, Apr 15, 2024 at 03:27:42PM -0500, Moger, Babu wrote:
> 
> 
> On 3/21/24 11:50, James Morse wrote:
> > commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> > searching closid_num_dirty_rmid") added a Kconfig option that causes
> 
> This is not true. The Kconfig option is never added. It is added later in
> the series. There is also comment
> on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/
> 
> 
> Shouldn't the Kconfig option added first before doing this change?
> 
> > resctrl to search for the CLOSID with the fewest dirty cache lines when
> > creating a new control group. This depends on the values read from the
> > llc_occupancy counters.

See David's comments and previous discussion on this patch.

You're right to point out that the description of the original commit
does seem a bit garbled though: CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is
not present in Kconfig here, but already referenced by other code.

We seem to have a consensus that it's OK to have a dangling IS_ENABLED()
so long as the option is added formally to Kconfig later, but it looks
like the commit message here should be reworded.

Does the following make sense?

--8<--

commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
searching closid_num_dirty_rmid") added logic that causes resctrl to
search for the CLOSID with the fewest dirty cache lines when creating a
new control group, if requested by the arch code. This depends on the
values read from the llc_occupancy counters. The logic is applicable to
architectures where the CLOSID effectively forms part of the monitoring
identifier and so do not allow complete freedom to choose an unused
monitoring identifier for a given CLOSID.

-->8--


Although it would probably have been better if the Kconfig option had
been added upstream, this patch does not create that that situation and
the series (taken as a whole) resolves it.

So I am not sure that anything would be solved or improved by changing
the body of this patch, but if people still have concerns then I guess
we can look at it.


> > 
> > This support missed that some platforms may not have these counters.
> > This causes a NULL pointer dereference when creating a new control
> > group as the array was not allocated by dom_data_init().
> > 
> > As this feature isn't necessary on platforms that don't have cache
> > occupancy monitors, add this to the check that occurs when a new
> > control group is allocated.
> > 
> > The existing code is not selected by any upstream platform, it makes
> > no sense to backport this patch to stable.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 011e17efb1a6..1767c1affa60 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -149,7 +149,8 @@ static int closid_alloc(void)
> >  
> >  	lockdep_assert_held(&rdtgroup_mutex);
> >  
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> > +	    is_llc_occupancy_enabled()) {
> >  		cleanest_closid = resctrl_find_cleanest_closid();
> >  		if (cleanest_closid < 0)
> >  			return cleanest_closid;
> 
> -- 
> Thanks
> Babu Moger
> 

[...]

Cheers
---Dave

Hi Dave,

On 4/16/24 11:13, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 03:27:42PM -0500, Moger, Babu wrote:
>>
>>
>> On 3/21/24 11:50, James Morse wrote:
>>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
>>> searching closid_num_dirty_rmid") added a Kconfig option that causes
>>
>> This is not true. The Kconfig option is never added. It is added later in
>> the series. There is also comment
>> on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/
>>
>>
>> Shouldn't the Kconfig option added first before doing this change?
>>
>>> resctrl to search for the CLOSID with the fewest dirty cache lines when
>>> creating a new control group. This depends on the values read from the
>>> llc_occupancy counters.
> 
> See David's comments and previous discussion on this patch.
> 
> You're right to point out that the description of the original commit
> does seem a bit garbled though: CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is
> not present in Kconfig here, but already referenced by other code.
> 
> We seem to have a consensus that it's OK to have a dangling IS_ENABLED()
> so long as the option is added formally to Kconfig later, but it looks
> like the commit message here should be reworded.
> 
> Does the following make sense?
> 
> --8<--
> 
> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> searching closid_num_dirty_rmid") added logic that causes resctrl to
> search for the CLOSID with the fewest dirty cache lines when creating a
> new control group, if requested by the arch code. This depends on the
> values read from the llc_occupancy counters. The logic is applicable to
> architectures where the CLOSID effectively forms part of the monitoring
> identifier and so do not allow complete freedom to choose an unused
> monitoring identifier for a given CLOSID.
> 
> -->8--

That looks good. Thanks

> 
> 
> Although it would probably have been better if the Kconfig option had
> been added upstream, this patch does not create that that situation and
> the series (taken as a whole) resolves it.
> 
> So I am not sure that anything would be solved or improved by changing
> the body of this patch, but if people still have concerns then I guess
> we can look at it.

> 
> 
>>>
>>> This support missed that some platforms may not have these counters.
>>> This causes a NULL pointer dereference when creating a new control
>>> group as the array was not allocated by dom_data_init().
>>>
>>> As this feature isn't necessary on platforms that don't have cache
>>> occupancy monitors, add this to the check that occurs when a new
>>> control group is allocated.
>>>
>>> The existing code is not selected by any upstream platform, it makes
>>> no sense to backport this patch to stable.
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 011e17efb1a6..1767c1affa60 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -149,7 +149,8 @@ static int closid_alloc(void)
>>>  
>>>  	lockdep_assert_held(&rdtgroup_mutex);
>>>  
>>> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>>> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
>>> +	    is_llc_occupancy_enabled()) {
>>>  		cleanest_closid = resctrl_find_cleanest_closid();
>>>  		if (cleanest_closid < 0)
>>>  			return cleanest_closid;
>>
>> -- 
>> Thanks
>> Babu Moger
>>
> 
> [...]
> 
> Cheers
> ---Dave

-- 
Thanks
Babu Moger

On 21.03.24 17:50, James Morse wrote:
> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> searching closid_num_dirty_rmid") added a Kconfig option that causes
> resctrl to search for the CLOSID with the fewest dirty cache lines when
> creating a new control group. This depends on the values read from the
> llc_occupancy counters.
> 
> This support missed that some platforms may not have these counters.
> This causes a NULL pointer dereference when creating a new control
> group as the array was not allocated by dom_data_init().
> 
> As this feature isn't necessary on platforms that don't have cache
> occupancy monitors, add this to the check that occurs when a new
> control group is allocated.
> 
> The existing code is not selected by any upstream platform, it makes
> no sense to backport this patch to stable.
> 

It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
I guess it will all make sense once the refactoring is done :)

As Reinette comments, likely we want here:

Fixes: 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by searching closid_num_dirty_rmid")

> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 011e17efb1a6..1767c1affa60 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -149,7 +149,8 @@ static int closid_alloc(void)
>   
>   	lockdep_assert_held(&rdtgroup_mutex);
>   
> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> +	    is_llc_occupancy_enabled()) {
>   		cleanest_closid = resctrl_find_cleanest_closid();
>   		if (cleanest_closid < 0)
>   			return cleanest_closid;

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb

On Tue, Apr 09, 2024 at 10:05:33AM +0200, David Hildenbrand wrote:
> On 21.03.24 17:50, James Morse wrote:
> > commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> > searching closid_num_dirty_rmid") added a Kconfig option that causes
> > resctrl to search for the CLOSID with the fewest dirty cache lines when
> > creating a new control group. This depends on the values read from the
> > llc_occupancy counters.

[...]

> It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
> I guess it will all make sense once the refactoring is done :)

Agreed; a stub Kconfig item could be added, but since the file layout
and naming conventions change after this patch, doing this would
probably just create noise in the series though.

Looking at <linux/kconfig.h> (yikes!), IS_ENABLED() is designed to do
the right thing for non-existing Kconfigs...

If nobody is too concerned about the temporarily dangling IS_ENABLED()s
in this series, I won't propose any change here.


> As Reinette comments, likely we want here:
> 
> Fixes: 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by searching closid_num_dirty_rmid")

Noted for James' attention.

> 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 011e17efb1a6..1767c1affa60 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -149,7 +149,8 @@ static int closid_alloc(void)
> >   	lockdep_assert_held(&rdtgroup_mutex);
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> > +	    is_llc_occupancy_enabled()) {
> >   		cleanest_closid = resctrl_find_cleanest_closid();
> >   		if (cleanest_closid < 0)
> >   			return cleanest_closid;
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb

Thanks; noted for James' attention.

Cheers
---Dave

Hi Dave,

On 4/11/2024 7:13 AM, Dave Martin wrote:
> On Tue, Apr 09, 2024 at 10:05:33AM +0200, David Hildenbrand wrote:
>> On 21.03.24 17:50, James Morse wrote:
>>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
>>> searching closid_num_dirty_rmid") added a Kconfig option that causes
>>> resctrl to search for the CLOSID with the fewest dirty cache lines when
>>> creating a new control group. This depends on the values read from the
>>> llc_occupancy counters.
> 
> [...]
> 
>> It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
>> I guess it will all make sense once the refactoring is done :)
> 
> Agreed; a stub Kconfig item could be added, but since the file layout
> and naming conventions change after this patch, doing this would
> probably just create noise in the series though.
> 
> Looking at <linux/kconfig.h> (yikes!), IS_ENABLED() is designed to do
> the right thing for non-existing Kconfigs...
> 
> If nobody is too concerned about the temporarily dangling IS_ENABLED()s
> in this series, I won't propose any change here.

I am not concerned about this. Please note that these IS_ENABLED() checks
are not introduced in this series, they were introduced in the previous
portion of this MPAM work that can be found in upstream resctrl.

Reinette

On Thu, Apr 11, 2024 at 10:35:56AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/11/2024 7:13 AM, Dave Martin wrote:
> > On Tue, Apr 09, 2024 at 10:05:33AM +0200, David Hildenbrand wrote:
> >> On 21.03.24 17:50, James Morse wrote:
> >>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> >>> searching closid_num_dirty_rmid") added a Kconfig option that causes
> >>> resctrl to search for the CLOSID with the fewest dirty cache lines when
> >>> creating a new control group. This depends on the values read from the
> >>> llc_occupancy counters.
> > 
> > [...]
> > 
> >> It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
> >> I guess it will all make sense once the refactoring is done :)
> > 
> > Agreed; a stub Kconfig item could be added, but since the file layout
> > and naming conventions change after this patch, doing this would
> > probably just create noise in the series though.
> > 
> > Looking at <linux/kconfig.h> (yikes!), IS_ENABLED() is designed to do
> > the right thing for non-existing Kconfigs...
> > 
> > If nobody is too concerned about the temporarily dangling IS_ENABLED()s
> > in this series, I won't propose any change here.
> 
> I am not concerned about this. Please note that these IS_ENABLED() checks
> are not introduced in this series, they were introduced in the previous
> portion of this MPAM work that can be found in upstream resctrl.
> 
> Reinette
> 

Ah, right.  Thanks for the explanation.

Cheers
---Dave

Hi David,

(Thank you very much for taking a look at these)

On 4/9/2024 1:05 AM, David Hildenbrand wrote:
> On 21.03.24 17:50, James Morse wrote:
>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
>> searching closid_num_dirty_rmid") added a Kconfig option that causes
>> resctrl to search for the CLOSID with the fewest dirty cache lines when
>> creating a new control group. This depends on the values read from the
>> llc_occupancy counters.
>>
>> This support missed that some platforms may not have these counters.
>> This causes a NULL pointer dereference when creating a new control
>> group as the array was not allocated by dom_data_init().
>>
>> As this feature isn't necessary on platforms that don't have cache
>> occupancy monitors, add this to the check that occurs when a new
>> control group is allocated.
>>
>> The existing code is not selected by any upstream platform, it makes
>> no sense to backport this patch to stable.
>>
> 
> It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
> I guess it will all make sense once the refactoring is done :)

This is done later in this series where patch #29, "fs/resctrl: Add boiler
plate for external resctrl code", introduces it to fs/resctrl/Kconfig.

Reinette

On 09.04.24 17:02, Reinette Chatre wrote:
> Hi David,
> 

Hi,

> (Thank you very much for taking a look at these)

I'm planning on reviewing more; as most of resctrl is code I haven't 
really read before, I'm not able to make progress as fast as I normally 
would in core-MM ... :)

> 
> On 4/9/2024 1:05 AM, David Hildenbrand wrote:
>> On 21.03.24 17:50, James Morse wrote:
>>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
>>> searching closid_num_dirty_rmid") added a Kconfig option that causes
>>> resctrl to search for the CLOSID with the fewest dirty cache lines when
>>> creating a new control group. This depends on the values read from the
>>> llc_occupancy counters.
>>>
>>> This support missed that some platforms may not have these counters.
>>> This causes a NULL pointer dereference when creating a new control
>>> group as the array was not allocated by dom_data_init().
>>>
>>> As this feature isn't necessary on platforms that don't have cache
>>> occupancy monitors, add this to the check that occurs when a new
>>> control group is allocated.
>>>
>>> The existing code is not selected by any upstream platform, it makes
>>> no sense to backport this patch to stable.
>>>
>>
>> It's weird to not see RESCTRL_RMID_DEPENDS_ON_CLOSID appear in any Kconfig file.
>> I guess it will all make sense once the refactoring is done :)
> 
> This is done later in this series where patch #29, "fs/resctrl: Add boiler
> plate for external resctrl code", introduces it to fs/resctrl/Kconfig.

Oh, already in this series, great!

-- 
Cheers,

David / dhildenb

On Tue, Apr 09, 2024 at 05:06:03PM +0200, David Hildenbrand wrote:
> On 09.04.24 17:02, Reinette Chatre wrote:
> > Hi David,
> > 
> 
> Hi,
> 
> > (Thank you very much for taking a look at these)
> 
> I'm planning on reviewing more; as most of resctrl is code I haven't really
> read before, I'm not able to make progress as fast as I normally would in
> core-MM ... :)

[...]

> -- 
> Cheers,
> 
> David / dhildenb

Thanks, all review is appreciated (I'm somewhat in the same boat myself,
but gradually finding my way around...)

Cheers
---Dave

Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:
> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> searching closid_num_dirty_rmid") added a Kconfig option that causes

Please see section about "Fixes" tags in section "Ordering of commit tags"
in Documentation/process/maintainer-tip.rst

> resctrl to search for the CLOSID with the fewest dirty cache lines when
> creating a new control group. This depends on the values read from the
> llc_occupancy counters.
> 
> This support missed that some platforms may not have these counters.
> This causes a NULL pointer dereference when creating a new control
> group as the array was not allocated by dom_data_init().
> 
> As this feature isn't necessary on platforms that don't have cache
> occupancy monitors, add this to the check that occurs when a new
> control group is allocated.
> 
> The existing code is not selected by any upstream platform, it makes
> no sense to backport this patch to stable.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 011e17efb1a6..1767c1affa60 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -149,7 +149,8 @@ static int closid_alloc(void)
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> +	    is_llc_occupancy_enabled()) {
>  		cleanest_closid = resctrl_find_cleanest_closid();
>  		if (cleanest_closid < 0)
>  			return cleanest_closid;

Patch looks good.

Thank you

Reinette

On Mon, Apr 08, 2024 at 08:13:40PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> > searching closid_num_dirty_rmid") added a Kconfig option that causes
> 
> Please see section about "Fixes" tags in section "Ordering of commit tags"
> in Documentation/process/maintainer-tip.rst

Noted for James' attention.

> > resctrl to search for the CLOSID with the fewest dirty cache lines when
> > creating a new control group. This depends on the values read from the
> > llc_occupancy counters.
> > 
> > This support missed that some platforms may not have these counters.
> > This causes a NULL pointer dereference when creating a new control
> > group as the array was not allocated by dom_data_init().
> > 
> > As this feature isn't necessary on platforms that don't have cache
> > occupancy monitors, add this to the check that occurs when a new
> > control group is allocated.
> > 
> > The existing code is not selected by any upstream platform, it makes
> > no sense to backport this patch to stable.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 011e17efb1a6..1767c1affa60 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -149,7 +149,8 @@ static int closid_alloc(void)
> >  
> >  	lockdep_assert_held(&rdtgroup_mutex);
> >  
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> > +	    is_llc_occupancy_enabled()) {
> >  		cleanest_closid = resctrl_find_cleanest_closid();
> >  		if (cleanest_closid < 0)
> >  			return cleanest_closid;
> 
> Patch looks good.
> 
> Thank you
> 
> Reinette
> 

Noted, thanks.

Cheers
---Dave