Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Ming Lei]

On Fri, Apr 27, 2018 at 09:39:47AM -0600, Jens Axboe wrote:
> On 4/27/18 9:31 AM, Bart Van Assche wrote:
> > On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> >> This patches removes the expensive atomic opeation on host-wide counter
> >> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
> >> 15% with this change in IO test over scsi_debug.
> >>
> >>
> >> Ming Lei (3):
> >>   scsi: introduce scsi_host_busy()
> >>   scsi: read host_busy via scsi_host_busy()
> >>   scsi: avoid to hold host-wide counter of host_busy for scsi_mq
> >>
> >>  drivers/scsi/advansys.c   |  8 
> >>  drivers/scsi/hosts.c  | 32 
> >> +++
> >>  drivers/scsi/libsas/sas_scsi_host.c   |  4 ++--
> >>  drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
> >>  drivers/scsi/mpt3sas/mpt3sas_base.c   |  4 ++--
> >>  drivers/scsi/qlogicpti.c  |  2 +-
> >>  drivers/scsi/scsi.c   |  2 +-
> >>  drivers/scsi/scsi_error.c |  6 +++---
> >>  drivers/scsi/scsi_lib.c   | 23 --
> >>  drivers/scsi/scsi_sysfs.c |  2 +-
> >>  include/scsi/scsi_host.h  |  1 +
> >>  11 files changed, 65 insertions(+), 21 deletions(-)\
> > 
> > Hello Ming,
> > 
> > From the MAINTAINERS file:
> > 
> > SCSI SUBSYSTEM
> > M:  "James E.J. Bottomley" 
> > T:  git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
> > M:  "Martin K. Petersen" 
> > T:  git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> > L:  linux-s...@vger.kernel.org
> > S:  Maintained
> > F:  Documentation/devicetree/bindings/scsi/
> > F:  drivers/scsi/
> > F:  include/scsi/
> > 
> > Hence my surprise when I saw that you sent this patch series to Jens instead
> > of James and Martin?
> 
> Martin and James are both on the CC as well. For what it's worth, the patch
> seems like a good approach to me. To handle the case that Hannes was concerned
> about (older drivers doing internal command issue), I would suggest that those
> drivers get instrumented to include a inc/dec of the host busy count for
> internal commands that bypass the normal tagging. That means the mq case needs
> to be
> 
> blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
>   _flight);
> return in_flight.cnt + atomic_read(>host_busy);
> 
> The atomic read is basically free, once we get rid of the dirty of that
> variable on each IO.

Actually the internal command isn't submitted via normal IO path, then
it won't be completed via the normal completion path(soft_irq_done, or
.timeout), so handling internal command doesn't touch the scsi generic
counter of .host_busy.

I have talked with Hannes a bit about this at LSFMM, looks he agreed too.

Thanks,
Ming

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Jens Axboe]

On 4/27/18 9:48 AM, Bart Van Assche wrote:
> On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote:
>> blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
>>  _flight);
>> return in_flight.cnt + atomic_read(>host_busy);
>>
>> The atomic read is basically free, once we get rid of the dirty of that
>> variable on each IO.
> 
> Hello Jens,
> 
> What makes you think that " + atomic_read(>host_busy)" is necessary?
> I am not aware of any code outside the SCSI core that modifies the host_busy
> member.

It's a (scalable) hack to count those as well. Going forward they should be
converted to just using reserved tags, of course.

-- 
Jens Axboe

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Laurence Oberman]

On Fri, 2018-04-27 at 15:48 +, Bart Van Assche wrote:
> On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote:
> > blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
> > _flight);
> > return in_flight.cnt + atomic_read(>host_busy);
> > 
> > The atomic read is basically free, once we get rid of the dirty of
> > that
> > variable on each IO.
> 
> Hello Jens,
> 
> What makes you think that " + atomic_read(>host_busy)" is
> necessary?
> I am not aware of any code outside the SCSI core that modifies the
> host_busy
> member.
> 
> Thanks,
> 
> Bart.
> 
> 
> 

As part of testing latest upstream in MQ and non-MQ I intend to test
this patch series fully on actual hardware
F/C 8G to memory backed array LUNS and of course SRP/RDMA
I have started working on this and will report back.
Thanks
Laurence

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Bart Van Assche]

On Fri, 2018-04-27 at 09:39 -0600, Jens Axboe wrote:
> blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
>   _flight);
> return in_flight.cnt + atomic_read(>host_busy);
> 
> The atomic read is basically free, once we get rid of the dirty of that
> variable on each IO.

Hello Jens,

What makes you think that " + atomic_read(>host_busy)" is necessary?
I am not aware of any code outside the SCSI core that modifies the host_busy
member.

Thanks,

Bart.

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Jens Axboe]

