[PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Xiongfeng Wang]

On ARM64, when PPTT(Processor Properties Topology Table) is not
implemented in ACPI boot, we will goto 'free_ci' with the following
print:
  Unable to detect cache hierarchy for CPU 0

But some other codes may still use 'num_leaves' to iterate through the
'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
would crash. So clear 'num_leaves' in free_cache_attributes().

Fixes: 246246cbde5e ("drivers: base: support cpu cache information
interface to userspace via sysfs")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/base/cacheinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index bfc0959..dad2962 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -297,6 +297,7 @@ static void free_cache_attributes(unsigned int cpu)
 
 	kfree(per_cpu_cacheinfo(cpu));
 	per_cpu_cacheinfo(cpu) = NULL;
+	cache_leaves(cpu) = 0;
 }
 
 int __weak init_cache_level(unsigned int cpu)
-- 
1.7.12.4

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Sudeep Holla]

On Tue, Jul 13, 2021 at 11:47:38AM +0800, Xiongfeng Wang wrote:
> On ARM64, when PPTT(Processor Properties Topology Table) is not
> implemented in ACPI boot, we will goto 'free_ci' with the following
> print:
>   Unable to detect cache hierarchy for CPU 0
>

The change itself looks good and I am fine with that. However,...

> But some other codes may still use 'num_leaves' to iterate through the

Can you point me exactly where it is used to make sure there are no
other issues associated with that.

> 'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
> would crash. So clear 'num_leaves' in free_cache_attributes().
>

And can you provide the crash dump please ? If we are not hitting any
issue and you just figured this with code inspection, that is fine. It
helps to determine if this needs to be backport or just good to have
clean up.

> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  drivers/base/cacheinfo.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index bfc0959..dad2962 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -297,6 +297,7 @@ static void free_cache_attributes(unsigned int cpu)
>
>  	kfree(per_cpu_cacheinfo(cpu));
>  	per_cpu_cacheinfo(cpu) = NULL;
> +	cache_leaves(cpu) = 0;

I initially thought it might get used and crash in cache_shared_cpu_map_remove
but you are setting it later. So where do you suspect it to be used ? Sorry
if I am missing something obvious, looking at this code after long time.

-- 
Regards,
Sudeep

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Xiongfeng Wang]

Hi Sudeep,

On 2021/7/13 19:33, Sudeep Holla wrote:
> On Tue, Jul 13, 2021 at 11:47:38AM +0800, Xiongfeng Wang wrote:
>> On ARM64, when PPTT(Processor Properties Topology Table) is not
>> implemented in ACPI boot, we will goto 'free_ci' with the following
>> print:
>>   Unable to detect cache hierarchy for CPU 0
>>
> 
> The change itself looks good and I am fine with that. However,...
> 
>> But some other codes may still use 'num_leaves' to iterate through the
> 
> Can you point me exactly where it is used to make sure there are no
> other issues associated with that.
> 
>> 'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
>> would crash. So clear 'num_leaves' in free_cache_attributes().
>>
> 
> And can you provide the crash dump please ? If we are not hitting any
> issue and you just figured this with code inspection, that is fine. It
> helps to determine if this needs to be backport or just good to have
> clean up.

There is no issue in the mainline kernel. get_cpu_cacheinfo_id() is only called
on x86. I didn't hit any issue using the mainline kernel.

Actually, it's our own code that crashed. My colleague Shaobo(CCed) tried to add
MPAM support on ARM64. His code called get_cpu_cacheinfo_id() and crashed when
PPTT is not implemented. Maybe he should check whether PPTT is implemented
before calling get_cpu_cacheinfo_id(), but we think it is also better to clear
cache_leaves(cpu) in free_cache_attributes().
Sorry for not clearly expressed.

Thanks,
Xiongfeng

> 
>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>>  drivers/base/cacheinfo.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index bfc0959..dad2962 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -297,6 +297,7 @@ static void free_cache_attributes(unsigned int cpu)
>>
>>  	kfree(per_cpu_cacheinfo(cpu));
>>  	per_cpu_cacheinfo(cpu) = NULL;
>> +	cache_leaves(cpu) = 0;
> 
> I initially thought it might get used and crash in cache_shared_cpu_map_remove
> but you are setting it later. So where do you suspect it to be used ? Sorry
> if I am missing something obvious, looking at this code after long time.
> 

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Sudeep Holla]

