[PATCH] fs/resctrl: fix domid loss precision issue

[PATCH] fs/resctrl: fix domid loss precision issue

[Rex Nie]

Below statement from mkdir_mondata_subdir function will loss precision,
because it assigns int to 14 bits bitfield.
	priv.u.domid = d->id;

This will cause below issue if cache_id > 0x3fff likes:
/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat llc_occupancy
cat: read error: No such file or directory

This is the call trace when cat llc_occupancy:
rdtgroup_mondata_show()
	domid = md.u.domid
	d = resctrl_arch_find_domain(r, domid)

d is null here because of lossing precision

Signed-off-by: Rex Nie <rex.nie@jaguarmicro.com>
Signed-off-by: Liming Wu <liming.wu@jaguar.com>
---
 fs/resctrl/internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index 7a6f46b4edd0..096317610949 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -94,7 +94,7 @@ union mon_data_bits {
 	struct {
 		unsigned int rid		: 10;
 		enum resctrl_event_id evtid	: 8;
-		unsigned int domid		: 14;
+		u32				domid;
 	} u;
 };
 
-- 
2.34.1

Re: [PATCH] fs/resctrl: fix domid loss precision issue

[Maciej Wieczor-Retman]

Hello,

On 2024-03-11 at 14:48:22 +0800, Rex Nie wrote:
>Below statement from mkdir_mondata_subdir function will loss precision,
>because it assigns int to 14 bits bitfield.
>	priv.u.domid = d->id;
>
>This will cause below issue if cache_id > 0x3fff likes:

Is there some reason for cache_id ever being this high?

I thought the max for cache_id was the amount of L3 caches on a system. And I
only observed it going up to 3 on some server platforms. So not nearly in the
range of 0x3fff or 16k.

>/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat llc_occupancy

How did you get this file to appear? Could you maybe show how your mon_data
directory looks like?

>cat: read error: No such file or directory
>
>This is the call trace when cat llc_occupancy:
>rdtgroup_mondata_show()
>	domid = md.u.domid
>	d = resctrl_arch_find_domain(r, domid)
>
>d is null here because of lossing precision
>
>Signed-off-by: Rex Nie <rex.nie@jaguarmicro.com>
>Signed-off-by: Liming Wu <liming.wu@jaguar.com>
>---
> fs/resctrl/internal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>index 7a6f46b4edd0..096317610949 100644
>--- a/fs/resctrl/internal.h
>+++ b/fs/resctrl/internal.h
>@@ -94,7 +94,7 @@ union mon_data_bits {
> 	struct {
> 		unsigned int rid		: 10;
> 		enum resctrl_event_id evtid	: 8;
>-		unsigned int domid		: 14;
>+		u32				domid;
> 	} u;
> };
> 
>-- 
>2.34.1
>

-- 
Kind regards
Maciej Wieczór-Retman

答复: [PATCH] fs/resctrl: fix domid loss precision issue

[Rex Nie]

Hello,
	Please kindly check my inline reply. Thanks.
Best regards
Rex Nie

> -----邮件原件-----
> 发件人: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 发送时间: 2024年3月11日 16:24
> 收件人: Rex Nie <rex.nie@jaguarmicro.com>
> 抄送: james.morse@arm.com; fenghua.yu@intel.com;
> reinette.chatre@intel.com; linux-kernel@vger.kernel.org; Liming Wu
> <liming.wu@jaguar.com>
> 主题: Re: [PATCH] fs/resctrl: fix domid loss precision issue
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> Hello,
> 
> On 2024-03-11 at 14:48:22 +0800, Rex Nie wrote:
> >Below statement from mkdir_mondata_subdir function will loss precision,
> >because it assigns int to 14 bits bitfield.
> >       priv.u.domid = d->id;
> >
> >This will cause below issue if cache_id > 0x3fff likes:
> 
> Is there some reason for cache_id ever being this high?
> 
> I thought the max for cache_id was the amount of L3 caches on a system. And I
> only observed it going up to 3 on some server platforms. So not nearly in the
> range of 0x3fff or 16k.
> 
It is exactly as you said on X86 platforms, but cache_Id on Arm platform is different.
According to ACPI for mpam, cache id is used as locator for cache MSC. Reference to RD_PPTT_CACHE_ID definition from edk2-platforms:
#define RD_PPTT_CACHE_ID(PackageId, ClusterId, CoreId, CacheType)         \
	(                                                              \
	  (((PackageId) & 0xF) << 20) | (((ClusterId) & 0xFF) << 12) |              \
	  (((CoreId) & 0xFF) << 4) | ((CacheType) & 0xF)                        \
	)
