Re: [PATCH V4 1/2] scsi: core: avoid host-wide host_busy counter for scsi_mq

[Ming Lei <ming.lei@redhat.com>]

On Thu, Oct 24, 2019 at 10:19:21AM +0100, John Garry wrote:
> On 24/10/2019 01:58, Ming Lei wrote:
> > On Wed, Oct 23, 2019 at 09:52:41AM +0100, John Garry wrote:
> > > On 09/10/2019 10:32, Ming Lei wrote:
> > > > It isn't necessary to check the host depth in scsi_queue_rq() any more
> > > > since it has been respected by blk-mq before calling scsi_queue_rq() via
> > > > getting driver tag.
> > > > 
> > > > Lots of LUNs may attach to same host and per-host IOPS may reach millions,
> > > > so we should avoid expensive atomic operations on the host-wide counter in
> > > > the IO path.
> > > > 
> > > > This patch implements scsi_host_busy() via blk_mq_tagset_busy_iter()
> > > > with one scsi command state for reading the count of busy IOs for scsi_mq.
> > > > 
> > > > It is observed that IOPS is increased by 15% in IO test on scsi_debug (32
> > > > LUNs, 32 submit queues, 1024 can_queue, libaio/dio) in a dual-socket
> > > > system.
> > > > 
> > > > V2:
> > > > 	- introduce SCMD_STATE_INFLIGHT for getting accurate host busy
> > > > 	via blk_mq_tagset_busy_iter()
> > > > 	- verified that original Jens's report[1] is fixed
> > > > 	- verified that SCSI timeout/abort works fine
> > > > 
> > > > [1] https://www.spinics.net/lists/linux-scsi/msg122867.html
> > > > [2] V1 & its revert:
> > > > 
> > > > d772a65d8a6c Revert "scsi: core: avoid host-wide host_busy counter for scsi_mq"
> > > > 23aa8e69f2c6 Revert "scsi: core: fix scsi_host_queue_ready"
> > > > 265d59aacbce scsi: core: fix scsi_host_queue_ready
> > > > 328728630d9f scsi: core: avoid host-wide host_busy counter for scsi_mq
> > > > 
> 
> [not trimming]
> 
> > > > Cc: Jens Axboe <axboe@kernel.dk>
> > > > Cc: Ewan D. Milne <emilne@redhat.com>
> > > > Cc: Omar Sandoval <osandov@fb.com>,
> > > > Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
> > > > Cc: James Bottomley <james.bottomley@hansenpartnership.com>,
> > > > Cc: Christoph Hellwig <hch@lst.de>,
> > > > Cc: Kashyap Desai <kashyap.desai@broadcom.com>
> > > > Cc: Hannes Reinecke <hare@suse.de>
> > > > Cc: Laurence Oberman <loberman@redhat.com>
> > > > Cc: Bart Van Assche <bart.vanassche@wdc.com>
> > > > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > > > ---
> > > >    drivers/scsi/hosts.c     | 19 ++++++++++++++++++-
> > > >    drivers/scsi/scsi.c      |  2 +-
> > > >    drivers/scsi/scsi_lib.c  | 35 +++++++++++++++++------------------
> > > >    drivers/scsi/scsi_priv.h |  2 +-
> > > >    include/scsi/scsi_cmnd.h |  1 +
> > > >    include/scsi/scsi_host.h |  1 -
> > > >    6 files changed, 38 insertions(+), 22 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > > > index 55522b7162d3..1d669e47b692 100644
> > > > --- a/drivers/scsi/hosts.c
> > > > +++ b/drivers/scsi/hosts.c
> > > > @@ -38,6 +38,7 @@
> > > >    #include <scsi/scsi_device.h>
> > > >    #include <scsi/scsi_host.h>
> > > >    #include <scsi/scsi_transport.h>
> > > > +#include <scsi/scsi_cmnd.h>
> > > 
> > > alphabetic ordering could be maintained
> > > 
> > > >    #include "scsi_priv.h"
> > > >    #include "scsi_logging.h"
> > > > @@ -554,13 +555,29 @@ struct Scsi_Host *scsi_host_get(struct Scsi_Host *shost)
> > > >    }
> > > >    EXPORT_SYMBOL(scsi_host_get);
> > > > +static bool scsi_host_check_in_flight(struct request *rq, void *data,
> > > > +				      bool reserved)
> > > > +{
> > > > +	int *count = data;
> > > > +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
> > > > +
> > > > +	if (test_bit(SCMD_STATE_INFLIGHT, &cmd->state))
> > > > +		(*count)++;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >    /**
> > > >     * scsi_host_busy - Return the host busy counter
> > > 
> > > is this comment accurate now?
> > 
> > Nothing changed wrt. the above comment, I will explain below.
> 
> I mean that the host does not maintain a dedicated "busy counter" any longer
> (that was Scsi_Host.host_busy), so could be reworded accordingly.

It is still a counter in logical.

> 
> > 
> > > 
> > > >     * @shost:	Pointer to Scsi_Host to inc.
> > > >     **/
> > > >    int scsi_host_busy(struct Scsi_Host *shost)
> > > >    {
> > > > -	return atomic_read(&shost->host_busy);
> > > > +	int cnt = 0;
> > > > +
> > > > +	blk_mq_tagset_busy_iter(&shost->tag_set,
> > > > +				scsi_host_check_in_flight, &cnt);
> > > 
> > > When I check blk_mq_tagset_busy_iter(), it does not seem to lock the tags
> > > for all hw queues against preemption for the counting, so how can we
> > > guarantee that this count will be always accurate?
> > > 
> > > Or it never can be and you just don't care?
> > 
> > When the atomic variable of .host_busy is used, there isn't any host lock
> > to sync updating the variable and reading the variable, so nothing
> > changed wrt. its usage given atomic variable can be updated when reading the
> > variable.
> > 
> > That means it depends on its users. If the user doesn't care it, no lock
> > is needed, otherwise user will deal with that, such as
> > scsi_eh_scmd_add() and scsi_eh_inc_host_failed(), the host is put into
> > SHOST_RECOVERY state first to prevent new requests from being queued,
> > then read the counter in RCU callback via call_rcu().
> > 
> > > 
> > > > +	return cnt;
> > > >    }
> > > >    EXPORT_SYMBOL(scsi_host_busy);
> > > > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > > > index 1f5b5c8a7f72..ddc4ec6ea2a1 100644
> > > > --- a/drivers/scsi/scsi.c
> > > > +++ b/drivers/scsi/scsi.c
> > > > @@ -186,7 +186,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
> > > >    	struct scsi_driver *drv;
> > > >    	unsigned int good_bytes;
> > > > -	scsi_device_unbusy(sdev);
> > > > +	scsi_device_unbusy(sdev, cmd);
> > > >    	/*
> > > >    	 * Clear the flags that say that the device/target/host is no longer
> > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > > index dc210b9d4896..b6f66dcb15a5 100644
> > > > --- a/drivers/scsi/scsi_lib.c
> > > > +++ b/drivers/scsi/scsi_lib.c
> > > > @@ -189,7 +189,7 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
> > > >    	 * active on the host/device.
> > > >    	 */
> > > >    	if (unbusy)
> > > > -		scsi_device_unbusy(device);
> > > > +		scsi_device_unbusy(device, cmd);
> > > >    	/*
> > > >    	 * Requeue this command.  It will go before all other commands > @@
> > > > -329,12 +329,12 @@ static void scsi_init_cmd_errh(struct scsi_cmnd
> > > *cmd)
> > > 
> > > 
> > > The comment for scsi_dec_host_busy() is "Decrement the host_busy counter and
> > > ", so does this need to be fixed up?
> > 
> > Yeah, will fix it in next version.
> > 
> > > 
> > > I'm guessing that there's lots more places like this...
> > 
> > I just run a grep, looks not found others.
> > 
> > > 
> > > >     * host_failed counter or that it notices the shost state change made by
> > > >     * scsi_eh_scmd_add().
> > > >     */
> > > 
> > > 
> > > > -static void scsi_dec_host_busy(struct Scsi_Host *shost)
> > > > +static void scsi_dec_host_busy(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> > > >    {
> > > >    	unsigned long flags;
> > > >    	rcu_read_lock();
> > > > -	atomic_dec(&shost->host_busy);
> > > > +	__clear_bit(SCMD_STATE_INFLIGHT, &cmd->state);
> > > >    	if (unlikely(scsi_host_in_recovery(shost))) {
> > > >    		spin_lock_irqsave(shost->host_lock, flags);
> > > >    		if (shost->host_failed || shost->host_eh_scheduled)
> > > > @@ -344,12 +344,12 @@ static void scsi_dec_host_busy(struct Scsi_Host *shost)
> > > >    	rcu_read_unlock();
> > > >    }
> > > > -void scsi_device_unbusy(struct scsi_device *sdev)
> > > > +void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
> > > >    {
> > > >    	struct Scsi_Host *shost = sdev->host;
> > > >    	struct scsi_target *starget = scsi_target(sdev);
> > > > -	scsi_dec_host_busy(shost);
> > > > +	scsi_dec_host_busy(shost, cmd);
> > > >    	if (starget->can_queue > 0)
> > > >    		atomic_dec(&starget->target_busy);
> > > > @@ -430,9 +430,6 @@ static inline bool scsi_target_is_busy(struct scsi_target *starget)
> > > >    static inline bool scsi_host_is_busy(struct Scsi_Host *shost)
> > > >    {
> > > > -	if (shost->can_queue > 0 &&
> > > > -	    atomic_read(&shost->host_busy) >= shost->can_queue)
> > > > -		return true;
> > > 
> > > Do we still honour "do not send more than can_queue commands to the
> > > adapter", regardless of how many queues the LLDD exposes?
> > 
> > What we should honour is that 'do not send more than can_queue commands
> > to each hw queue', that is exactly guaranteed by blk-mq.
> 
> In scsi_host.h, we have for scsi_host_template.can_queue: "It is set to the
> maximum number of simultaneous commands a given host adapter will accept.",
> so that should be honoured.

That words should have been changed to:

"It is set to the maximum number of simultaneous commands a given host adapter's
hw queue will accept."

> 
> And Scsi_host.nr_hw_queues: "it is assumed that each hardware queue has a
> queue depth of can_queue. In other words, the total queue depth per host is
> nr_hw_queues * can_queue."

The above is correct.

> 
> I don't read "total queue depth per host" same as "maximum number of
> simultaneous commands a given host adapter will accept". If anything, the
> nr_hw_queues comment is ambiguous.
> 
> > 
> > The point is simple, because each hw queue has its own independent tags,
> > that is why I mentioned your Hisilicon SAS can't be converted to MQ
> > easily cause this hardware has only single shared tags.
> 
> Please be aware that HiSilicon SAS HW would not be unique for SCSI HBAs in
> this regard, in that the unique hostwide tag is not just for HBA HW IO
> management, but also is used as the tag for SCSI TMFs.

Right.

> 
> Just checking mpt3sas seems similar:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/mpt3sas/mpt3sas_scsih.c#n2918
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/scsi/mpt3sas/mpt3sas_base.c#n3546

Not only mpt3sas, there are also HPSA and more. And these drivers have to
support single hw queue of blk-mq, instead of real MQ. And the reason is that
these HBA has single tags.


Thanks,
Ming