Re: [PATCH v7 3/3] mm/mempolicy: Support memory hotplug in weighted interleave

[Rakie Kim]

On Wed, 16 Apr 2025 13:04:32 +0900 Honggyu Kim <honggyu.kim@xxxxxx> wrote:

Hi Jonathan and Honggyu,

Thank you for reviewing this patch and for offering valuable ideas to
address the issues. I have accepted all of your suggestions and am
currently preparing a new patch series, version v8.

> Hi Jonathan,
> 
> Thanks for reviewing our patches.
> 
> I have a few comments and the rest will be addressed by Rakie.
> 
> On 4/16/2025 1:00 AM, Jonathan Cameron wrote:
> > On Tue, 8 Apr 2025 16:32:42 +0900
> > Rakie Kim <rakie.kim@xxxxxx> wrote:
> > 
> >> @@ -3470,13 +3472,24 @@ static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> >>   
> >>   static void sysfs_wi_node_delete(int nid)
> >>   {
> >> -	if (!wi_group->nattrs[nid])
> >> +	struct iw_node_attr *attr;
> >> +
> >> +	if (nid < 0 || nid >= nr_node_ids)
> >> +		return;
> >> +
> >> +	mutex_lock(&wi_group->kobj_lock);
> >> +	attr = wi_group->nattrs[nid];
> >> +	if (!attr) {
> >> +		mutex_unlock(&wi_group->kobj_lock);
> >>   		return;
> >> +	}
> >> +
> >> +	wi_group->nattrs[nid] = NULL;
> >> +	mutex_unlock(&wi_group->kobj_lock);
> >>   
> >> -	sysfs_remove_file(&wi_group->wi_kobj,
> >> -			  &wi_group->nattrs[nid]->kobj_attr.attr);
> >> -	kfree(wi_group->nattrs[nid]->kobj_attr.attr.name);
> >> -	kfree(wi_group->nattrs[nid]);
> >> +	sysfs_remove_file(&wi_group->wi_kobj, &attr->kobj_attr.attr);
> >> +	kfree(attr->kobj_attr.attr.name);
> >> +	kfree(attr);
> > Here you go through a careful dance to not touch wi_group->nattrs[nid]
> > except under the lock, but later you are happy to do so in the
> > error handling paths.  Maybe better to do similar to here and
> > set it to NULL under the lock but do the freeing on a copy taken
> > under that lock.

I have updated the error handling path in sysfs_wi_node_add() as you
suggested.

> > .
> >>   }
> >>   
> >>   static void sysfs_wi_release(struct kobject *wi_kobj)
> >> @@ -3495,35 +3508,77 @@ static const struct kobj_type wi_ktype = {
> >>   
> >>   static int sysfs_wi_node_add(int nid)
> >>   {
> >> -	struct iw_node_attr *node_attr;
> >> +	int ret = 0;
> > 
> > Trivial but isn't ret always set when it is used? So no need to initialize
> > here.

In the updated code for v8, I retained the initialization of `ret = 0`
because it is required for proper cleanup handling in the current
version.

> 
> If we don't initialize it, then this kind of trivial fixup might be needed later
> so I think there is no reason not to initialize it.
> https://lore.kernel.org/mm-commits/20240705010631.46743C4AF07@xxxxxxxxxxxxxxx
> 
> > 
> >>   	char *name;
> >> +	struct iw_node_attr *new_attr = NULL;
> > 
> > This is also always set before use so I'm not seeing a
> > reason to initialize it to NULL.
> 
> Ditto.

I also removed the unnecessary `= NULL` initializer for `new_attr`,
as it is always assigned before use.

> 
> > 
> > 
> >>   
> >> -	node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> >> -	if (!node_attr)
> >> +	if (nid < 0 || nid >= nr_node_ids) {
> >> +		pr_err("Invalid node id: %d\n", nid);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	new_attr = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
> > 
> > I'd prefer sizeof(*new_attr) because I'm lazy and don't like checking
> > types for allocation sizes :)  Local style seems to be a bit
> > of a mix though.
> 
> Agreed.

As you recommended, I changed the allocation from
`sizeof(struct iw_node_attr)` to `sizeof(*new_attr)` for better 
readability and consistency.

> 
> > 
> >> +	if (!new_attr)
> >>   		return -ENOMEM;
> >>   
> >>   	name = kasprintf(GFP_KERNEL, "node%d", nid);
> >>   	if (!name) {
> >> -		kfree(node_attr);
> >> +		kfree(new_attr);
> >>   		return -ENOMEM;
> >>   	}
> >>   
> >> -	sysfs_attr_init(&node_attr->kobj_attr.attr);
> >> -	node_attr->kobj_attr.attr.name = name;
> >> -	node_attr->kobj_attr.attr.mode = 0644;
> >> -	node_attr->kobj_attr.show = node_show;
> >> -	node_attr->kobj_attr.store = node_store;
> >> -	node_attr->nid = nid;
> >> +	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;
> >> +	}
> >> +	wi_group->nattrs[nid] = new_attr;
> 
> This set can be done after all the "wi_group->nattrs[nid]" related set is done.
> 
> >>   
> >> -	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;
> >> +	sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
> > 
> > I'd have been tempted to use the new_attr pointer but perhaps
> > this brings some documentation like advantages.
> 
> +1

Additionally, I replaced all usage of `wi_group->nattrs[nid]` in
sysfs_wi_node_add() with the `new_attr` pointer to simplify the logic
and improve clarity. This also aligns with your suggestion to treat
`new_attr` consistently throughout the function.

> 
> > 
> >> +	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;
> 
> As Jonathan mentioned, all the "wi_group->nattrs[nid]" here is better to be
> "new_attr" for simplicity.
> 
> Thanks,
> Honggyu
> 
> >> +
> >> +	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);
> > 
> > See comment above on the rather different handling here to in
> > sysfs_wi_node_delete() where you set it to NULL first, release the lock and tidy up.
> > new_attrand name are still set so you could even combine the handling with the
> > if (wi_group->nattrs[nid]) above via appropriate gotos.

