On Sat, 10 May 2025 13:25:32 +0800 "Huang, Ying" <ying.huang@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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
