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

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

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,
>> 
>> Thanks for your patch and sorry for late reply.
>
> Hi Ying, no worries! Thank you for taking time to review this patch.
>
>> Joshua Hahn <joshua.hahnjy@gmail.com> writes:

[snip]

>> > +
>> > +static ssize_t weighted_interleave_mode_store(struct kobject *kobj,
>> > +		struct kobj_attribute *attr, const char *buf, size_t count)
>> > +{
>> > +	uint64_t *bw;
>> > +	u8 *old_iw, *new_iw;
>> > +
>> > +	if (count == 0)
>> > +		return -EINVAL;
>> > +
>> > +	if (sysfs_streq(buf, "N") || sysfs_streq(buf, "0")) {
>> 
>> kstrtobool() can be used here.  It can deal with 'count == 0' case too.
>
> These kernel string tools are very helpful, thank you for bringing
> them to my attention : -)
>
>> > +		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.

---
Best Regards,
Huang, Ying