Re: Full hostlock pushdown available

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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