On 1/24/24 2:31 AM, Christoph Hellwig wrote:
> On Tue, Jan 23, 2024 at 12:13:29PM -0700, Jens Axboe wrote:
>> On 1/23/24 11:36 AM, Bart Van Assche wrote:
>>> On 1/23/24 09:34, Jens Axboe wrote:
>>>> + struct {
>>>> + spinlock_t lock;
>>>> + spinlock_t zone_lock;
>>>> + } ____cacheline_aligned_in_smp;
>>>
>>> It is not clear to me why the ____cacheline_aligned_in_smp attribute
>>> is applied to the two spinlocks combined? Can this cause both spinlocks
>>> to end up in the same cache line? If the ____cacheline_aligned_in_smp
>>> attribute would be applied to each spinlock separately, could that
>>> improve performance even further? Otherwise this patch looks good to me,
>>> hence:
>>
>> It is somewhat counterintuitive, but my testing shows that there's no
>> problem with them in the same cacheline. Hence I'm reluctant to move
>> them out of the struct and align both of them, as it'd just waste memory
>> for seemingly no runtime benefit.
>
> Is there ay benefit in aligning either of them? The whole cache line
> align locks thing seemed to have been very popular 20 years ago,
> and these days it tends to not make much of a difference.
It's about 1% for me, so does make a difference. Probably because it
shares with run_state otherwise. And I'll have to violently disagree
that aligning frequently used locks outside of other modified or
mostly-read regions was a fad from 20 years ago, it still very much
makes sense. It may be overdone, like any "trick" like that, but it's
definitely not useless.
--
Jens Axboe
