linux-kernel - Re: [PATCH] fs/resctl: use for_each_bit_from() in __check

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: <e4910487-4780-4825-8017-432960c10549@intel.com>
Date: Wed, 23 Jul 2025 08:48:38 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Yury Norov <yury.norov@...il.com>, Tony Luck <tony.luck@...el.com>, "Dave
 Martin" <Dave.Martin@....com>, James Morse <james.morse@....com>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fs/resctl: use for_each_bit_from() in __check_limbo()

Hi Yury,

Thank you very much. This change looks good to me. I do have a couple
of comments related to customs [1] to help this patch travel smoothly via the
tip tree.

For the subject, consider (typo in prefix and description starts
with upper case):
	fs/resctrl: Use for_each_bit_from() in __check_limbo()

On 7/19/25 2:40 PM, Yury Norov wrote:
> On Sat, Jul 19, 2025 at 05:34:23PM -0400, Yury Norov wrote:
>> From: Yury Norov (NVIDIA) <yury.norov@...il.com>
>>
>> The function opencodes for_each_set_bit_from(). Switch to the dedicated

"The function" -> "__check_limbo()"

Move "Switch to ..." to a new paragraph (doing so separates
problem and solution descriptions).

>> macro, and drop some housekeeping code.

It is not obvious to me what "housekeeping code" is referred to here. I think 
"and drop some housekeeping code" can be removed? If I am missing something,
please add a bit of detail about what "housekeeping code" refers to?

>>
>> While there, switch to non-atomic __clear_bit(), because atomicity is
>> not possible in this context, and that may confuse readers.
> 
> s/possible/guaranteed
> 
> Because find_next_bit() and therefore for_each() iterator are not
> atomic, we can't make sure that concurrent threads won't pick the
> same idx, therefore atomic clear_bit() may confuse readers.
> 
> But atomicity is possible if we use external lock, or
> test_and_clear_bit(), if needed.

I find this update very useful and easier to understand than the
original changelog. Please do merge it with original changelog.
To support the final sentence above you can add that the rmid_busy_llc
bitmap is always traversed with the rdtgroup_mutex held.

>> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@...il.com>
>> ---
>>  fs/resctrl/monitor.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index f5637855c3ac..3e1c14214ea8 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -135,7 +135,7 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
>>  	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
>>  	struct rmid_entry *entry;
>> -	u32 idx, cur_idx = 1;
>> +	u32 idx = 1;
>>  	void *arch_mon_ctx;
>>  	bool rmid_dirty;
>>  	u64 val = 0;

Please maintain the reverse fir ordering by moving the now shorter line
containing idx towards the end of the variable declarations.

>> @@ -153,11 +153,7 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
>>  	 * is less than the threshold decrement the busy counter of the
>>  	 * RMID and move it to the free list when the counter reaches 0.
>>  	 */
>> -	for (;;) {
>> -		idx = find_next_bit(d->rmid_busy_llc, idx_limit, cur_idx);
>> -		if (idx >= idx_limit)
>> -			break;
>> -
>> +	for_each_set_bit_from(idx, d->rmid_busy_llc, idx_limit) {
>>  		entry = __rmid_entry(idx);
>>  		if (resctrl_arch_rmid_read(r, d, entry->closid, entry->rmid,
>>  					   QOS_L3_OCCUP_EVENT_ID, &val,
>> @@ -178,11 +174,10 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free)
>>  		}
>>  
>>  		if (force_free || !rmid_dirty) {
>> -			clear_bit(idx, d->rmid_busy_llc);
>> +			__clear_bit(idx, d->rmid_busy_llc);
>>  			if (!--entry->busy)
>>  				limbo_release_entry(entry);
>>  		}
>> -		cur_idx = idx + 1;
>>  	}
>>  
>>  	resctrl_arch_mon_ctx_free(r, QOS_L3_OCCUP_EVENT_ID, arch_mon_ctx);

This looks good to me. Thank you very much.

When you resubmit, could you please also cc x86@...nel.org?

Reinette

[1] Documentation/process/maintainer-tip.rst