linux-kernel - Re: [PATCH] raid10: avoid spin_lock from fastpath from raid10

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 19 Jun 2023 21:00:00 +0800
From:   Yu Kuai <yukuai1@...weicloud.com>
To:     Paul Menzel <pmenzel@...gen.mpg.de>,
        Yu Kuai <yukuai1@...weicloud.com>
Cc:     aligrudi@...il.com, song@...nel.org, linux-raid@...r.kernel.org,
        linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
        yangerkun@...wei.com, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH] raid10: avoid spin_lock from fastpath from
 raid10_unplug()

Hi,

在 2023/06/19 18:26, Paul Menzel 写道:
> Dear Yu,
> 
> 
> Thank you for your patch. Some minor nits from my side, you can also 
> ignore.
> 
> Am 18.06.23 um 16:25 schrieb Yu Kuai:
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> Commit 0c0be98bbe67 ("md/raid10: prevent unnecessary calls to wake_up()
>> in fast path") missed one place, for example, while testing with:
> 
> … one place. For example, with
> 
>>
>> fio -direct=1 -rw=write/randwrite -iodepth=1 ...
>>
>> Then plug and unplug will be called for each io, then wake_up() from
> 
> Maybe:
> 
>      fio -direct=1 -rw=write/randwrite -iodepth=1 ...
> 
> plug und unplug are called for each io, then …
> 
>> raid10_unplug() will cause lock contention as well.
> 
> Maybe paste the perf command and output?

Related test and test result and perf result can be found in the below
Link.

By the way, I'll wait for more review to send a v2 to optmize commit
message.

Thanks,
Kuai

> 
>> Avoid this contention by using wake_up_barrier() instead of wake_up(),
>> where spin_lock is not held while waitqueue is empty.
> 
> It’d be great if you added also the test results to the commit message.
> 
>> By the way, in this scenario, each blk_plug_cb() will be allocated and
>> freed for each io, this seems need to be optimized as well.
>>
>> Reported-and-tested-by: Ali Gholami Rudi <aligrudi@...il.com>
>> Link: https://lore.kernel.org/all/20231606122233@laper.mirepesht/
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>> ---
>>   drivers/md/raid10.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index d0de8c9fb3cf..fbaaa5e05edc 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1118,7 +1118,7 @@ static void raid10_unplug(struct blk_plug_cb 
>> *cb, bool from_schedule)
>>           spin_lock_irq(&conf->device_lock);
>>           bio_list_merge(&conf->pending_bio_list, &plug->pending);
>>           spin_unlock_irq(&conf->device_lock);
>> -        wake_up(&conf->wait_barrier);
>> +        wake_up_barrier(conf);
>>           md_wakeup_thread(mddev->thread);
>>           kfree(plug);
>>           return;
>> @@ -1127,7 +1127,7 @@ static void raid10_unplug(struct blk_plug_cb 
>> *cb, bool from_schedule)
>>       /* we aren't scheduling, so we can do the write-out directly. */
>>       bio = bio_list_get(&plug->pending);
>>       raid1_prepare_flush_writes(mddev->bitmap);
>> -    wake_up(&conf->wait_barrier);
>> +    wake_up_barrier(conf);
>>       while (bio) { /* submit pending writes */
>>           struct bio *next = bio->bi_next;
> 
> Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul
> 
> .
>