Date:   Wed, 13 Jan 2021 12:02:44 +0000
From:   Damien Le Moal <Damien.LeMoal@....com>
To:     Ming Lei <ming.lei@...hat.com>
CC:     Ming Lei <tom.leiming@...il.com>,
        Changheun Lee <nanich.lee@...sung.com>,
        Johannes Thumshirn <Johannes.Thumshirn@....com>,
        Jens Axboe <axboe@...nel.dk>,
        "jisoo2146.oh@...sung.com" <jisoo2146.oh@...sung.com>,
        "junho89.kim@...sung.com" <junho89.kim@...sung.com>,
        linux-block <linux-block@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "mj0123.lee@...sung.com" <mj0123.lee@...sung.com>,
        "seunghwan.hyun@...sung.com" <seunghwan.hyun@...sung.com>,
        "sookwan7.kim@...sung.com" <sookwan7.kim@...sung.com>,
        Tejun Heo <tj@...nel.org>,
        "yt0928.kim@...sung.com" <yt0928.kim@...sung.com>,
        "woosung2.lee@...sung.com" <woosung2.lee@...sung.com>
Subject: Re: [PATCH] bio: limit bio max size.

On 2021/01/13 20:48, Ming Lei wrote:
> On Wed, Jan 13, 2021 at 11:16:11AM +0000, Damien Le Moal wrote:
>> On 2021/01/13 19:25, Ming Lei wrote:
>>> On Wed, Jan 13, 2021 at 09:28:02AM +0000, Damien Le Moal wrote:
>>>> On 2021/01/13 18:19, Ming Lei wrote:
>>>>> On Wed, Jan 13, 2021 at 12:09 PM Changheun Lee <nanich.lee@...sung.com> wrote:
>>>>>>
>>>>>>> On 2021/01/12 21:14, Changheun Lee wrote:
>>>>>>>>> On 2021/01/12 17:52, Changheun Lee wrote:
>>>>>>>>>> From: "Changheun Lee" <nanich.lee@...sung.com>
>>>>>>>>>>
>>>>>>>>>> bio size can grow up to 4GB when muli-page bvec is enabled.
>>>>>>>>>> but sometimes it would lead to inefficient behaviors.
>>>>>>>>>> in case of large chunk direct I/O, - 64MB chunk read in user space -
>>>>>>>>>> all pages for 64MB would be merged to a bio structure if memory address is
>>>>>>>>>> continued phsycally. it makes some delay to submit until merge complete.
>>>>>>>>>> bio max size should be limited as a proper size.
>>>>>>>>>
>>>>>>>>> But merging physically contiguous pages into the same bvec + later automatic bio
>>>>>>>>> split on submit should give you better throughput for large IOs compared to
>>>>>>>>> having to issue a bio chain of smaller BIOs that are arbitrarily sized and will
>>>>>>>>> likely need splitting anyway (because of DMA boundaries etc).
>>>>>>>>>
>>>>>>>>> Do you have a specific case where you see higher performance with this patch
>>>>>>>>> applied ? On Intel, BIO_MAX_SIZE would be 1MB... That is arbitrary and too small
>>>>>>>>> considering that many hardware can execute larger IOs than that.
>>>>>>>>>
>>>>>>>>
>>>>>>>> When I tested 32MB chunk read with O_DIRECT in android, all pages of 32MB
>>>>>>>> is merged into a bio structure.
>>>>>>>> And elapsed time to merge complete was about 2ms.
>>>>>>>> It means first bio-submit is after 2ms.
>>>>>>>> If bio size is limited with 1MB with this patch, first bio-submit is about
>>>>>>>> 100us by bio_full operation.
>>>>>>>
>>>>>>> bio_submit() will split the large BIO case into multiple requests while the
>>>>>>> small BIO case will likely result one or two requests only. That likely explain
>>>>>>> the time difference here. However, for the large case, the 2ms will issue ALL
>>>>>>> requests needed for processing the entire 32MB user IO while the 1MB bio case
>>>>>>> will need 32 different bio_submit() calls. So what is the actual total latency
>>>>>>> difference for the entire 32MB user IO ? That is I think what needs to be
>>>>>>> compared here.
>>>>>>>
>>>>>>> Also, what is your device max_sectors_kb and max queue depth ?
>>>>>>>
>>>>>>
>>>>>> 32MB total latency is about 19ms including merge time without this patch.
>>>>>> But with this patch, total latency is about 17ms including merge time too.
>>>>>
>>>>> 19ms looks too big just for preparing one 32MB sized bio, which isn't
>>>>> supposed to
>>>>> take so long.  Can you investigate where the 19ms is taken just for
>>>>> preparing one
>>>>> 32MB sized bio?
>>>>
>>>> Changheun mentioned that the device side IO latency is 16.7ms out of the 19ms
>>>> total. So the BIO handling, submission+completion takes about 2.3ms, and
>>>> Changheun points above to 2ms for the submission part.
>>>
>>> OK, looks I misunderstood the data.
>>>
>>>>
>>>>>
>>>>> It might be iov_iter_get_pages() for handling page fault. If yes, one suggestion
>>>>> is to enable THP(Transparent HugePage Support) in your application.
>>>>
>>>> But if that was due to page faults, the same large-ish time would be taken for
>>>> the preparing the size-limited BIOs too, no ? No matter how the BIOs are diced,
>>>> all 32MB of pages of the user IO are referenced...
>>>
>>> If bio size is reduced to 1MB, just 256 pages need to be faulted before submitting this
>>> bio, instead of 256*32 pages, that is why the following words are mentioned:
>>>
>>> 	It means first bio-submit is after 2ms.
>>> 	If bio size is limited with 1MB with this patch, first bio-submit is about
>>> 	100us by bio_full operation.
>>
>> Yes, but eventually, all pages for the 32MB IO will be faulted in, just not in
>> one go. Overall number of page faults is likely the same as with the large BIO
>> preparation. So I think we are back to my previous point, that is, reducing the
>> device idle time by starting a BIO more quickly, even a small one, leads to
>> overlap between CPU time needed for the next BIO preparation and previous BIO
>> execution, reducing overall the latency for the entire 32MB user IO.
> 
> When bio size is reduced from 32M to 1M:
> 
> 1MB/(P(1M) + D(1M)) may become bigger than 32MB/(P(1M) + D(1M)), so
> throughput is improved.

I think that the reason is that P(1M) < D(1M) and so there is overlap between P
and D: P of the next BIO is done on the CPU while D of the previous BIO is
ongoing on the device, assuming there is no plugging.

Somewhat the same should happen for the 32MB case too since that would still
lead to a D(329 generating 32 x P(1). And again assuming that there is no plug,
the first 1M BIO generated by the BIO split in D(32) should start executing
right away while D(329 split continues on the CPU. I guess the fragments are not
issued (a plug or an IO sched holding them ?) so the overall latency becomes
D(32) + 32 x P(1). Leading to the 2ms D(32) difference in latency that Changheun
observed. A blktrace could confirm that I guess.

> 
> P(x) means time for preparing 'x' sized IO
> D(x) means time for device to handle 'x' sized IO >
> I depend on both CPU and the UFS drive.
> 
>> I don't think that the reason is page faulting in itself.
> 
> What I meant is that page faulting might contribute most part of the
> 100us(preparing 1MB data) and 2ms(preparing 32MB data). It can be others,
> but should be easy to figure out.

I may be wrong here (need to check again), but I though the pages were faulted
in at get_user_pages() time, that is, for the 32MB user buffer, regardless of
the BIO size limit. If that's the case, then page fault should not be a
significant factor in the latency difference.

-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists