Re: [PATCH 03/11] scsi: Add IRQ_DISABLE_SCSI_QCMD wrapper

Nicholas A. Bellinger
2010-11-19T00:09:52Z

On Thu, 2010-11-18 at 18:15 -0500, Jeff Garzik wrote:
> On 11/18/2010 06:05 PM, Nicholas A. Bellinger wrote:
> > On Thu, 2010-11-18 at 12:25 +0200, Boaz Harrosh wrote:
> >> On 11/18/2010 12:37 AM, Christoph Hellwig wrote:
> >>> On Wed, Nov 17, 2010 at 02:29:30PM -0800, Nicholas A. Bellinger wrote:
> >>>> Hmmm, this is following jgarzik's recommendation for LLDs that we could
> >>>> not immediately identify a internal spin_lock to disable interrupts
> >>>> upon. (eg: not libiscsi and libata).
> >>>
> >>> In that case wait for the driver author to identify it. If there's
> >>> no maintainer in reach chance is the driver doesn't care about the push
> >>> down. No need to rush any of this, do it one driver at a time and get
> >>> it right.
> >>
> >> I totally agree with Christoph. Patches 5, 6, 8, 11 all change behaviour
> >>
> >> I would like to see an "I audit the driver and ..." Please see my comment
> >> to [patch 6]
> >
> > Just to reiterate the point here.. The only LLDs that have been made
> > 'lock-less' in this first round are LLDs that:
> >
> > *) Where already using the legacy optimization of releasing host_lock,
> > doing lld_work, and reaquring before returning from their
> > lld_queuecommand().
> >
> > *) Have been tested by folks explictly during the various initial
> > attempts at lock_less mode, which still appear to be functionally
> > equivilent minus the precatuion of local_irq_[save,release] usage.
> >
> >>
> >> Do it one by one and open-code the local_irq_save/restore inside the
> >> main function. It's not like you can get away from a total audit and
> >> testing.
> >>
> >
> > Hmmm, I am not sure I agree with open coding these atm. I would much
> > rather get LLD maintainer feedback first for these initial cases so we
> > can figure out which of these patches can be further optimized along the
> > lines of libiscsi and libata.
>
> I don't think we'll be able to get around looking at each case
> individually, therefore I do not see the utility of adding
> IRQ_DISABLE_SCSI_QCMD().
>

I am definately not trying to imply that this should be merged as
lock_less without the necessary individual review and maintainer
signoffs.

> If you're looking at each case, then you likely have an idea of exactly
> how the code should be updated.
>

Sure, the main purpose of this series is to get the ball rolling on LLDs
that we (you can read this as selfish we ;) collectively care about for
high performance modern lock_less usage.. I am more than happy to get
NACKs on a per LLD basis for hardware that I am not currently able to
test with in order to get the maintainers involved to make the call if
proper semi optimized -> fully optimized lock_less operation is in their
near future.

Best,

--nab