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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250521153730.2196701-1-joshua.hahnjy@gmail.com>
Date: Wed, 21 May 2025 08:37:29 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: David Hildenbrand <david@...hat.com>
Cc: gourry@...rry.net,
	akpm@...ux-foundation.org,
	harry.yoo@...cle.com,
	ying.huang@...ux.alibaba.com,
	honggyu.kim@...com,
	yunjeong.mun@...com,
	gregkh@...uxfoundation.org,
	rakie.kim@...com,
	rafael@...nel.org,
	lenb@...nel.org,
	dan.j.williams@...el.com,
	Jonathan.Cameron@...wei.com,
	dave.jiang@...el.com,
	horen.chuang@...ux.dev,
	hannes@...xchg.org,
	osalvador@...e.de,
	linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org,
	linux-mm@...ck.org,
	kernel-team@...a.com
Subject: Re: [PATCH v9] mm/mempolicy: Weighted Interleave Auto-tuning

On Wed, 21 May 2025 15:04:00 +0200 David Hildenbrand <david@...hat.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@...hat.com>

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

> -- 
> Cheers,
> 
> David / dhildenb