Re: [PATCH v9] mm/mempolicy: Weighted Interleave Auto-tuning

[David Hildenbrand <david@redhat.com>]

On 21.05.25 17:37, Joshua Hahn wrote:
> On Wed, 21 May 2025 15:04:00 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>> On 20.05.25 16:12, Joshua Hahn wrote:
> 
> [...snip...]
>   
>> [...]
>>
>>> -static void iw_table_free(void)
>>> +static void wi_state_free(void)
>>>    {
>>> -	u8 *old;
>>> +	struct weighted_interleave_state *old_wi_state;
>>>    
>>> -	mutex_lock(&iw_table_lock);
>>> -	old = rcu_dereference_protected(iw_table,
>>> -					lockdep_is_held(&iw_table_lock));
>>> -	rcu_assign_pointer(iw_table, NULL);
>>> -	mutex_unlock(&iw_table_lock);
>>> +	mutex_lock(&wi_state_lock);
>>> +
>>> +	old_wi_state = rcu_dereference_protected(wi_state,
>>> +			lockdep_is_held(&wi_state_lock));
>>> +	if (!old_wi_state) {
>>> +		mutex_unlock(&wi_state_lock);
>>> +		goto out;
>>> +	}
>>>    
>>> +	rcu_assign_pointer(wi_state, NULL);
>>> +	mutex_unlock(&wi_state_lock);
>>
>> Just one nit: if written as:
>>
>> ...
>> rcu_assign_pointer(wi_state, NULL);
>> mutex_unlock(&wi_state_lock);
>>
>> old_wi_state = ...
>> if (old_wi_state) {
>> 	synchronize_rcu();
>> 	kfree(old_wi_state);
>> }
>> kfree(&wi_group->wi_kobj);
>>
>> You can easily avoid the goto.
> 
> Ah I see, thank you for the suggestion!
> I think we would have to move the "old_wi_state = ..." to be inside
> the lock and before the rcu_assign_pointer since wi_state will be
> NULL at that point if we do not, but other than that, I think this
> is a great optimization over the version I have : -)
> 
> I will send in a fix patch for this later as a cleanup patch, if
> that sounds good with you!

Sure, you can do a cleanup on top.

-- 
Cheers,

David / dhildenb