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

["Huang, Ying" <ying.huang@linux.alibaba.com>]

Joshua Hahn <joshua.hahnjy@gmail.com> writes:

> On Thu, 13 Feb 2025 09:32:49 +0800 "Huang, Ying" <ying.huang@linux.alibaba.com> wrote:
>
>> Joshua Hahn <joshua.hahnjy@gmail.com> writes:
>> 
>> > On Wed, 12 Feb 2025 10:49:32 +0800 "Huang, Ying" <ying.huang@linux.alibaba.com> 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?

Because weighted_interleave_auto and iw_table are 2 variables, you may
read new weighted_interleave_auto and old iw_table or vice versa.  If
we put them into one struct and write/read the pointer to the struct
with rcu_assign_pointer() / rcu_dereference(), we can avoid this.

> 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!

Thanks!

---
Best Regards,
Huang, Ying