On Wed, 21 May 2025 15:04:00 +0200 David Hildenbrand <david@xxxxxxxxxx> 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@xxxxxxxxxx>
Thank you for your Ack, David! I hope you have a great day!! : -)
Joshua
> --
> Cheers,
>
> David / dhildenb
