On Mon, Sep 08 2025 at 09:26, Daniel Wagner wrote:
> On Mon, Sep 08, 2025 at 08:13:31AM +0200, Hannes Reinecke wrote:
>> > const struct cpumask *blk_mq_online_queue_affinity(void)
>> > {
>> > + if (housekeeping_enabled(HK_TYPE_IO_QUEUE)) {
>> > + cpumask_and(&blk_hk_online_mask, cpu_online_mask,
>> > + housekeeping_cpumask(HK_TYPE_IO_QUEUE));
>> > + return &blk_hk_online_mask;
>>
>> Can you explain the use of 'blk_hk_online_mask'?
>> Why is a static variable?
>
> The blk_mq_*_queue_affinity helpers return a const struct cpumask *, the
> caller doesn't need to free the return value. Because cpumask_and needs
> store its result somewhere, I opted for the global static variable.
>
>> To my untrained eye it's being recalculated every time one calls
>> this function. And only the first invocation run on an empty mask,
>> all subsequent ones see a populated mask.
>
> The cpu_online_mask might change over time, it's not a static bitmap.
> Thus it's necessary to update the blk_hk_online_mask. Doing some sort of
> caching is certainly possible. Given that we have plenty of cpumask
> logic operation in the cpu_group_evenly code path later, I am not so
> sure this really makes a huge difference.
Sure, but none of this is serialized against CPU hotplug operations. So
the resulting mask, which is handed into the spreading code can be
concurrently modified. IOW it's not as const as the code claims.
How is this even remotely correct?
Thanks,
tglx
