Re: Full hostlock pushdown available

Re: Full hostlock pushdown available

[Jeff Garzik]

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)

Regards,

	Jeff

Re: Full hostlock pushdown available

[Nicholas A. Bellinger]

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

Re: Full hostlock pushdown available

[Andi Kleen]

> 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.

The goal here is not really what comes out of this patch,
but dropping the host lock completely. This is just the first step.

> 
> 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?

Possibly, but it's not a mechanic change.

-Andi

Re: Full hostlock pushdown available

[Jeff Garzik]

On 11/02/2010 05:21 AM, Andi Kleen wrote:
>> 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?
>
> Possibly, but it's not a mechanic change.

Oh come on.  Anybody can run a script.  It's not a mechanical change if 
you failed to create an equivalent transformation, fail to maintain 
existing lock order, _inverting_ the existing locking.

Have you done any analysis on the correctness of this new locking?


> The goal here is not really what comes out of this patch,
> but dropping the host lock completely. This is just the first step.

That doesn't excuse lack of analysis or correctness.

Boaz' approach is OBVIOUSLY mechanical, correct and bisectable.  Yours 
is not.

	Jeff

Re: Full hostlock pushdown available

[Andi Kleen]

> Boaz' approach is OBVIOUSLY mechanical, correct and bisectable.
> Yours is not.

Sorry Jeff, but my patch has 100% the same locking order as Boaz.
The only difference is in which function it is.

-Andi

Re: Full hostlock pushdown available

[Jeff Garzik]

On 11/02/2010 01:53 PM, Andi Kleen wrote:
>> Boaz' approach is OBVIOUSLY mechanical, correct and bisectable.
>> Yours is not.
>
> Sorry Jeff, but my patch has 100% the same locking order as Boaz.
> The only difference is in which function it is.

Incorrect.  Please re-read your own code.

Your code eliminates the drop+reacquire of the host_lock, choosing 
instead a brand new, untested, unreviewing locking scheme of holding the 
host_lock for the entirety of libata's queuecommand run.

In contrast, Boaz' proposed pattern of adding stub functions such as

	int queuecmd_unlocked(scsi_cmd cmd, callback done)
	{
		lock_irqsave
		get serial

		call driver's existing queuecommand

		unlock_irqrestore
	}

does not change libata's locking at all, because it does not modify a 
driver's queuecommand at all.  It is obviously correct.

Or to restate another way,

	Current libata locking
	----------------------
	spin_lock_irqsave(host_lock)
	spin_unlock(host_lock)
	spin_lock(ap lock)
	...
	spin_unlock(ap lock)
	spin_lock(host_lock)
	spin_unlock_irqrestore(host_lock)


	Andi's brand new locking scheme
	(missing the release of host lock)
	----------------------------------
	spin_lock_irqsave(host_lock)
	spin_lock(ap lock)
	...
	spin_unlock(ap lock)
	spin_unlock_irqrestore(host_lock)

The two versions are quite obviously NOT equivalent in any way, because 
you have ADDED the holding of host_lock for the duration of libata's 
queuecommand.

	Jeff

Re: Full hostlock pushdown available

[Nicholas A. Bellinger]

On Tue, 2010-11-02 at 18:53 +0100, Andi Kleen wrote:
> > Boaz' approach is OBVIOUSLY mechanical, correct and bisectable.
> > Yours is not.
> 
> Sorry Jeff, but my patch has 100% the same locking order as Boaz.
> The only difference is in which function it is.
> 
> -Andi

Hey Guys,

Just a heads up..  I am respinning a host_lock-less-for-37-v10 that
converts to use macros following Boaz's suggesstion and Jeff's
recommendations to ensure consistency for those LLDs that we have
identified as being able to run in 'lock-less' mode.

I was hoping to have this pushed out last night but ended up getting
sidetracked.  This should be ready to go later this afternoon.

Best,

--nab

Re: Full hostlock pushdown available

[Jeff Garzik]

A proper host-lock pushdown should be two one-line changes to each driver:


1) call

	DEF_SCSI_QUEUECMD_NOLCK(function name of existing queuecommand);

2) in each driver's Scsi_Host_Template, rename .queuecommand hook from

	XXX

to

	XXX_unlocked

Then define DEF_SCSI_QUEUECMD_NOLCK() macro in some common scsi header.

Simple.  Easy to review.  Obviously correct.  Fewest LOC changed.

	Jeff