linux-kernel - Re: [PATCH] fs/resctrl: Slightly optimize cbm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7776eb1c-418a-444b-aa24-0dfb23f05a2a@wanadoo.fr>
Date: Sat, 1 Nov 2025 14:40:58 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Dave Martin <Dave.Martin@....com>, Tony Luck <tony.luck@...el.com>
Cc: Reinette Chatre <reinette.chatre@...el.com>,
 James Morse <james.morse@....com>, Babu Moger <babu.moger@....com>,
 linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] fs/resctrl: Slightly optimize cbm_validate()

Le 27/10/2025 à 12:43, Dave Martin a écrit :
> Hi,
> 
> [Tony, I have a side question on min_cbm_bits -- see below.]
> 
> On Sun, Oct 26, 2025 at 08:39:52AM +0100, Christophe JAILLET wrote:
>> 'first_bit' is known to be 1, so it can be skipped when searching for the
>> next 0 bit. Doing so mimics bitmap_next_set_region() and can save a few
>> cycles.
> 
> This seems reasonable, although:
> 
> Nit: missing statement of what the patch does.  (Your paragraph
> describes only something that _could_ be done and gives rationale for
> it.)

Will add it in v2.

> 
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>> ---
>> Compile tested only.
>>
>> For the records, on x86, the diff of the asm code is:
>> --- fs/resctrl/ctrlmondata.s.old        2025-10-26 08:21:46.928920563 +0100
>> +++ fs/resctrl/ctrlmondata.s    2025-10-26 08:21:40.864024143 +0100
>> @@ -1603,11 +1603,12 @@
>>          call    _find_first_bit
>>   # ./include/linux/find.h:192:  return _find_next_zero_bit(addr, size, offset);
>>          movq    %r12, %rsi
>> -       leaq    48(%rsp), %rdi
>> -       movq    %rax, %rdx
>> +# fs/resctrl/ctrlmondata.c:133:        zero_bit = find_next_zero_bit(&val, cbm_len, first_bit + 1);
>> +       leaq    1(%rax), %rdx
>>   # ./include/linux/find.h:214:  return _find_first_bit(addr, size);
>>          movq    %rax, 8(%rsp)
>>   # ./include/linux/find.h:192:  return _find_next_zero_bit(addr, size, offset);
>> +       leaq    48(%rsp), %rdi
> 
> (This is really only showing that the compiler works.  The real
> question is whether the logic is still sound after this change to the
> arguments of _find_first_bit()...)

Will remove in v2, if not useful.

> 
>>          call    _find_next_zero_bit
>>   # fs/resctrl/ctrlmondata.c:136:        if (!r->cache.arch_has_sparse_bitmasks &&
>>          leaq    28(%rbx), %rdi
>> ---
>>   fs/resctrl/ctrlmondata.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
>> index 0d0ef54fc4de..1ff479a2dbbc 100644
>> --- a/fs/resctrl/ctrlmondata.c
>> +++ b/fs/resctrl/ctrlmondata.c
>> @@ -130,7 +130,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>   	}
>>   
>>   	first_bit = find_first_bit(&val, cbm_len);
>> -	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>> +	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit + 1);
> 
> Does this definitely do the right thing if val was zero?

Yes, IMHO, it does.

If val is zero, first_bit will be assigned to cbm_len (see [1]).
Then, find_next_zero_bit() will do the same because 'first_bit + 1' will 
overflow the size of the bitmap. (see [2] and [3])

The only case were we could have trouble would be to have 'first_bit + 
1' overflow and be equal to 0. I don't think that such a case is possible.

> 
>>   
>>   	/* Are non-contiguous bitmasks allowed? */
>>   	if (!r->cache.arch_has_sparse_bitmasks &&
> 
> Also, what about the find_first_bit() below?

Should be updated as well.
Will send a v2.

> 
> 
> [...]
> 
> <aside>
> 
> Also, not directly related to this patch, but, looking at the final if
> statement:
> 
> 	if ((zero_bit - first_bit) < r->cache.min_cbm_bits) {
> 	        rdt_last_cmd_printf("Need at least %d bits in the mask\n",
> 	                            r->cache.min_cbm_bits);
> 	        return false;
> 	}
> 
> If min_cbm_bits is two or greater, this can fail if the bitmap has
> enough contiguous set bits but not in the first block of set bits,
> and it can succeed if there are blocks of set bits beyond the first
> block, that have fewer than min_cbm_bits.
> 
> Is that intended?  Do we ever expect arch_has_sparse_bitmasks alongside
> min_cbm_bits > 1, or should these be mutually exclusive?
> 
> </aside>
> 
> Cheers
> ---Dave
> 
> 
CJ

[1]: 
https://elixir.bootlin.com/linux/v6.18-rc3/source/include/linux/find.h#L203
[2]: 
https://elixir.bootlin.com/linux/v6.18-rc3/source/include/linux/find.h#L185
[3]: https://elixir.bootlin.com/linux/v6.18-rc3/source/lib/find_bit.c#L55