So it may be > 0x3fff on Arm platform.

Reference RD_PPTT_CACHE_ID from edk2-platforms: https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h#L202

> >/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat
> >llc_occupancy
> 
> How did you get this file to appear? Could you maybe show how your
> mon_data directory looks like?
> 
I found this issue on Arm FVP N1 platform and my N2 platform.

Below is the steps on Arm FVP N1:
mount -t resctrl resctrl / /sys/fs/resctrl
cd /sys/fs/resctrl/mon_data

/sys/fs/resctrl/mon_data # ls -l
total 0
dr-xr-xr-x    2 0        0                0 Mar 11 09:24 mon_L3_1048564

cd /sys/fs/resctrl/mon_data # cd mon_L3_1048564
/sys/fs/resctrl/mon_data/mon_L3_1048564 # cat llc_occupancy
cat: read error: No such file or directory

Arm FVP MPAM: https://neoverse-reference-design.docs.arm.com/en/latest/mpam/mpam-resctrl.html#memory-system-resource-partitioning-and-monitoring-mpam

> >cat: read error: No such file or directory
> >
> >This is the call trace when cat llc_occupancy:
> >rdtgroup_mondata_show()
> >       domid = md.u.domid
> >       d = resctrl_arch_find_domain(r, domid)
> >
> >d is null here because of lossing precision
> >
> >Signed-off-by: Rex Nie <rex.nie@jaguarmicro.com>
> >Signed-off-by: Liming Wu <liming.wu@jaguar.com>
> >---
> > fs/resctrl/internal.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h index
> >7a6f46b4edd0..096317610949 100644
> >--- a/fs/resctrl/internal.h
> >+++ b/fs/resctrl/internal.h
> >@@ -94,7 +94,7 @@ union mon_data_bits {
> >       struct {
> >               unsigned int rid                : 10;
> >               enum resctrl_event_id evtid     : 8;
> >-              unsigned int domid              : 14;
> >+              u32                             domid;
> >       } u;
> > };
> >
> >--
> >2.34.1
> >
> 
> --
> Kind regards
> Maciej Wieczór-Retman

Re: 答复: [PATCH] fs/resctrl: fix domid loss precision issue

[Maciej Wieczor-Retman]

Thanks for the reply,

On 2024-03-11 at 09:37:37 +0000, Rex Nie wrote:
>Hello,
>	Please kindly check my inline reply. Thanks.
>Best regards
>Rex Nie
>
>> >This will cause below issue if cache_id > 0x3fff likes:
>> 
>> Is there some reason for cache_id ever being this high?
>> 
>> I thought the max for cache_id was the amount of L3 caches on a system. And I
>> only observed it going up to 3 on some server platforms. So not nearly in the
>> range of 0x3fff or 16k.
>> 
>It is exactly as you said on X86 platforms, but cache_Id on Arm platform is different.
>According to ACPI for mpam, cache id is used as locator for cache MSC. Reference to RD_PPTT_CACHE_ID definition from edk2-platforms:
>#define RD_PPTT_CACHE_ID(PackageId, ClusterId, CoreId, CacheType)         \
>	(                                                              \
>	  (((PackageId) & 0xF) << 20) | (((ClusterId) & 0xFF) << 12) |              \
>	  (((CoreId) & 0xFF) << 4) | ((CacheType) & 0xF)                        \
>	)
>So it may be > 0x3fff on Arm platform.
>
>Reference RD_PPTT_CACHE_ID from edk2-platforms: https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h#L202

and thanks for clearing it up for me! I browsed some MPAM patches but didn't
notice cache_id was used differently on ARM.

