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: <aP9a9ZtigAWCWSWk@e133380.arm.com>
Date: Mon, 27 Oct 2025 11:43:49 +0000
From: Dave Martin <Dave.Martin@....com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
	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()

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.)

> 
> 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()...)

>         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?

>  
>  	/* Are non-contiguous bitmasks allowed? */
>  	if (!r->cache.arch_has_sparse_bitmasks &&

Also, what about the find_first_bit() below?


[...]

<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