On 4/27/18 9:31 AM, Bart Van Assche wrote:
> On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
>> This patches removes the expensive atomic opeation on host-wide counter
>> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
>> 15% with this change in IO test over scsi_debug.
>>
>>
>> Ming Lei (3):
>>   scsi: introduce scsi_host_busy()
>>   scsi: read host_busy via scsi_host_busy()
>>   scsi: avoid to hold host-wide counter of host_busy for scsi_mq
>>
>>  drivers/scsi/advansys.c   |  8 
>>  drivers/scsi/hosts.c  | 32 
>> +++
>>  drivers/scsi/libsas/sas_scsi_host.c   |  4 ++--
>>  drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
>>  drivers/scsi/mpt3sas/mpt3sas_base.c   |  4 ++--
>>  drivers/scsi/qlogicpti.c  |  2 +-
>>  drivers/scsi/scsi.c   |  2 +-
>>  drivers/scsi/scsi_error.c |  6 +++---
>>  drivers/scsi/scsi_lib.c   | 23 --
>>  drivers/scsi/scsi_sysfs.c |  2 +-
>>  include/scsi/scsi_host.h  |  1 +
>>  11 files changed, 65 insertions(+), 21 deletions(-)\
> 
> Hello Ming,
> 
> From the MAINTAINERS file:
> 
> SCSI SUBSYSTEM
> M:  "James E.J. Bottomley" 
> T:  git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
> M:  "Martin K. Petersen" 
> T:  git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
> L:  linux-s...@vger.kernel.org
> S:  Maintained
> F:  Documentation/devicetree/bindings/scsi/
> F:  drivers/scsi/
> F:  include/scsi/
> 
> Hence my surprise when I saw that you sent this patch series to Jens instead
> of James and Martin?

Martin and James are both on the CC as well. For what it's worth, the patch
seems like a good approach to me. To handle the case that Hannes was concerned
about (older drivers doing internal command issue), I would suggest that those
drivers get instrumented to include a inc/dec of the host busy count for
internal commands that bypass the normal tagging. That means the mq case needs
to be

blk_mq_tagset_busy_iter(>tag_set, scsi_host_check_in_flight,
_flight);
return in_flight.cnt + atomic_read(>host_busy);

The atomic read is basically free, once we get rid of the dirty of that
variable on each IO.

-- 
Jens Axboe

Re: [PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Bart Van Assche]

On Fri, 2018-04-20 at 14:57 +0800, Ming Lei wrote:
> This patches removes the expensive atomic opeation on host-wide counter
> of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
> 15% with this change in IO test over scsi_debug.
> 
> 
> Ming Lei (3):
>   scsi: introduce scsi_host_busy()
>   scsi: read host_busy via scsi_host_busy()
>   scsi: avoid to hold host-wide counter of host_busy for scsi_mq
> 
>  drivers/scsi/advansys.c   |  8 
>  drivers/scsi/hosts.c  | 32 
> +++
>  drivers/scsi/libsas/sas_scsi_host.c   |  4 ++--
>  drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
>  drivers/scsi/mpt3sas/mpt3sas_base.c   |  4 ++--
>  drivers/scsi/qlogicpti.c  |  2 +-
>  drivers/scsi/scsi.c   |  2 +-
>  drivers/scsi/scsi_error.c |  6 +++---
>  drivers/scsi/scsi_lib.c   | 23 --
>  drivers/scsi/scsi_sysfs.c |  2 +-
>  include/scsi/scsi_host.h  |  1 +
>  11 files changed, 65 insertions(+), 21 deletions(-)\

Hello Ming,

From the MAINTAINERS file:

SCSI SUBSYSTEM
M:  "James E.J. Bottomley" 
T:  git git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git
M:  "Martin K. Petersen" 
T:  git git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git
L:  linux-s...@vger.kernel.org
S:  Maintained
F:  Documentation/devicetree/bindings/scsi/
F:  drivers/scsi/
F:  include/scsi/

Hence my surprise when I saw that you sent this patch series to Jens instead
of James and Martin?

Bart.

[PATCH 0/3] scsi: scsi-mq: don't hold host_busy in IO path

[Ming Lei]

Hi,

This patches removes the expensive atomic opeation on host-wide counter
of .host_busy for scsi-mq, and it is observed that IOPS can be increased by
15% with this change in IO test over scsi_debug.


Ming Lei (3):
  scsi: introduce scsi_host_busy()
  scsi: read host_busy via scsi_host_busy()
  scsi: avoid to hold host-wide counter of host_busy for scsi_mq

 drivers/scsi/advansys.c   |  8 
 drivers/scsi/hosts.c  | 32 +++
 drivers/scsi/libsas/sas_scsi_host.c   |  4 ++--
 drivers/scsi/megaraid/megaraid_sas_base.c |  2 +-
 drivers/scsi/mpt3sas/mpt3sas_base.c   |  4 ++--
 drivers/scsi/qlogicpti.c  |  2 +-
 drivers/scsi/scsi.c   |  2 +-
 drivers/scsi/scsi_error.c |  6 +++---
 drivers/scsi/scsi_lib.c   | 23 --
 drivers/scsi/scsi_sysfs.c |  2 +-
 include/scsi/scsi_host.h  |  1 +
 11 files changed, 65 insertions(+), 21 deletions(-)


Cc: Omar Sandoval ,
Cc: "Martin K. Petersen" ,
Cc: James Bottomley ,
Cc: Christoph Hellwig ,
Cc: Don Brace 
Cc: Kashyap Desai 
Cc: Mike Snitzer 
Cc: Hannes Reinecke 
Cc: Laurence Oberman 

-- 
2.9.5