>
>> >/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat
>> >llc_occupancy
>> 
>> How did you get this file to appear? Could you maybe show how your
>> mon_data directory looks like?
>> 
>I found this issue on Arm FVP N1 platform and my N2 platform.
>
>Below is the steps on Arm FVP N1:
>mount -t resctrl resctrl / /sys/fs/resctrl
>cd /sys/fs/resctrl/mon_data
>
>/sys/fs/resctrl/mon_data # ls -l
>total 0
>dr-xr-xr-x    2 0        0                0 Mar 11 09:24 mon_L3_1048564
>
>cd /sys/fs/resctrl/mon_data # cd mon_L3_1048564
>/sys/fs/resctrl/mon_data/mon_L3_1048564 # cat llc_occupancy
>cat: read error: No such file or directory
>
>Arm FVP MPAM: https://neoverse-reference-design.docs.arm.com/en/latest/mpam/mpam-resctrl.html#memory-system-resource-partitioning-and-monitoring-mpam
>

-- 
Kind regards
Maciej Wieczór-Retman

答复: 答复: [PATCH] fs/resctrl: fix domid loss precision issue

[Rex Nie]

Hello,
	Reply as below.
Best regards
Rex

> -----邮件原件-----
> 发件人: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
> 发送时间: 2024年3月11日 19:50
> 收件人: Rex Nie <rex.nie@jaguarmicro.com>
> 抄送: james.morse@arm.com; fenghua.yu@intel.com;
> reinette.chatre@intel.com; linux-kernel@vger.kernel.org; Liming Wu
> <liming.wu@jaguar.com>
> 主题: Re: 答复: [PATCH] fs/resctrl: fix domid loss precision issue
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> Thanks for the reply,
> 
> On 2024-03-11 at 09:37:37 +0000, Rex Nie wrote:
> >Hello,
> >       Please kindly check my inline reply. Thanks.
> >Best regards
> >Rex Nie
> >
> >> >This will cause below issue if cache_id > 0x3fff likes:
> >>
> >> Is there some reason for cache_id ever being this high?
> >>
> >> I thought the max for cache_id was the amount of L3 caches on a
> >> system. And I only observed it going up to 3 on some server
> >> platforms. So not nearly in the range of 0x3fff or 16k.
> >>
> >It is exactly as you said on X86 platforms, but cache_Id on Arm platform is
> different.
> >According to ACPI for mpam, cache id is used as locator for cache MSC.
> Reference to RD_PPTT_CACHE_ID definition from edk2-platforms:
> >#define RD_PPTT_CACHE_ID(PackageId, ClusterId, CoreId, CacheType)
> \
> >
> (                                                              \
> >         (((PackageId) & 0xF) << 20) | (((ClusterId) & 0xFF) << 12) |
> \
> >         (((CoreId) & 0xFF) << 4) | ((CacheType) & 0xF)
> \
> >       )
> >So it may be > 0x3fff on Arm platform.
> >
> >Reference RD_PPTT_CACHE_ID from edk2-platforms:
> >https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/Sg
> >iPkg/Include/SgiAcpiHeader.h#L202
> 
> and thanks for clearing it up for me! I browsed some MPAM patches but didn't
> notice cache_id was used differently on ARM.
> 
Yes, the mpam driver from kernel use the cache_id as component id, domid directly reported from mpam acpi table(struct acpi_table_mpam_msc_res->locator1),
so the use of cache_id in mpam driver is NO different, but the definition of cache_id, or how to generate cache_id from firmware side(UEFI) is different.

I take 2 commits from edk2-platforms of ARM FVP for your reference.
https://git.gitlab.arm.com/arm-reference-solutions/edk2-platforms/-/commit/d9f3290b0ba63dcc4b124017f6dede5afb681cbb
https://git.gitlab.arm.com/arm-reference-solutions/edk2-platforms/-/commit/08fe90e90a84c7ae074ddcfcfa58e73ea7c03a49