On Tue, Jul 13, 2021 at 08:46:19PM +0800, Xiongfeng Wang wrote:
> Hi Sudeep,
>
> On 2021/7/13 19:33, Sudeep Holla wrote:
> > On Tue, Jul 13, 2021 at 11:47:38AM +0800, Xiongfeng Wang wrote:
> >> On ARM64, when PPTT(Processor Properties Topology Table) is not
> >> implemented in ACPI boot, we will goto 'free_ci' with the following
> >> print:
> >>   Unable to detect cache hierarchy for CPU 0
> >>
> >
> > The change itself looks good and I am fine with that. However,...
> >
> >> But some other codes may still use 'num_leaves' to iterate through the
> >
> > Can you point me exactly where it is used to make sure there are no
> > other issues associated with that.
> >
> >> 'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
> >> would crash. So clear 'num_leaves' in free_cache_attributes().
> >>
> >
> > And can you provide the crash dump please ? If we are not hitting any
> > issue and you just figured this with code inspection, that is fine. It
> > helps to determine if this needs to be backport or just good to have
> > clean up.
>
> There is no issue in the mainline kernel. get_cpu_cacheinfo_id() is only called
> on x86. I didn't hit any issue using the mainline kernel.
>

Thanks for confirming.

> Actually, it's our own code that crashed. My colleague Shaobo(CCed) tried to add
> MPAM support on ARM64. His code called get_cpu_cacheinfo_id() and crashed when
> PPTT is not implemented. Maybe he should check whether PPTT is implemented
> before calling get_cpu_cacheinfo_id(), but we think it is also better to clear
> cache_leaves(cpu) in free_cache_attributes().
> Sorry for not clearly expressed.
>

Thanks for detailed explanation. In this case I would drop the Fixes: tag
as it is not fixing anything in the commit mentioned in the tag.

Also not sure if we can tag this as fixes
709c4362725a ("cacheinfo: Move resctrl's get_cache_id() to the cacheinfo header file")
as that is introducing the possible access that could crash. @James ?

Anyways, irrespective of whether you add above tag, feel free to add
once to drop the original fixes tag in the patch.

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[James Morse]

Hello,

On 13/07/2021 14:26, Sudeep Holla wrote:
> On Tue, Jul 13, 2021 at 08:46:19PM +0800, Xiongfeng Wang wrote:
>> On 2021/7/13 19:33, Sudeep Holla wrote:
>>> On Tue, Jul 13, 2021 at 11:47:38AM +0800, Xiongfeng Wang wrote:
>>>> On ARM64, when PPTT(Processor Properties Topology Table) is not
>>>> implemented in ACPI boot, we will goto 'free_ci' with the following
>>>> print:
>>>>   Unable to detect cache hierarchy for CPU 0
>>>>
>>>
>>> The change itself looks good and I am fine with that. However,...
>>>
>>>> But some other codes may still use 'num_leaves' to iterate through the
>>>
>>> Can you point me exactly where it is used to make sure there are no
>>> other issues associated with that.
>>>
>>>> 'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
>>>> would crash. So clear 'num_leaves' in free_cache_attributes().
>>>>
>>>
>>> And can you provide the crash dump please ? If we are not hitting any
>>> issue and you just figured this with code inspection, that is fine. It
>>> helps to determine if this needs to be backport or just good to have
>>> clean up.

>> There is no issue in the mainline kernel. get_cpu_cacheinfo_id() is only called
>> on x86. I didn't hit any issue using the mainline kernel.

>> Actually, it's our own code that crashed. My colleague Shaobo(CCed) tried to add

Seems to have dropped off the CC list.

>> MPAM support on ARM64.

Do you want me to CC either of you on the series that refactor the resctrl code? This is
the bit that needs doing to get MPAM working upstream

(I copy Shameerali, but I've not heard from him in a while.)


>> His code called get_cpu_cacheinfo_id() and crashed when
>> PPTT is not implemented. Maybe he should check whether PPTT is implemented
>> before calling get_cpu_cacheinfo_id(), but we think it is also better to clear
>> cache_leaves(cpu) in free_cache_attributes().
>> Sorry for not clearly expressed.

The ACPI tables for MPAM reference the PPTT, so you're going to need one.


> Thanks for detailed explanation. In this case I would drop the Fixes: tag
> as it is not fixing anything in the commit mentioned in the tag.
> 
> Also not sure if we can tag this as fixes
> 709c4362725a ("cacheinfo: Move resctrl's get_cache_id() to the cacheinfo header file")
> as that is introducing the possible access that could crash. @James ?

If you like. If there is nothing broken its hard to care.
I guess this helps people doing backports.



Thanks,

James

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Sudeep Holla]

