Re: [PATCH v7 2/3] mm/mempolicy: Prepare weighted interleave sysfs for memory hotplug

[Rakie Kim <rakie.kim@sk.com>]

On Thu, 10 Apr 2025 16:53:33 +0900 Rakie Kim <rakie.kim@sk.com> wrote:
> On Wed, 9 Apr 2025 11:51:36 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
> > Rakie Kim wrote:
> > > > > > +static void sysfs_wi_node_delete(int nid)
> > > > > >  {
> > > > > > -	if (!node_attr)
> > > > > > +	if (!wi_group->nattrs[nid])
> > > > > >  		return;
> > > > > > -	sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
> > > > > > -	kfree(node_attr->kobj_attr.attr.name);
> > > > > > -	kfree(node_attr);
> > > > > > +
> > > > > > +	sysfs_remove_file(&wi_group->wi_kobj,
> > > > > > +			  &wi_group->nattrs[nid]->kobj_attr.attr);
> > > > > 
> > > > > This still looks broken to me, but I think this is more a problem that
> > > > > was present in the original code.
> > > > > 
> > > > > At this point @wi_group's reference count is zero because
> > > > > sysfs_wi_release() has been called. However, it can only be zero if it has
> > > > > properly transitioned through kobject_del() and final kobject_put(). It
> > > > > follows that kobject_del() arranges for kobj->sd to be NULL. That means
> > > > > that this *should* be hitting the WARN() in kernfs_remove_by_name_ns()
> > > > > for the !parent case.
> > > > > 
> > > > > So, either you are not triggering that path, or testing that path, but
> > > > > sys_remove_file() of the child attributes should be happening *before*
> > > > > sysfs_wi_release().
> > > > > 
> > > > > Did I miss something?
> > > > 
> > > > I think the missing change is that sysfs_wi_node_add() failures need to
> > > > be done with a sysfs_wi_node_delete() of the added attrs *before* the
> > > > kobject_del() of @wi_group.
> > > 
> > > Hi Dan Williams
> > > 
> > > Thank you very much for identifying this potential issue in the code.
> > > 
> > > As you pointed out, this seems to be a problem that was already present in
> > > the original implementation, and I agree that it needs to be addressed.
> > > 
> > > However, since this issue existed prior to the changes in this patch
> > > series, I believe it would be more appropriate to fix it in a separate
> > > follow-up patch rather than include it here.
> > 
> > I tend to disagree. The whole motivation of this series is to get the
> > kobject lifetime handling correct in order to add the new dynamic
> > capability. The claimed correctness fixups are incomplete. There is time
> > to respin this (we are only at -rc1) and get it right before landing the
> > new dynamic capability.
> > 
> > One of the outcomes of the "MM Process" topic at LSF/MM was that Andrew
> > wanted more feedback on when patches are not quite ready for prime-time
> > and I think this is an example of a patch set that deserves another spin
> > to meet the stated goals.
> > 
> > > I will start preparing a new patch to address this problem, and I would
> > > greatly appreciate it if you could review it once it's ready.
> > 
> > Will definitely review it. I will leave to Andrew if he wants an
> > incremental fixup on top of this series, or rebase on top of a fully
> > fixed baseline. My preference is finish fixing all the old kobject()
> > issues and then rebase the new dynamic work on top. Either way, do not
> > be afraid to ask Andrew to replace a series in -mm, that's a sign of the
> > process working as expected.
> 
> Thank you very much for your advice, and I completely agree with your
> recommendation. I will immediately ask Andrew to remove this patch series
> from -mm. Then, I will prepare a new version, v8, which properly addresses
> the kobject-related issues you pointed out.
> 
> Once again, I sincerely appreciate your thoughtful and detailed feedback.
> 
> Rakie
> 

To Andrew

I sincerely apologize for the inconvenience. It appears that this commit still
requires additional corrections. I would appreciate it if you could drop the
changes you merged into -mm, mm-new branch yesterday.

<1>
The patch titled
     Subject: mm/mempolicy: fix memory leaks in weighted interleave sysfs has been added to the -mm mm-new branch.  Its filename is
     mm-mempolicy-fix-memory-leaks-in-weighted-interleave-sysfs.patch
This patch will shortly appear at
     https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mempolicy-fix-memory-leaks-in-weighted-interleave-sysfs.patch

<2>
The patch titled
     Subject: mm/mempolicy: prepare weighted interleave sysfs for memory hotplug has been added to the -mm mm-new branch.  Its filename is
     mm-mempolicy-prepare-weighted-interleave-sysfs-for-memory-hotplug.patch
This patch will shortly appear at
     https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mempolicy-prepare-weighted-interleave-sysfs-for-memory-hotplug.patch

<3>
The patch titled
     Subject: mm/mempolicy: support memory hotplug in weighted interleave has been added to the -mm mm-new branch.  Its filename is
     mm-mempolicy-support-memory-hotplug-in-weighted-interleave.patch
This patch will shortly appear at
     https://git.kernel.org/pub/scm/linux/kernel/git/akpm/25-new.git/tree/patches/mm-mempolicy-support-memory-hotplug-in-weighted-interleave.patch

Rakie