> >
> >> >/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat
> >> >llc_occupancy
> >>
> >> How did you get this file to appear? Could you maybe show how your
> >> mon_data directory looks like?
> >>
> >I found this issue on Arm FVP N1 platform and my N2 platform.
> >
> >Below is the steps on Arm FVP N1:
> >mount -t resctrl resctrl / /sys/fs/resctrl cd /sys/fs/resctrl/mon_data
> >
> >/sys/fs/resctrl/mon_data # ls -l
> >total 0
> >dr-xr-xr-x    2 0        0                0 Mar 11 09:24
> mon_L3_1048564
> >
> >cd /sys/fs/resctrl/mon_data # cd mon_L3_1048564
> >/sys/fs/resctrl/mon_data/mon_L3_1048564 # cat llc_occupancy
> >cat: read error: No such file or directory
> >
> >Arm FVP MPAM:
> >https://neoverse-reference-design.docs.arm.com/en/latest/mpam/mpam-res
> c
> >trl.html#memory-system-resource-partitioning-and-monitoring-mpam
> >
> 
> --
> Kind regards
> Maciej Wieczór-Retman

Re: 答复: [PATCH] fs/resctrl: fix domid loss precision issue

[Ilpo Järvinen]

On Mon, 11 Mar 2024, Maciej Wieczor-Retman wrote:

> Thanks for the reply,
> 
> On 2024-03-11 at 09:37:37 +0000, Rex Nie wrote:
> >Hello,
> >	Please kindly check my inline reply. Thanks.
> >Best regards
> >Rex Nie
> >
> >> >This will cause below issue if cache_id > 0x3fff likes:
> >> 
> >> Is there some reason for cache_id ever being this high?
> >> 
> >> I thought the max for cache_id was the amount of L3 caches on a system. And I
> >> only observed it going up to 3 on some server platforms. So not nearly in the
> >> range of 0x3fff or 16k.
> >> 
> >It is exactly as you said on X86 platforms, but cache_Id on Arm platform is different.
> >According to ACPI for mpam, cache id is used as locator for cache MSC. Reference to RD_PPTT_CACHE_ID definition from edk2-platforms:
> >#define RD_PPTT_CACHE_ID(PackageId, ClusterId, CoreId, CacheType)         \
> >	(                                                              \
> >	  (((PackageId) & 0xF) << 20) | (((ClusterId) & 0xFF) << 12) |              \
> >	  (((CoreId) & 0xFF) << 4) | ((CacheType) & 0xF)                        \
> >	)
> >So it may be > 0x3fff on Arm platform.

Hi Rex,

Please also put that kind of knowledge into the commit message upfront. No 
need to be as verbose as you're here (with code quotes, etc.) but stating 
that some platforms use higher IDs (e.g., Arm) would be pretty useful in 
answering the question why you're doing this change (which is one of the 
key points of describing your change).

-- 
 i.

> >Reference RD_PPTT_CACHE_ID from edk2-platforms: https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/SgiPkg/Include/SgiAcpiHeader.h#L202
> 
> and thanks for clearing it up for me! I browsed some MPAM patches but didn't
> notice cache_id was used differently on ARM.
> 
> >
> >> >/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat
> >> >llc_occupancy
> >> 
> >> How did you get this file to appear? Could you maybe show how your
> >> mon_data directory looks like?
> >> 
> >I found this issue on Arm FVP N1 platform and my N2 platform.
> >
> >Below is the steps on Arm FVP N1:
> >mount -t resctrl resctrl / /sys/fs/resctrl
> >cd /sys/fs/resctrl/mon_data
> >
> >/sys/fs/resctrl/mon_data # ls -l
> >total 0
> >dr-xr-xr-x    2 0        0                0 Mar 11 09:24 mon_L3_1048564
> >
> >cd /sys/fs/resctrl/mon_data # cd mon_L3_1048564
> >/sys/fs/resctrl/mon_data/mon_L3_1048564 # cat llc_occupancy
> >cat: read error: No such file or directory
> >
> >Arm FVP MPAM: https://neoverse-reference-design.docs.arm.com/en/latest/mpam/mpam-resctrl.html#memory-system-resource-partitioning-and-monitoring-mpam
> >
> 
> 