I agree with your observation regarding the difference in error
handling between sysfs_wi_node_add() and sysfs_wi_node_delete(), so I
refactored sysfs_wi_node_add() to follow the same structure.

I will apply all of these updates in the new v8 series. Thank you
again for your thoughtful and detailed feedback.
Below is the revised code after incorporating your feedback.

Rakie

@@ -3532,14 +3532,14 @@ static int sysfs_wi_node_add(int nid)
 {
        int ret = 0;
        char *name;
-       struct iw_node_attr *new_attr = NULL;
+       struct iw_node_attr *new_attr;
 
        if (nid < 0 || nid >= nr_node_ids) {
-               pr_err("Invalid node id: %d\n", nid);
+               pr_err("invalid node id: %d\n", nid);
                return -EINVAL;
        }
 
-       new_attr = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
+       new_attr = kzalloc(sizeof(*new_attr), GFP_KERNEL);
        if (!new_attr)
                return -ENOMEM;
 
@@ -3549,33 +3549,32 @@ static int sysfs_wi_node_add(int nid)
                return -ENOMEM;
        }
 
+       sysfs_attr_init(&new_attr->kobj_attr.attr);
+       new_attr->kobj_attr.attr.name = name;
+       new_attr->kobj_attr.attr.mode = 0644;
+       new_attr->kobj_attr.show = node_show;
+       new_attr->kobj_attr.store = node_store;
+       new_attr->nid = nid;
+
        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;
+               pr_info("node%d already exists\n", nid);
+               goto out;
        }
-       wi_group->nattrs[nid] = new_attr;
-
-       sysfs_attr_init(&wi_group->nattrs[nid]->kobj_attr.attr);
-               pr_info("Node [%d] already exists\n", nid);
-               kfree(new_attr);
-               kfree(name);
-               return 0;
+               pr_info("node%d already exists\n", nid);
+               goto out;
        }
-       wi_group->nattrs[nid] = new_attr;
-
-       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);
+       ret = sysfs_create_file(&wi_group->wi_kobj, &new_attr->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);
+               mutex_unlock(&wi_group->kobj_lock);
+               goto out;
        }
+       wi_group->nattrs[nid] = new_attr;
        mutex_unlock(&wi_group->kobj_lock);
+       return 0;
 
+out:
+       kfree(new_attr->kobj_attr.attr.name);
+       kfree(new_attr);
        return ret;
}

> > 
> >> +		kfree(wi_group->nattrs[nid]);
> >> +		wi_group->nattrs[nid] = NULL;
> >> +		pr_err("Failed to add attribute to weighted_interleave: %d\n", ret);
> >>   	}
> >> +	mutex_unlock(&wi_group->kobj_lock);
> >>   
> >> -	wi_group->nattrs[nid] = node_attr;
> >> -	return 0;
> >> +	return ret;
> >> +}
> > 
> >