On Tue, Jul 13, 2021 at 06:38:15PM +0100, James Morse wrote:
> Hello,
>

[...]

> If you like. If there is nothing broken its hard to care.
> I guess this helps people doing backports.
>

Thanks James, indeed I was trying to avoid unnecessary backport.

Xiongfeng,

Please don't add any fixes and post the patch with my Reviewed-by, we can
then ask Greg to pick. I am fine with the patch as is, it is nice to have
as it ensures teardown is more complete but it doesn't fix anything
in upstream or stable trees.

--
Regards,
Sudeep

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Sudeep Holla]

Sorry, I managed to accidentally delete 'o' in .com from James
email id earlier with my fat fingers I guess.

On Tue, Jul 13, 2021 at 06:48:18PM +0100, Sudeep Holla wrote:
> On Tue, Jul 13, 2021 at 06:38:15PM +0100, James Morse wrote:
> > Hello,
> >
> 
> [...]
> 
> > If you like. If there is nothing broken its hard to care.
> > I guess this helps people doing backports.
> >
> 
> Thanks James, indeed I was trying to avoid unnecessary backport.
> 
> Xiongfeng,
> 
> Please don't add any fixes and post the patch with my Reviewed-by, we can
> then ask Greg to pick. I am fine with the patch as is, it is nice to have
> as it ensures teardown is more complete but it doesn't fix anything
> in upstream or stable trees.
> 
> --
> Regards,
> Sudeep

-- 
Regards,
Sudeep

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Xiongfeng Wang]

On 2021/7/14 1:48, Sudeep Holla wrote:
> On Tue, Jul 13, 2021 at 06:38:15PM +0100, James Morse wrote:
>> Hello,
>>
> 
> [...]
> 
>> If you like. If there is nothing broken its hard to care.
>> I guess this helps people doing backports.
>>
> 
> Thanks James, indeed I was trying to avoid unnecessary backport.
> 
> Xiongfeng,
> 
> Please don't add any fixes and post the patch with my Reviewed-by, we can

Sorry, I forgot to add your Reviewed-by tag in the new version.

> then ask Greg to pick. I am fine with the patch as is, it is nice to have
> as it ensures teardown is more complete but it doesn't fix anything
> in upstream or stable trees.
> 
> --
> Regards,
> Sudeep
> .
> 

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[Xiongfeng Wang]

Hi James,

On 2021/7/14 1:38, James Morse wrote:
> Hello,
> 
> On 13/07/2021 14:26, Sudeep Holla wrote:
>> On Tue, Jul 13, 2021 at 08:46:19PM +0800, Xiongfeng Wang wrote:
>>> On 2021/7/13 19:33, Sudeep Holla wrote:
>>>> On Tue, Jul 13, 2021 at 11:47:38AM +0800, Xiongfeng Wang wrote:
>>>>> On ARM64, when PPTT(Processor Properties Topology Table) is not
>>>>> implemented in ACPI boot, we will goto 'free_ci' with the following
>>>>> print:
>>>>>   Unable to detect cache hierarchy for CPU 0
>>>>>
>>>>
>>>> The change itself looks good and I am fine with that. However,...
>>>>
>>>>> But some other codes may still use 'num_leaves' to iterate through the
>>>>
>>>> Can you point me exactly where it is used to make sure there are no
>>>> other issues associated with that.
>>>>
>>>>> 'info_list', such as get_cpu_cacheinfo_id(). If 'info_list' is NULL , it
>>>>> would crash. So clear 'num_leaves' in free_cache_attributes().
>>>>>
>>>>
>>>> And can you provide the crash dump please ? If we are not hitting any
>>>> issue and you just figured this with code inspection, that is fine. It
>>>> helps to determine if this needs to be backport or just good to have
>>>> clean up.
> 
>>> There is no issue in the mainline kernel. get_cpu_cacheinfo_id() is only called
>>> on x86. I didn't hit any issue using the mainline kernel.
> 
>>> Actually, it's our own code that crashed. My colleague Shaobo(CCed) tried to add
> 
> Seems to have dropped off the CC list.

Yes. I don't know why I didn't CC him success. CCed again.

> 
>>> MPAM support on ARM64.
> 
> Do you want me to CC either of you on the series that refactor the resctrl code? This is
> the bit that needs doing to get MPAM working upstream

It would be nice if you could CC him. His email address is
bobo.shaobowang@huawei.com. Thanks a lot !

