fs/resctl: use for_each_bit_from() in __check_limbo()

[PATCH] fs/resctl: use for_each_bit_from() in __check_limbo()

Posted by Yury Norov 2 months, 2 weeks ago

From: Yury Norov (NVIDIA) <yury.norov@gmail.com>

The function opencodes for_each_set_bit_from(). Switch to the dedicated
macro, and drop some housekeeping code.

While there, switch to non-atomic __clear_bit(), because atomicity is
not possible in this context, and that may confuse readers.

Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.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;
@@ -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);
-- 
2.43.0

Re: [PATCH] fs/resctl: use for_each_bit_from() in __check_limbo()

Posted by Yury Norov 2 months, 2 weeks ago

On Sat, Jul 19, 2025 at 05:34:23PM -0400, Yury Norov wrote:
> From: Yury Norov (NVIDIA) <yury.norov@gmail.com>
> 
> The function opencodes for_each_set_bit_from(). Switch to the dedicated
> macro, and drop some housekeeping code.
> 
> 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.

> 
> Signed-off-by: Yury Norov (NVIDIA) <yury.norov@gmail.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;
> @@ -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);
> -- 
> 2.43.0

Re: [PATCH] fs/resctl: use for_each_bit_from() in __check_limbo()

Posted by Reinette Chatre 2 months, 2 weeks ago

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@gmail.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@gmail.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@kernel.org?

Reinette

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