The resctrl CLOSID allocator uses a single 32bit word to track which
CLOSID are free. The setting and clearing of bits is open coded.
A subsequent patch adds resctrl_closid_is_free(), which adds more open
coded bitmaps operations. These will eventually need changing to use
the bitops helpers so that a CLOSID bitmap of the correct size can be
allocated dynamically.
Convert the existing open coded bit manipulations of closid_free_map
to use set_bit() and friends.
Reviewed-by: Shaopeng Tan <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Tested-By: Peter Newman <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index ac1a6437469f..fa449ee0d1a7 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -106,7 +106,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)
@@ -126,7 +126,7 @@ 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;
+ clear_bit(0, &closid_free_map);
closid_free_map_len = rdt_min_closid;
}
@@ -137,14 +137,14 @@ static int closid_alloc(void)
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;
+ set_bit(closid, &closid_free_map);
}
/**
@@ -156,7 +156,7 @@ void closid_free(int closid)
*/
static bool closid_allocated(unsigned int closid)
{
- return (closid_free_map & (1 << closid)) == 0;
+ return !test_bit(closid, &closid_free_map);
}
/**
--
2.39.2
From: James Morse
> Sent: 14 September 2023 18:21
>
> The resctrl CLOSID allocator uses a single 32bit word to track which
> CLOSID are free. The setting and clearing of bits is open coded.
>
> A subsequent patch adds resctrl_closid_is_free(), which adds more open
> coded bitmaps operations. These will eventually need changing to use
> the bitops helpers so that a CLOSID bitmap of the correct size can be
> allocated dynamically.
>
> Convert the existing open coded bit manipulations of closid_free_map
> to use set_bit() and friends.
>
> int closids_supported(void)
> @@ -126,7 +126,7 @@ 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;
> + clear_bit(0, &closid_free_map);
Don't the clear_bit() etc functions use locked accesses?
These are always measurably more expensive than the C operators.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hi David,
On 17/09/2023 22:00, David Laight wrote:
> From: James Morse
>> Sent: 14 September 2023 18:21
>>
>> The resctrl CLOSID allocator uses a single 32bit word to track which
>> CLOSID are free. The setting and clearing of bits is open coded.
>>
>> A subsequent patch adds resctrl_closid_is_free(), which adds more open
>> coded bitmaps operations. These will eventually need changing to use
>> the bitops helpers so that a CLOSID bitmap of the correct size can be
>> allocated dynamically.
>>
>> Convert the existing open coded bit manipulations of closid_free_map
>> to use set_bit() and friends.
>>
>> int closids_supported(void)
>> @@ -126,7 +126,7 @@ 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;
>> + clear_bit(0, &closid_free_map);
> Don't the clear_bit() etc functions use locked accesses?
Yes. In this case there is no need for it to be atomic, just to use the bitmap API so this
can be made bigger in the future. It's currently protected by the rdtgroup_mutex (I'll add
some lockdep annotations to document that).
> These are always measurably more expensive than the C operators.
I'll switch this to use the double-underscore version which are non-atomic,
double-underscore is usually a warning not to use this function!
I doubt the performance matters as this is only ever called from a mkdir() syscall when
the configuration is changed, which we anticipate only really happens once at boot.
Thanks,
James
Hi James,
On 9/14/2023 10:21 AM, 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.
>
> A subsequent patch adds resctrl_closid_is_free(), which adds more open
resctrl_closid_is_free() is not added in this series.
> coded bitmaps operations. These will eventually need changing to use
> the bitops helpers so that a CLOSID bitmap of the correct size can be
> allocated dynamically.
>
> Convert the existing open coded bit manipulations of closid_free_map
> to use set_bit() and friends.
>
> Reviewed-by: Shaopeng Tan <[email protected]>
> Tested-by: Shaopeng Tan <[email protected]>
> Tested-By: Peter Newman <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ac1a6437469f..fa449ee0d1a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -106,7 +106,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)
> @@ -126,7 +126,7 @@ 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;
> + clear_bit(0, &closid_free_map);
> closid_free_map_len = rdt_min_closid;
> }
>
> @@ -137,14 +137,14 @@ static int closid_alloc(void)
> 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;
> + set_bit(closid, &closid_free_map);
> }
>
> /**
> @@ -156,7 +156,7 @@ void closid_free(int closid)
> */
> static bool closid_allocated(unsigned int closid)
> {
> - return (closid_free_map & (1 << closid)) == 0;
> + return !test_bit(closid, &closid_free_map);
> }
>
> /**
The patch looks good to me.
Reinette
Hi James,
On 9/14/23 12:21, 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.
>
> A subsequent patch adds resctrl_closid_is_free(), which adds more open
> coded bitmaps operations. These will eventually need changing to use
> the bitops helpers so that a CLOSID bitmap of the correct size can be
> allocated dynamically.
>
> Convert the existing open coded bit manipulations of closid_free_map
> to use set_bit() and friends.
>
> Reviewed-by: Shaopeng Tan <[email protected]>
> Tested-by: Shaopeng Tan <[email protected]>
> Tested-By: Peter Newman <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ac1a6437469f..fa449ee0d1a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -106,7 +106,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)
> @@ -126,7 +126,7 @@ 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;
> + clear_bit(0, &closid_free_map);
How about using RESCTRL_RESERVED_CLOSID instead of 0 here?
Thanks
Babu Moger
Hi Babu,
On 04/10/2023 21:38, Moger, Babu wrote:
> On 9/14/23 12:21, 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.
>>
>> A subsequent patch adds resctrl_closid_is_free(), which adds more open
>> coded bitmaps operations. These will eventually need changing to use
>> the bitops helpers so that a CLOSID bitmap of the correct size can be
>> allocated dynamically.
>>
>> Convert the existing open coded bit manipulations of closid_free_map
>> to use set_bit() and friends.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index ac1a6437469f..fa449ee0d1a7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -106,7 +106,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)
>> @@ -126,7 +126,7 @@ 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;
>> + clear_bit(0, &closid_free_map);
>
> How about using RESCTRL_RESERVED_CLOSID instead of 0 here?
Great idea - even more readable.
Thanks,
James