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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

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!

> >   	synchronize_rcu();
> > -	kfree(old);
> > +	kfree(old_wi_state);
> > +out:
> > +	kfree(&wi_group->wi_kobj);
> >   }
> 
> I'll note that this rather unrelated churn (renaming functions + 
> variables) is a bit abd for review as it adds noise. Having that as part 
> of a cleanup patch might have been better.

I see, thank you for your feedback.
I thought it might be necessary for this series, since I embedded the
iw_table inside the wi_struct, so we can no longer just free the table,
we would have to free the entire wi_state it was embedded in. I apologize if
this was difficult to review -- I agree that this patch was on the longer
side. I will do a better job of isolating parts of the patch in the future.

> Nothing else jumped at me (did not an in-depth review of the logic)
> 
> Acked-by: David Hildenbrand <david@redhat.com>

Thank you for your Ack, David! I hope you have a great day!! : -)
Joshua

> -- 
> Cheers,
> 
> David / dhildenb