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....@samsung.com> > >>> wrote: > >>>> > >>>>> On 2021/01/12 21:14, Changheun Lee wrote: > >>>>>>> On 2021/01/12 17:52, Changheun Lee wrote: > >>>>>>>> From: "Changheun Lee" <nanich....@samsung.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
