[v6,0/6] Disable fair tag sharing for UFS devices

Message

Bart Van Assche Nov. 30, 2023, 7:31 p.m. UTC

Hi Jens,

The fair tag sharing algorithm reduces performance for UFS devices
significantly. This is because UFS devices have multiple logical units, a
limited queue depth (32 for UFS 3.1 devices), because it happens often that
multiple logical units are accessed and also because it takes time to
give tags back after activity on a request queue has stopped. This patch series
restores UFS device performance to that of the legacy block layer by disabling
fair tag sharing for UFS devices.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v5:
 - Request queues are frozen before fair tag sharing is changed.
 - Added a sysfs attribute to SCSI hosts for configuring fair tag sharing.

Changes compared to v4:
 - Rebased on top of kernel v6.7-rc1.

Changes compared to v3:
 - Instead of disabling fair tag sharing for all block drivers, introduce a
   flag for disabling it conditionally.

Changes between v2 and v3:
 - Rebased on top of the latest kernel.

Changes between v1 and v2:
 - Restored the tags->active_queues variable and thereby fixed the
   "uninitialized variable" warning reported by the kernel test robot.

Bart Van Assche (4):
  block: Make fair tag sharing configurable
  scsi: core: Make fair tag sharing configurable in the host template
  scsi: core: Make fair tag sharing configurable via sysfs
  scsi: ufs: Disable fair tag sharing

 block/blk-mq-debugfs.c    |  1 +
 block/blk-mq.c            | 28 ++++++++++++++++++++++++++++
 block/blk-mq.h            |  3 ++-
 drivers/scsi/hosts.c      |  1 +
 drivers/scsi/scsi_lib.c   |  2 ++
 drivers/scsi/scsi_sysfs.c | 30 ++++++++++++++++++++++++++++++
 drivers/ufs/core/ufshcd.c |  1 +
 include/linux/blk-mq.h    |  6 ++++--
 include/scsi/scsi_host.h  |  6 ++++++
 9 files changed, 75 insertions(+), 3 deletions(-)

Comments

Christoph Hellwig Dec. 4, 2023, 7:52 a.m. UTC | #1

On Thu, Nov 30, 2023 at 11:31:27AM -0800, Bart Van Assche wrote:
> Hi Jens,
> 
> The fair tag sharing algorithm reduces performance for UFS devices
> significantly. This is because UFS devices have multiple logical units, a
> limited queue depth (32 for UFS 3.1 devices), because it happens often that
> multiple logical units are accessed and also because it takes time to
> give tags back after activity on a request queue has stopped. This patch series
> restores UFS device performance to that of the legacy block layer by disabling
> fair tag sharing for UFS devices.

I feel like a broken record:

fair tag sharing exists for a reason.  Opting out of it for a specific
driver does not make any sense.  Either you can make a good argument
why you don't want it at all, or for specific configurations you
can clearly explain, or you make it work faster.  A "treat my driver
special" flag is never acceptable.

Bart Van Assche Dec. 5, 2023, 3:15 a.m. UTC | #2

On 12/3/23 23:52, Christoph Hellwig wrote:
> On Thu, Nov 30, 2023 at 11:31:27AM -0800, Bart Van Assche wrote:
>> The fair tag sharing algorithm reduces performance for UFS devices
>> significantly. This is because UFS devices have multiple logical units, a
>> limited queue depth (32 for UFS 3.1 devices), because it happens often that
>> multiple logical units are accessed and also because it takes time to
>> give tags back after activity on a request queue has stopped. This patch series
>> restores UFS device performance to that of the legacy block layer by disabling
>> fair tag sharing for UFS devices.
> 
> I feel like a broken record:
> 
> fair tag sharing exists for a reason.  Opting out of it for a specific
> driver does not make any sense.  Either you can make a good argument
> why you don't want it at all, or for specific configurations you
> can clearly explain, or you make it work faster.  A "treat my driver
> special" flag is never acceptable.

Hi Christoph,

Feedback that helps improving a patch series is always welcome.

Here is how I see fair tag sharing:
* Users probably want to configure minimum and maximum bandwidth or IOPS
   values instead of an equal number of tags per request queue. I assume
   that the fair tag sharing algorithm assigns an equal number of tags to
   each request queue since the run-time cost of the latter algorithm is
   much lower than the run-time cost of the previous algorithms.
* Any algorithm that is more sophisticated than the current fair tag
   sharing algorithm would have a higher runtime cost. We don't want to
   increase the cost of command processing in the block layer core.

Hence my proposal to disable the fair tag sharing algorithm if it's not
needed. I don't see a good alternative to this approach.

Thanks,

Bart.