| Message ID | 20220726225232.1362251-1-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] scsi: ufs: Increase the maximum data buffer size | expand |
Bart, > Measurements for one particular UFS controller + UFS device show a 25% > higher read bandwidth if the maximum data buffer size is increased from > 512 KiB to 1 MiB. Hence increase the maximum size of the data buffer > associated with a single request from SCSI_DEFAULT_MAX_SECTORS (1024) > * 512 bytes = 512 KiB to 1 MiB. Applied to 5.20/scsi-staging, thanks!
Hi bart Is it possible by adding only max_sector to increase the data buffer size? I think the data buffer will split to 512 KiB, because the sg_table size is SG_ALL Thanks, yohan > -----Original Message----- > From: Bart Van Assche <bvanassche@acm.org> > Sent: Wednesday, July 27, 2022 7:52 AM > To: Martin K . Petersen <martin.petersen@oracle.com> > Cc: Jaegeuk Kim <jaegeuk@kernel.org>; linux-scsi@vger.kernel.org; Adrian > Hunter <adrian.hunter@intel.com>; Bart Van Assche <bvanassche@acm.org>; > Avri Altman <avri.altman@wdc.com>; Bean Huo <beanhuo@micron.com>; Stanley > Chu <stanley.chu@mediatek.com>; James E.J. Bottomley <jejb@linux.ibm.com>; > Matthias Brugger <matthias.bgg@gmail.com> > Subject: [PATCH v3] scsi: ufs: Increase the maximum data buffer size > > Measurements for one particular UFS controller + UFS device show a 25% > higher read bandwidth if the maximum data buffer size is increased from > 512 KiB to 1 MiB. Hence increase the maximum size of the data buffer > associated with a single request from SCSI_DEFAULT_MAX_SECTORS (1024) > * 512 bytes = 512 KiB to 1 MiB. > > Notes: > - The maximum data buffer size supported by the UFSHCI specification > is 65535 * 256 KiB or about 16 GiB. > - The maximum data buffer size for READ(10) commands is 65535 logical > blocks. To transfer more than 65535 * 4096 bytes = 255 MiB with a > single SCSI command, the READ(16) command is required. Support for > READ(16) is optional in the UFS 3.1 and UFS 4.0 standards. > > Cc: Adrian Hunter <adrian.hunter@intel.com> > Cc: Avri Altman <avri.altman@wdc.com> > Cc: Bean Huo <beanhuo@micron.com> > Cc: Stanley Chu <stanley.chu@mediatek.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > Changes compared to v2: changed maximum transfer size 255 MiB to 1 MiB. > Changes compared to v1: changed maximum transfer size from 1 GiB to 255 MiB. > > drivers/ufs/core/ufshcd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 36b7212e9cb5..678bc8d6d6aa 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8365,6 +8365,7 @@ static struct scsi_host_template > ufshcd_driver_template = { > .cmd_per_lun = UFSHCD_CMD_PER_LUN, > .can_queue = UFSHCD_CAN_QUEUE, > .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, > + .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB */ > .max_host_blocked = 1, > .track_queue_depth = 1, > .sdev_groups = ufshcd_driver_groups,
On 8/2/22 16:40, yohan.joung@sk.com wrote: > Is it possible by adding only max_sector to increase the data buffer size? Yes. > I think the data buffer will split to 512 KiB, because the sg_table size is SG_ALL I don't think so. With this patch applied, the limits supported by the UFS driver are as follows: .sg_tablesize = SG_ALL, /* 128 */ .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, /* 256 KiB*/ .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB */ So the maximum data buffer size is min(max_sectors * 512, sg_tablesize * max_segment_size) = min(1 MiB, 128 * 256 KiB) = 1 MiB. On a system with 4 KiB pages, the data buffer size will be 128 * 4 KiB = 512 MiB if none of the pages involved in the I/O are contiguous. Bart.
> On 8/2/22 16:40, yohan.joung@sk.com wrote: > > Is it possible by adding only max_sector to increase the data buffer size? > > Yes. > > > I think the data buffer will split to 512 KiB, because the sg_table > > size is SG_ALL > > I don't think so. With this patch applied, the limits supported by the UFS > driver are as follows: > > .sg_tablesize = SG_ALL, /* 128 */ > .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, /* 256 KiB*/ > .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB */ > > So the maximum data buffer size is min(max_sectors * 512, sg_tablesize * > max_segment_size) = min(1 MiB, 128 * 256 KiB) = 1 MiB. On a system with > 4 KiB pages, the data buffer size will be 128 * 4 KiB = 512 MiB if none of > the pages involved in the I/O are contiguous. In block layer, max_segment_size is obtained from get_max_segment_size. seg_boundary_mask is set to PAGE_SIZE - 1 in the ufs driver. The segment size is the PAGE size, and the max buffer size is segment size * max segment count ( PAGE SIZE * 128 ) = 512 KiB in block layer Right? > > Bart.
On 8/3/22 18:50, yohan.joung@sk.com wrote: > In block layer, max_segment_size is obtained from get_max_segment_size. > seg_boundary_mask is set to PAGE_SIZE - 1 in the ufs driver. > The segment size is the PAGE size, and the max buffer size is > segment size * max segment count ( PAGE SIZE * 128 ) = 512 KiB in block layer > Right? Thanks for having reported this. I had overlooked that the UFS host controller driver sets the dma_boundary member of the host_template field. Is my understanding correct that UFS host controllers should support DMA segments that consist of multiple physical pages? Thanks, Bart.
> > In block layer, max_segment_size is obtained from get_max_segment_size. > > seg_boundary_mask is set to PAGE_SIZE - 1 in the ufs driver. > > The segment size is the PAGE size, and the max buffer size is segment > > size * max segment count ( PAGE SIZE * 128 ) = 512 KiB in block layer > > Right? > > Thanks for having reported this. I had overlooked that the UFS host > controller driver sets the dma_boundary member of the host_template field. > Is my understanding correct that UFS host controllers should support DMA > segments that consist of multiple physical pages? max value of data byte count is 256kb in PRDT. 256kb (data byte count) * 128 (max segments) = 32768kb If physical pages are continuous, it seems that IO can be delivered up to 32mb. performance checks are required, but we don't have to consider saturation. because the device vendor can set opt_xfer_blocks . Thanks, yohan > > Thanks, > > Bart.
On Wed, 2022-08-03 at 09:23 -0700, Bart Van Assche wrote: > On 8/2/22 16:40, yohan.joung@sk.com wrote: > > Is it possible by adding only max_sector to increase the data > > buffer size? > > Yes. > > > I think the data buffer will split to 512 KiB, because the sg_table > > size is SG_ALL > > I don't think so. With this patch applied, the limits supported by > the > UFS driver are as follows: > > .sg_tablesize = SG_ALL, /* 128 */ > .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, /* 256 > KiB*/ > .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB > */ > > So the maximum data buffer size is min(max_sectors * 512, > sg_tablesize * > max_segment_size) = min(1 MiB, 128 * 256 KiB) = 1 MiB. On a system > with > 4 KiB pages, the data buffer size will be 128 * 4 KiB = 512 MiB if > none > of the pages involved in the I/O are contiguous. > > Bart. Bart, This change just increases the shost->max_sectors limit from 501KB to 1Mb, but the final value will be overridden by the optimal transfer length defined in the VPD, right? Kind regards, Bean
On 9/2/22 07:52, Bean Huo wrote: > On Wed, 2022-08-03 at 09:23 -0700, Bart Van Assche wrote: >> On 8/2/22 16:40, yohan.joung@sk.com wrote: >>> Is it possible by adding only max_sector to increase the data >>> buffer size? >> >> Yes. >> >>> I think the data buffer will split to 512 KiB, because the sg_table >>> size is SG_ALL >> >> I don't think so. With this patch applied, the limits supported by >> the >> UFS driver are as follows: >> >> .sg_tablesize = SG_ALL, /* 128 */ >> .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, /* 256 >> KiB*/ >> .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB >> */ >> >> So the maximum data buffer size is min(max_sectors * 512, >> sg_tablesize * >> max_segment_size) = min(1 MiB, 128 * 256 KiB) = 1 MiB. On a system >> with >> 4 KiB pages, the data buffer size will be 128 * 4 KiB = 512 MiB if >> none >> of the pages involved in the I/O are contiguous. > > This change just increases the shost->max_sectors limit from 501KB to > 1Mb, but the final value will be overridden by the optimal transfer > length defined in the VPD, right? Hi Bean, It seems to me that the block layer only uses the optimal transfer size (io_opt) to determine how much data to read ahead during sequential reads? See also disk_update_readahead(). The above patch increases max_sectors but is not sufficient to increase the maximum transfer size: SG_ALL (128) * 4 KiB (dma_boundary + 1) = 512 KiB. To increase the maximum transfer size, the dma_boundary parameter would have to be modified. I have not yet submitted a patch that modifies that parameter since on my test setup (Exynos host controller) the current value is the largest value supported. Thanks, Bart.
On Tue, 2022-09-06 at 11:04 -0700, Bart Van Assche wrote: > > Hi Bean, > > It seems to me that the block layer only uses the optimal transfer > size > (io_opt) to determine how much data to read ahead during sequential > reads? See also disk_update_readahead(). > Bart, Sorry for later reply, Didn't notice a question here. In the upstream standard Linux kernel, if the device supports a valid VPD, I think, your this change will also not change the read-ahead window. .... sdkp->opt_xfer_blocks = get_unaligned_be32(&vpd->data[12]); sd_revalidate_disk() { ... if (sd_validate_opt_xfer_size(sdkp, dev_max)) { q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); } ... } device_add_disk() disk_update_readahead(disk) { disk->bdi->ra_pages = max(queue_io_opt(q) * 2 / PAGE_SIZE, VM_READAHEAD_PAGES); } e.g., if device reports opt 512KB, and the maximum transfer length will be 512KB, and readahead window will be 1MB. Kind regards, Bean
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 36b7212e9cb5..678bc8d6d6aa 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8365,6 +8365,7 @@ static struct scsi_host_template ufshcd_driver_template = { .cmd_per_lun = UFSHCD_CMD_PER_LUN, .can_queue = UFSHCD_CAN_QUEUE, .max_segment_size = PRDT_DATA_BYTE_COUNT_MAX, + .max_sectors = (1 << 20) / SECTOR_SIZE, /* 1 MiB */ .max_host_blocked = 1, .track_queue_depth = 1, .sdev_groups = ufshcd_driver_groups,
Measurements for one particular UFS controller + UFS device show a 25% higher read bandwidth if the maximum data buffer size is increased from 512 KiB to 1 MiB. Hence increase the maximum size of the data buffer associated with a single request from SCSI_DEFAULT_MAX_SECTORS (1024) * 512 bytes = 512 KiB to 1 MiB. Notes: - The maximum data buffer size supported by the UFSHCI specification is 65535 * 256 KiB or about 16 GiB. - The maximum data buffer size for READ(10) commands is 65535 logical blocks. To transfer more than 65535 * 4096 bytes = 255 MiB with a single SCSI command, the READ(16) command is required. Support for READ(16) is optional in the UFS 3.1 and UFS 4.0 standards. Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: Avri Altman <avri.altman@wdc.com> Cc: Bean Huo <beanhuo@micron.com> Cc: Stanley Chu <stanley.chu@mediatek.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- Changes compared to v2: changed maximum transfer size 255 MiB to 1 MiB. Changes compared to v1: changed maximum transfer size from 1 GiB to 255 MiB. drivers/ufs/core/ufshcd.c | 1 + 1 file changed, 1 insertion(+)