On Thu, 13 Feb 2025 09:32:49 +0800 "Huang, Ying" <ying.huang@xxxxxxxxxxxxxxxxx> wrote:
> Joshua Hahn <joshua.hahnjy@xxxxxxxxx> writes:
>
> > On Wed, 12 Feb 2025 10:49:32 +0800 "Huang, Ying" <ying.huang@xxxxxxxxxxxxxxxxx> wrote:
> >
> >> Hi, Joshua,
> >>
[...snip...]
> >> > + weighted_interleave_auto = false;
> >> > + return count;
> >> > + } else if (!sysfs_streq(buf, "Y") && !sysfs_streq(buf, "1")) {
> >> > + return -EINVAL;
> >> > + }
> >> > +
> >> > + new_iw = kcalloc(nr_node_ids, sizeof(u8), GFP_KERNEL);
> >> > + if (!new_iw)
> >> > + return -ENOMEM;
> >> > +
> >> > + mutex_lock(&iw_table_lock);
> >> > + bw = node_bw_table;
> >> > +
> >> > + if (!bw) {
> >> > + mutex_unlock(&iw_table_lock);
> >> > + kfree(new_iw);
> >> > + return -ENODEV;
> >> > + }
> >> > +
> >> > + old_iw = rcu_dereference_protected(iw_table,
> >> > + lockdep_is_held(&iw_table_lock));
> >> > +
> >> > + reduce_interleave_weights(bw, new_iw);
> >> > + rcu_assign_pointer(iw_table, new_iw);
> >> > + mutex_unlock(&iw_table_lock);
> >> > +
> >> > + synchronize_rcu();
> >> > + kfree(old_iw);
> >> > +
> >> > + weighted_interleave_auto = true;
> >>
> >> Why assign weighted_interleave_auto after synchronize_rcu()? To reduce
> >> the race window, it's better to change weighted_interleave_auto and
> >> iw_table together? Is it better to put them into a data structure and
> >> change them together always?
> >>
> >> struct weighted_interleave_state {
> >> bool weighted_interleave_auto;
> >> u8 iw_table[0]
> >> };
> >
> > I see, I think your explanation makes sense. For the first question,
> > I think your point makes sense, so I will move the updating to be
> > inside the rcu section.
> >
> > As for the combined data structure, I think that this makes sense,
> > but I have a few thoughts. First, there are some times when we don't
> > update both of them, like moving from auto --> manual, and whenever
> > we just update iw_table, we don't need to update the weighted_interleave
> > auto field. I also have a concern that this might make the code a bit
> > harder to read, but that is just my humble opinion.
>
> I think the overhead is relatively small. With that, we can avoid the
> inconsistency between weighted_interleave_auto and iw_table[].
> struct_size() or struct_size_t() family helpers can be used to manage
> the flexible array at the end of the struct.
That sounds good to me. I don't have any strong opinions about this
change, so I am happy to combine them into a struct. I just want to
make sure I am understanding your perspective correctly: what is the
incosistency between weighted_interleave_auto and iw_table[]?
If I move the weighted_interleave_auto = true statement inside the
rcu section, will the inconsistency still be there?
Just want to make sure so that I am not missing anything important!
Thank you again for your great feedback. I hope you have a happy Friday!
Joshua
> ---
> Best Regards,
> Huang, Ying
Sent using hkml (https://github.com/sjp38/hackermail)
