Re: [PATCH v8] mm/mempolicy: Weighted Interleave Auto-tuning

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Sat, 10 May 2025 13:25:32 +0800 "Huang, Ying" <ying.huang@linux.alibaba.com> wrote:

Hi Ying,
Thank you for reviewing my patch, as always!

> Hi, Joshua,
> 
> Thank you for updated version!  And sorry for late reply.
> 
> Joshua Hahn <joshua.hahnjy@gmail.com> writes:

[...snip...]

> > diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > index 0b7972de04e9..ec13382c606f 100644
> > --- a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > @@ -20,6 +20,35 @@ Description:	Weight configuration interface for nodeN
> >  		Minimum weight: 1
> >  		Maximum weight: 255
> >  
> > -		Writing an empty string or `0` will reset the weight to the
> > -		system default. The system default may be set by the kernel
> > -		or drivers at boot or during hotplug events.
> > +		Writing invalid values (i.e. any values not in [1,255],
> > +		empty string, ...) will return -EINVAL.
> > +
> > +		Changing the weight to a valid value will automatically
> > +		update the system to manual mode as well.
> 
> s/update/switch/ ?
> 
> But my English is poor, please keep your version if you think that it's
> better.

I have no particular preference here, whatever will make it easiest for the
users to understand what is happening. I'll take your suggestion!

[...snip...]

> > +/*
> > + * A null weighted_interleave_state is interpted as having .mode = "auto",
> > + * and .iw_table is interpreted as an array of 1s with length nr_node_ids.
> > + */
> 
> Better to move the comments to above "wi_state" definition?
> 
> > +struct weighted_interleave_state {
> > +	bool mode_auto;
> > +	u8 iw_table[];
> > +};
> > +
> 
> Why do you put the type definition in mempolicy.h instead of
> mempolicy.c?  I don't find other users except mempolicy.c.

Good point, I'll move the definition to mempolicy.c and move the comment
to the wi_state definition as well.

[...snip...]

> > @@ -3450,31 +3555,104 @@ static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
> >  static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
> >  			  const char *buf, size_t count)
> >  {
> > +	struct weighted_interleave_state *new_wi_state, *old_wi_state = NULL;
> >  	struct iw_node_attr *node_attr;
> > -	u8 *new;
> > -	u8 *old;
> >  	u8 weight = 0;
> > +	int i;
> >  
> >  	node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
> >  	if (count == 0 || sysfs_streq(buf, ""))
> >  		weight = 0;
> 
> According to revised ABI, we should return -EINVAL here?

Great catch, I completely ignored the ABI description that I wrote...
I'll go ahead and just return -EINVAL here!

[...snip...]

> > +static ssize_t weighted_interleave_auto_store(struct kobject *kobj,
> > +		struct kobj_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct weighted_interleave_state *new_wi_state, *old_wi_state = NULL;
> > +	unsigned int *bw;
> > +	bool input;
> > +	int i;
> > +
> > +	if (kstrtobool(buf, &input))
> > +		return -EINVAL;
> > +
> > +	new_wi_state = kzalloc(struct_size(new_wi_state, iw_table, nr_node_ids),
> > +			       GFP_KERNEL);
> > +	if (!new_wi_state)
> > +		return -ENOMEM;
> > +	for (i = 0; i < nr_node_ids; i++)
> > +		new_wi_state->iw_table[i] = 1;
> > +
> > +	mutex_lock(&wi_state_lock);
> > +	if (!input) {
> 
> If input == old_wi_state->mode_auto, we can return directly?

Yes, that makes sense to me.

> >  static void wi_cleanup(void) {
> > +	sysfs_remove_file(&wi_group->wi_kobj, &wi_group->auto_kobj_attr.attr);
> 
> Why not just
> 
> 	sysfs_remove_file(&wi_group->wi_kobj, &wi_auto_attr.attr);
> 
> ?

Also makes sense!!

> ---
> Best Regards,
> Huang, Ying

Thank you for your great feedback Ying, I'll make changes based on
your suggestions and shortly send up a v9. I hope you have a great day!
Joshua