Re: Full hostlock pushdown available

Nicholas A. Bellinger
2010-11-01T21:11:31Z

On Mon, 2010-11-01 at 14:59 -0400, Jeff Garzik wrote:
> On 11/01/2010 01:57 PM, Andi Kleen wrote:
> > @@ -3186,11 +3186,14 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done
> > struct ata_device *dev;
> > struct scsi_device *scsidev = cmd->device;
> > struct Scsi_Host *shost = scsidev->host;
> > + unsigned long irqflags;
> > int rc = 0;
> >
> > ap = ata_shost_to_port(shost);
> >
> > - spin_unlock(shost->host_lock);
> > + spin_lock_irqsave(shost->host_lock, irqflags);
> > + scsi_cmd_get_serial(shost, cmd);
> > +
> > spin_lock(ap->lock);
> >
> > ata_scsi_dump_cdb(ap, cmd);
> > @@ -3205,6 +3208,7 @@ int ata_scsi_queuecmd(struct scsi_cmnd *cmd, void (*done)(
> >
> > spin_unlock(ap->lock);
> > spin_lock(shost->host_lock);
> > + spin_unlock_irqrestore(shost->host_lock, irqflags);
> > return rc;
> > }
> >
>
> It's a bit disappointing that libata's lock profile in this patch is
> quite different than that of current upstream: with your patch, libata
> holds the scsi host lock for a considerably longer period of time, while
> also holding the ATA port/host spinlock.
>
> IOW, it's doing the exact opposite of what the previous code did
> (release the scsi host lock, before acquiring the ATA port/host
> spinlock), not at all an equivalent transformation.
>
> The following sequence would seem to better preserve the existing lock
> profile, correct?
>
> local_irq_save(flags)
>
> spin_lock(shost->host_lock)
> scsi_cmd_get_serial()
> spin_unlock(shost->host_lock)
>
> spin_lock(ap->lock)
> ...
> spin_unlock(ap->lock)
>
> local_irq_restore(flags)
>

Hmmmmm yes.. Along with Andi's generated patches I think we should
strongly consider including the atomic_t Scsi_Host->cmd_serial_number
patch here as well for scsi_cmd_get_serial() (which needs EXPORT_SYMBOL)

http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=c047a53c52ee6c90daf048b045750e18173fd011#patch68
http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=c047a53c52ee6c90daf048b045750e18173fd011#patch37
http://git.kernel.org/?p=linux/kernel/git/nab/lio-4.0.git;a=commitdiff;h=c047a53c52ee6c90daf048b045750e18173fd011#patch82

This ends up being a very simple change, and would allow libata to
immediately go host_lock less for scsi_cmd_dispatch() operation.

FYI, the current scoreboard for the global push down is available in
lio-core-4.0.git/host_lock-less-for-37-v9 at the top of the above
commit.. I think Andi is missing a few LLDs outside of /drivers/scsi/

Best,

--nab