Below is the (openEuler version) MPAM support code he wrote based on your
private version in linux-arm.org repo:
https://gitee.com/openeuler/kernel/tree/openEuler-1.0-LTS/arch/arm64/kernel/mpam
It would be appreciated if you could give some advice.

console display:
[root@localhost ~]# mount -t resctrl resctrl /sys/fs/resctrl/ -o
cdpl3,caPbm,mbHdl,mbMax
[root@localhost ~]# cd /sys/fs/resctrl/
[root@localhost resctrl]# cat schemata
L3CODEPBM:0=7fff;1=7fff;2=7fff;3=7fff
L3DATAPBM:0=7fff;1=7fff;2=7fff;3=7fff
MBHDL:0=1;1=1;2=1;3=1
MBMAX:0=100;1=100;2=100;3=100

Thanks,
Xiongfeng

> 
> (I copy Shameerali, but I've not heard from him in a while.)
> 
> 
>>> His code called get_cpu_cacheinfo_id() and crashed when
>>> PPTT is not implemented. Maybe he should check whether PPTT is implemented
>>> before calling get_cpu_cacheinfo_id(), but we think it is also better to clear
>>> cache_leaves(cpu) in free_cache_attributes().
>>> Sorry for not clearly expressed.
> 
> The ACPI tables for MPAM reference the PPTT, so you're going to need one.
> 
> 
>> Thanks for detailed explanation. In this case I would drop the Fixes: tag
>> as it is not fixing anything in the commit mentioned in the tag.
>>
>> Also not sure if we can tag this as fixes
>> 709c4362725a ("cacheinfo: Move resctrl's get_cache_id() to the cacheinfo header file")
>> as that is introducing the possible access that could crash. @James ?
> 
> If you like. If there is nothing broken its hard to care.
> I guess this helps people doing backports.
> 
> 
> 
> Thanks,
> 
> James
> .
> 

Re: [PATCH] cacheinfo: clear cache_leaves(cpu) in free_cache_attributes()

[James Morse]

Hi Xiongfeng,

(CC: dropping Greg, Rafael and Sudeep as this isn't likely to be of interest to them)

On 14/07/2021 03:10, Xiongfeng Wang wrote:
> On 2021/7/14 1:38, James Morse wrote:
>> On 13/07/2021 14:26, Sudeep Holla wrote:
>>> On Tue, Jul 13, 2021 at 08:46:19PM +0800, Xiongfeng Wang wrote:
>>>> Actually, it's our own code that crashed. My colleague Shaobo(CCed) tried to add
>>
>> Seems to have dropped off the CC list.
> 
> Yes. I don't know why I didn't CC him success. CCed again.
> 
>>
>>>> MPAM support on ARM64.
>>
>> Do you want me to CC either of you on the series that refactor the resctrl code? This is
>> the bit that needs doing to get MPAM working upstream
> 
> It would be nice if you could CC him. His email address is
> bobo.shaobowang@huawei.com. Thanks a lot !

Thanks, I will!


> Below is the (openEuler version) MPAM support code he wrote based on your
> private version in linux-arm.org repo:
> https://gitee.com/openeuler/kernel/tree/openEuler-1.0-LTS/arch/arm64/kernel/mpam
> It would be appreciated if you could give some advice.

(I've not looked in any detail, but)

Beware duplicating any of the code that is involved in the user facing ABI. This will
create subtle differences between the behaviour of x86 and arm64 for the resctrl
filesystem. This must not be allowed to happen as its bad for user-space.


> console display:
> [root@localhost ~]# mount -t resctrl resctrl /sys/fs/resctrl/ -o
> cdpl3,caPbm,mbHdl,mbMax
> [root@localhost ~]# cd /sys/fs/resctrl/
> [root@localhost resctrl]# cat schemata
> L3CODEPBM:0=7fff;1=7fff;2=7fff;3=7fff
> L3DATAPBM:0=7fff;1=7fff;2=7fff;3=7fff
> MBHDL:0=1;1=1;2=1;3=1
> MBMAX:0=100;1=100;2=100;3=100

Please don't create new 'things' visible to user-space.
You should not need to change user-space software.

The whole point of using resctrl is so that existing user-space software works, and linux
has one behaviour across all architectures. (It doesn't matter if the names are wrong,
only the behaviour matters)

If new schema are needed, we should wait until we've got the existing resctrl stuff
working upstream, then discuss it with the x86 folk, as any new schema must be possible to
support on x86 too.


Thanks,

James