[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250402012845.1064-1-rakie.kim@sk.com>
Date: Wed, 2 Apr 2025 10:28:37 +0900
From: Rakie Kim <rakie.kim@...com>
To: Gregory Price <gourry@...rry.net>
Cc: akpm@...ux-foundation.org,
linux-mm@...ck.org,
linux-kernel@...r.kernel.org,
linux-cxl@...r.kernel.org,
joshua.hahnjy@...il.com,
dan.j.williams@...el.com,
ying.huang@...ux.alibaba.com,
david@...hat.com,
Jonathan.Cameron@...wei.com,
kernel_team@...ynix.com,
honggyu.kim@...com,
yunjeong.mun@...com,
Rakie Kim <rakie.kim@...com>
Subject: Re: [PATCH v4 3/3] mm/mempolicy: Support memory hotplug in weighted interleave
On Tue, 1 Apr 2025 16:32:42 -0400 Gregory Price <gourry@...rry.net> wrote:
> On Tue, Apr 01, 2025 at 06:08:59PM +0900, Rakie Kim wrote:
> > static void sysfs_wi_release(struct kobject *wi_kobj)
> > @@ -3464,35 +3477,84 @@ static const struct kobj_type wi_ktype = {
> >
> > static int sysfs_wi_node_add(int nid)
> > {
> ... snip ..
> > + mutex_lock(&wi_group->kobj_lock);
> > + if (wi_group->nattrs[nid]) {
> > + mutex_unlock(&wi_group->kobj_lock);
> > + pr_info("Node [%d] already exists\n", nid);
> > + kfree(new_attr);
> > + kfree(name);
> > + return 0;
> > + }
> >
> > - if (sysfs_create_file(&wi_group->wi_kobj, &node_attr->kobj_attr.attr)) {
> > - kfree(node_attr->kobj_attr.attr.name);
> > - kfree(node_attr);
> > - pr_err("failed to add attribute to weighted_interleave\n");
> > - return -ENOMEM;
> > + wi_group->nattrs[nid] = new_attr;
> > + mutex_unlock(&wi_group->kobj_lock);
> > +
>
> Shouldn't all of this
> vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
> > + sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
> > + wi_group->nattrs[nid]->kobj_attr.attr.name = name;
> > + wi_group->nattrs[nid]->kobj_attr.attr.mode = 0644;
> > + wi_group->nattrs[nid]->kobj_attr.show = node_show;
> > + wi_group->nattrs[nid]->kobj_attr.store = node_store;
> > + wi_group->nattrs[nid]->nid = nid;
> > +
> > + ret = sysfs_create_file(&wi_group->wi_kobj,
> > + &wi_group->nattrs[nid]->kobj_attr.attr);
> > + if (ret) {
> > + kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> > + kfree(wi_group->nattrs[nid]);
> > + wi_group->nattrs[nid] = NULL;
> > + pr_err("Failed to add attribute to weighted_interleave: %d\n", ret);
> > }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Be happening inside the lock as well?
I agree that applying your suggestion would make the code more robust.
I will update the patch to follow your recommendation.
>
> > +
> > + switch(action) {
> > + case MEM_ONLINE:
> > + if (node_state(nid, N_MEMORY)) {
>
> Hm, I see the issue here, ok, this node_state check isn't needed, as it
> will always be true. So this function needs to handle duplicates still.
Yes, you're right. The `node_state(nid, N_MEMORY)` check I added here is
redundant because it will always be true in this context. I will remove it
to avoid unnecessary duplication.
> vvv
> > + err = sysfs_wi_node_add(nid);
> > + if (err) {
> > + pr_err("failed to add sysfs [node%d]\n", nid);
> > + return NOTIFY_BAD;
> > + }
> > + }
> > + break;
> > + case MEM_OFFLINE:
> > + if (!node_state(nid, N_MEMORY))
>
> This check is good for the time being.
This check looks appropriate for now and I'll keep it as-is.
>
> > + sysfs_wi_node_release(nid);
> > + break;
> > + }
> > +
> > +notifier_end:
> > + return NOTIFY_OK;
> > }
> >
> >
>
> But really I think we probably just want to change to build on top of this:
> https://lore.kernel.org/all/20250401092716.537512-2-osalvador@suse.de/
>
> And use register_node_notifier with NODE_BECAME_MEMORYLESS and NODE_BECAME_MEM_AWARE
>
> ~Gregory
Thank you for sharing the link regarding `node_notify`. I agree that the
mechanism you pointed out would be a better fit for this patch.
By using `register_node_notifier` with `NODE_BECAME_MEMORYLESS` and
`NODE_BECAME_MEM_AWARE`, we can avoid unnecessary callbacks and implement
this functionality more efficiently.
However, I think it would be better to apply the current patch first and
then update it to use `node_notify` once that support is finalized and
available upstream.
Rakie
Powered by blists - more mailing lists