Re: [PATCH] fs/resctrl: Slightly optimize cbm_validate()
From: Christophe JAILLET
Date: Sat Nov 01 2025 - 09:42:12 EST
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@xxxxxxxxxx>
---
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.
CJ
[...]
<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
[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