Re: [PATCH 1/2] block/mq-deadline: serialize request dispatching

[Jens Axboe <axboe@kernel.dk>]

On 1/18/24 11:24 AM, Bart Van Assche wrote:
> On 1/18/24 10:04, Jens Axboe wrote:
>> If we're entering request dispatch but someone else is already
>> dispatching, then just skip this dispatch. We know IO is inflight and
>> this will trigger another dispatch event for any completion. This will
>> potentially cause slightly lower queue depth for contended cases, but
>> those are slowed down anyway and this should not cause an issue.
> 
> Shouldn't a performance optimization patch include numbers that show by
> how much that patch improves the performance of different workloads?

The commit messages may want some editing, but all of that is in the
cover letter that I'm sure you saw. This is just a POC/RFC posting.

>>   struct deadline_data {
>>       /*
>>        * run time data
>>        */
>> +    struct {
>> +        spinlock_t lock;
>> +        spinlock_t zone_lock;
>> +    } ____cacheline_aligned_in_smp;
> 
> Is ____cacheline_aligned_in_smp useful here? struct deadline_data
> is not embedded in any other data structure but is allocated with kzalloc_node().

It's not for alignment of deadline_data, it's for alignment within
deadline_data so that grabbing/releasing these locks don't share a
cacheline with other bits. We could ensure that deadline_data itself
is aligned as well, that might be a good idea.

>> +    /*
>> +     * If someone else is already dispatching, skip this one. This will
>> +     * defer the next dispatch event to when something completes, and could
>> +     * potentially lower the queue depth for contended cases.
>> +     */
>> +    if (test_bit(DD_DISPATCHING, &dd->run_state) ||
>> +        test_and_set_bit(DD_DISPATCHING, &dd->run_state))
>> +        return NULL;
>> +
> 
> The above code behaves similar to spin_trylock(). Has it been
> considered to use spin_trylock() instead?

Do you read the replies to the emails, from the other thread? First of
all, you'd need another lock for this. And secondly, this avoids a RMW
if it's already set. So no, I think this is cleaner than a separate
lock, and you'd still have cacheline bouncing on that for ALL calls if
you did it that way.

-- 
Jens Axboe