Date:   Wed, 13 Jan 2021 19:47:22 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Damien Le Moal <Damien.LeMoal@....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 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.

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.

-- 
Ming

Powered by blists - more mailing lists