The resctrl CLOSID allocator uses a single 32bit word to track which
CLOSID are free. The setting and clearing of bits is open coded.
Convert the existing open coded bit manipulations of closid_free_map
to use __set_bit() and friends. These don't need to be atomic as this
list is protected by the mutex.
Signed-off-by: James Morse <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Tested-by: Peter Newman <[email protected]>
Tested-by: Babu Moger <[email protected]>
Tested-by: Carl Worth <[email protected]> # arm64
Reviewed-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Babu Moger <[email protected]>
---
Changes since v6:
* Use the __ inatomic helpers and add lockdep_assert_held() annotations to
document how this is safe.
* Fixed a resctrl_closid_is_free()/closid_allocated() rename in the commit
message.
* Use RESCTRL_RESERVED_CLOSID to improve readability.
Changes since v7:
* Removed paragraph explaining why this should be done now due to badword
'subsequent'.
* Changed a comment to refer to RESCTRL_RESERVED_CLOSID.
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index dcffd1c4a476..bc6e0f83c847 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -111,7 +111,7 @@ void rdt_staged_configs_clear(void)
* - Our choices on how to configure each resource become progressively more
* limited as the number of resources grows.
*/
-static int closid_free_map;
+static unsigned long closid_free_map;
static int closid_free_map_len;
int closids_supported(void)
@@ -130,8 +130,8 @@ static void closid_init(void)
closid_free_map = BIT_MASK(rdt_min_closid) - 1;
- /* CLOSID 0 is always reserved for the default group */
- closid_free_map &= ~1;
+ /* RESCTRL_RESERVED_CLOSID is always reserved for the default group */
+ __clear_bit(RESCTRL_RESERVED_CLOSID, &closid_free_map);
closid_free_map_len = rdt_min_closid;
}
@@ -139,17 +139,21 @@ static int closid_alloc(void)
{
u32 closid = ffs(closid_free_map);
+ lockdep_assert_held(&rdtgroup_mutex);
+
if (closid == 0)
return -ENOSPC;
closid--;
- closid_free_map &= ~(1 << closid);
+ __clear_bit(closid, &closid_free_map);
return closid;
}
void closid_free(int closid)
{
- closid_free_map |= 1 << closid;
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ __set_bit(closid, &closid_free_map);
}
/**
@@ -161,7 +165,9 @@ void closid_free(int closid)
*/
static bool closid_allocated(unsigned int closid)
{
- return (closid_free_map & (1 << closid)) == 0;
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ return !test_bit(closid, &closid_free_map);
}
/**
--
2.39.2
The following commit has been merged into the x86/cache branch of tip:
Commit-ID: 5d920b6881f2249be3a028ce0a7f31c5cc61b1ee
Gitweb: https://git.kernel.org/tip/5d920b6881f2249be3a028ce0a7f31c5cc61b1ee
Author: James Morse <[email protected]>
AuthorDate: Tue, 13 Feb 2024 18:44:23
Committer: Borislav Petkov (AMD) <[email protected]>
CommitterDate: Fri, 16 Feb 2024 19:18:31 +01:00
x86/resctrl: Use __set_bit()/__clear_bit() instead of open coding
The resctrl CLOSID allocator uses a single 32bit word to track which
CLOSID are free. The setting and clearing of bits is open coded.
Convert the existing open coded bit manipulations of closid_free_map
to use __set_bit() and friends. These don't need to be atomic as this
list is protected by the mutex.
Signed-off-by: James Morse <[email protected]>
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Reviewed-by: Shaopeng Tan <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Reviewed-by: Babu Moger <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Tested-by: Peter Newman <[email protected]>
Tested-by: Babu Moger <[email protected]>
Tested-by: Carl Worth <[email protected]> # arm64
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index dcffd1c..bc6e0f8 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -111,7 +111,7 @@ void rdt_staged_configs_clear(void)
* - Our choices on how to configure each resource become progressively more
* limited as the number of resources grows.
*/
-static int closid_free_map;
+static unsigned long closid_free_map;
static int closid_free_map_len;
int closids_supported(void)
@@ -130,8 +130,8 @@ static void closid_init(void)
closid_free_map = BIT_MASK(rdt_min_closid) - 1;
- /* CLOSID 0 is always reserved for the default group */
- closid_free_map &= ~1;
+ /* RESCTRL_RESERVED_CLOSID is always reserved for the default group */
+ __clear_bit(RESCTRL_RESERVED_CLOSID, &closid_free_map);
closid_free_map_len = rdt_min_closid;
}
@@ -139,17 +139,21 @@ static int closid_alloc(void)
{
u32 closid = ffs(closid_free_map);
+ lockdep_assert_held(&rdtgroup_mutex);
+
if (closid == 0)
return -ENOSPC;
closid--;
- closid_free_map &= ~(1 << closid);
+ __clear_bit(closid, &closid_free_map);
return closid;
}
void closid_free(int closid)
{
- closid_free_map |= 1 << closid;
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ __set_bit(closid, &closid_free_map);
}
/**
@@ -161,7 +165,9 @@ void closid_free(int closid)
*/
static bool closid_allocated(unsigned int closid)
{
- return (closid_free_map & (1 << closid)) == 0;
+ lockdep_assert_held(&rdtgroup_mutex);
+
+ return !test_bit(closid, &closid_free_map);
}
/**
On 13.02.24 19:44, James Morse wrote:
> The resctrl CLOSID allocator uses a single 32bit word to track which
> CLOSID are free. The setting and clearing of bits is open coded.
>
> Convert the existing open coded bit manipulations of closid_free_map
> to use __set_bit() and friends. These don't need to be atomic as this
> list is protected by the mutex.
>
> Signed-off-by: James Morse <[email protected]>
> Tested-by: Shaopeng Tan <[email protected]>
> Tested-by: Peter Newman <[email protected]>
> Tested-by: Babu Moger <[email protected]>
> Tested-by: Carl Worth <[email protected]> # arm64
> Reviewed-by: Shaopeng Tan <[email protected]>
> Reviewed-by: Reinette Chatre <[email protected]>
> Reviewed-by: Babu Moger <[email protected]>
> ---
> Changes since v6:
> * Use the __ inatomic helpers and add lockdep_assert_held() annotations to
> document how this is safe.
> * Fixed a resctrl_closid_is_free()/closid_allocated() rename in the commit
> message.
> * Use RESCTRL_RESERVED_CLOSID to improve readability.
>
> Changes since v7:
> * Removed paragraph explaining why this should be done now due to badword
> 'subsequent'.
> * Changed a comment to refer to RESCTRL_RESERVED_CLOSID.
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index dcffd1c4a476..bc6e0f83c847 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -111,7 +111,7 @@ void rdt_staged_configs_clear(void)
> * - Our choices on how to configure each resource become progressively more
> * limited as the number of resources grows.
> */
That comment talks about "free CLOSIDs in a single integer". Once could
think about rephrasing that to "free CLOSIDs in a simple bitmap."
> -static int closid_free_map;
> +static unsigned long closid_free_map;
> static int closid_free_map_len;
>
> int closids_supported(void)
> @@ -130,8 +130,8 @@ static void closid_init(void)
>
> closid_free_map = BIT_MASK(rdt_min_closid) - 1;
Now that we use "unsigned long", I was wondering for a second if we
should use "proper" bitmap functions here like
bitmap_fill(closid_free_map, rdt_min_closid);
and converting the alloc path (replace ffs() in closid_alloc()):
closid = find_first_bit(closid_free_map, closid_free_map_len);
if (closid == closid_free_map_len)
return -ENOSPC;
__clear_bit(closid, &closid_free_map);
(would get rid of the closid-- in closid_alloc())
Just a thought, so
Reviewed-by: David Hildenbrand <[email protected]>
--
Cheers,
David / dhildenb
Hi David,
On 20/02/2024 16:00, David Hildenbrand wrote:
> On 13.02.24 19:44, James Morse wrote:
>> The resctrl CLOSID allocator uses a single 32bit word to track which
>> CLOSID are free. The setting and clearing of bits is open coded.
>>
>> Convert the existing open coded bit manipulations of closid_free_map
>> to use __set_bit() and friends. These don't need to be atomic as this
>> list is protected by the mutex.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index dcffd1c4a476..bc6e0f83c847 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -111,7 +111,7 @@ void rdt_staged_configs_clear(void)
>> * - Our choices on how to configure each resource become progressively more
>> * limited as the number of resources grows.
>> */
>
> That comment talks about "free CLOSIDs in a single integer". Once could think about
> rephrasing that to "free CLOSIDs in a simple bitmap."
>
>> -static int closid_free_map;
>> +static unsigned long closid_free_map;
>> static int closid_free_map_len;
>> int closids_supported(void)
>> @@ -130,8 +130,8 @@ static void closid_init(void)
>> closid_free_map = BIT_MASK(rdt_min_closid) - 1;
>
> Now that we use "unsigned long", I was wondering for a second if we should use "proper"
> bitmap functions here like
>
> bitmap_fill(closid_free_map, rdt_min_closid);
>
> and converting the alloc path (replace ffs() in closid_alloc()):
>
> closid = find_first_bit(closid_free_map, closid_free_map_len);
> if (closid == closid_free_map_len)
> return -ENOSPC;
> __clear_bit(closid, &closid_free_map);
>
> (would get rid of the closid-- in closid_alloc())
Yup. I have this as something to post after all the MPAM changes as it's not necessary to
get MPAM going. The patch[0] uses the bitmap APIs you suggest to remove the fixed limit on
the number of CLOSID/PARTID.
MPAM systems are being built with more than 32, but will work without that patch.
> Just a thought, so
>
> Reviewed-by: David Hildenbrand <[email protected]>
Thanks!
James
[0]
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=mpam/snapshot/v6.7-rc2&id=b530deed244d9b45f3bce3cccde91f6ed0ebf7ea
On 20.02.24 17:27, James Morse wrote:
> Hi David,
>
> On 20/02/2024 16:00, David Hildenbrand wrote:
>> On 13.02.24 19:44, James Morse wrote:
>>> The resctrl CLOSID allocator uses a single 32bit word to track which
>>> CLOSID are free. The setting and clearing of bits is open coded.
>>>
>>> Convert the existing open coded bit manipulations of closid_free_map
>>> to use __set_bit() and friends. These don't need to be atomic as this
>>> list is protected by the mutex.
>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index dcffd1c4a476..bc6e0f83c847 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -111,7 +111,7 @@ void rdt_staged_configs_clear(void)
>>> * - Our choices on how to configure each resource become progressively more
>>> * limited as the number of resources grows.
>>> */
>>
>> That comment talks about "free CLOSIDs in a single integer". Once could think about
>> rephrasing that to "free CLOSIDs in a simple bitmap."
>>
>>> -static int closid_free_map;
>>> +static unsigned long closid_free_map;
>>> static int closid_free_map_len;
>>> int closids_supported(void)
>>> @@ -130,8 +130,8 @@ static void closid_init(void)
>>> closid_free_map = BIT_MASK(rdt_min_closid) - 1;
>>
>> Now that we use "unsigned long", I was wondering for a second if we should use "proper"
>> bitmap functions here like
>>
>> bitmap_fill(closid_free_map, rdt_min_closid);
>>
>> and converting the alloc path (replace ffs() in closid_alloc()):
>>
>> closid = find_first_bit(closid_free_map, closid_free_map_len);
>> if (closid == closid_free_map_len)
>> return -ENOSPC;
>> __clear_bit(closid, &closid_free_map);
>>
>> (would get rid of the closid-- in closid_alloc())
>
> Yup. I have this as something to post after all the MPAM changes as it's not necessary to
> get MPAM going. The patch[0] uses the bitmap APIs you suggest to remove the fixed limit on
> the number of CLOSID/PARTID.
> MPAM systems are being built with more than 32, but will work without that patch.
Make sense, thanks!
--
Cheers,
David / dhildenb