答复: 答复: [PATCH] fs/resctrl: fix domid loss precision issue

[Rex Nie]

Ok, I apply it as patch v2, and thanks for your comment.

> -----邮件原件-----
> 发件人: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 发送时间: 2024年3月11日 20:18
> 收件人: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>; Rex Nie
> <rex.nie@jaguarmicro.com>
> 抄送: james.morse@arm.com; fenghua.yu@intel.com;
> reinette.chatre@intel.com; linux-kernel@vger.kernel.org; Liming Wu
> <liming.wu@jaguar.com>
> 主题: Re: 答复: [PATCH] fs/resctrl: fix domid loss precision issue
> 
> External Mail: This email originated from OUTSIDE of the organization!
> Do not click links, open attachments or provide ANY information unless you
> recognize the sender and know the content is safe.
> 
> 
> On Mon, 11 Mar 2024, Maciej Wieczor-Retman wrote:
> 
> > Thanks for the reply,
> >
> > On 2024-03-11 at 09:37:37 +0000, Rex Nie wrote:
> > >Hello,
> > >     Please kindly check my inline reply. Thanks.
> > >Best regards
> > >Rex Nie
> > >
> > >> >This will cause below issue if cache_id > 0x3fff likes:
> > >>
> > >> Is there some reason for cache_id ever being this high?
> > >>
> > >> I thought the max for cache_id was the amount of L3 caches on a
> > >> system. And I only observed it going up to 3 on some server
> > >> platforms. So not nearly in the range of 0x3fff or 16k.
> > >>
> > >It is exactly as you said on X86 platforms, but cache_Id on Arm platform is
> different.
> > >According to ACPI for mpam, cache id is used as locator for cache MSC.
> Reference to RD_PPTT_CACHE_ID definition from edk2-platforms:
> > >#define RD_PPTT_CACHE_ID(PackageId, ClusterId, CoreId, CacheType)
> \
> > >
> (                                                              \
> > >       (((PackageId) & 0xF) << 20) | (((ClusterId) & 0xFF) << 12) |
> \
> > >       (((CoreId) & 0xFF) << 4) | ((CacheType) & 0xF)
> \
> > >     )
> > >So it may be > 0x3fff on Arm platform.
> 
> Hi Rex,
> 
> Please also put that kind of knowledge into the commit message upfront. No
> need to be as verbose as you're here (with code quotes, etc.) but stating that
> some platforms use higher IDs (e.g., Arm) would be pretty useful in answering
> the question why you're doing this change (which is one of the key points of
> describing your change).
> 
> --
>  i.
> 
> > >Reference RD_PPTT_CACHE_ID from edk2-platforms:
> > >https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/
> > >SgiPkg/Include/SgiAcpiHeader.h#L202
> >
> > and thanks for clearing it up for me! I browsed some MPAM patches but
> > didn't notice cache_id was used differently on ARM.
> >
> > >
> > >> >/sys/fs/resctrl/mon_groups/p1/mon_data/mon_L3_1048564 # cat
> > >> >llc_occupancy
> > >>
> > >> How did you get this file to appear? Could you maybe show how your
> > >> mon_data directory looks like?
> > >>
> > >I found this issue on Arm FVP N1 platform and my N2 platform.
> > >
> > >Below is the steps on Arm FVP N1:
> > >mount -t resctrl resctrl / /sys/fs/resctrl cd
> > >/sys/fs/resctrl/mon_data
> > >
> > >/sys/fs/resctrl/mon_data # ls -l
> > >total 0
> > >dr-xr-xr-x    2 0        0                0 Mar 11 09:24
> mon_L3_1048564
> > >
> > >cd /sys/fs/resctrl/mon_data # cd mon_L3_1048564
> > >/sys/fs/resctrl/mon_data/mon_L3_1048564 # cat llc_occupancy
> > >cat: read error: No such file or directory
> > >
> > >Arm FVP MPAM:
> > >https://neoverse-reference-design.docs.arm.com/en/latest/mpam/mpam-re
> > >sctrl.html#memory-system-resource-partitioning-and-monitoring-mpam
> > >
> >
> >