RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

Arjan,

> > No.  We've had this IOWR problem since the inception of 5.x series
> > driver.  Software (SMS 3.0) has been built on top of the this
> > IOCTL
> 
> how about removing most if not all of these ioctls ?  The scsi layer
> has a *generic* "send passthrough" mechanism already for example.
> 

I'm not entirely clear on what you are alluding to here, are you
referring to SCSI_IOCTL_SEND_COMMAND?  There's significantly more
functionality embedded within the IOCTLs than simply sending passthrus
to devices.  Also, all of QLogic's drivers (linux, solaris, windows)
implement to this 'external ioctl' spec, making changes to Linux alone
would difficult.

Regards,
Andrew Vasquez

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

On Thu, Nov 06, 2003 at 11:33:28AM -0800, Andrew Vasquez wrote:
> I'm not entirely clear on what you are alluding to here, are you
> referring to SCSI_IOCTL_SEND_COMMAND?  There's significantly more
> functionality embedded within the IOCTLs than simply sending passthrus
> to devices.  Also, all of QLogic's drivers (linux, solaris, windows)
> implement to this 'external ioctl' spec, making changes to Linux alone
> would difficult.

I thought you had a hba lib to abstract the difference away?  Once again
the proper solution would be to just exclude all the ioctl mess for a
kernel build, if your HSV IHV and other three letter acronym partners
need the broken API they can patch it in again.

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

Christoph,

> More comments:
> 
>  - qla_vendor.c is still unused and should be killed
>

Yes, more baggage from old failover code, we should be able to kill it.

>  - your ioctl API gets worse and worse.  You don't expect this huge
>  dungpile of ioctls all marked _BAD to be merged, do you?  
>

No.  We've had this IOWR problem since the inception of 5.x series
driver.  Software (SMS 3.0) has been built on top of the this IOCTL
interface.  We painfully discovered this problem when we began to look
at other non-x86 platforms (ppc64).

>  Also having different ioctl values for different plattforms is not
>  an option for Linux.
> 

The better (right) fix would be to push this interface change onto the
caller of the IOCTLs where they can manage the differences there, and
the driver could once and for all shed itself of this nagging problem.
That is the consensus here.  The _BAD conversion was only done so the
driver would compile.

Regards,
Andrew Vasquez

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 338 bytes --]

On Thu, 2003-11-06 at 18:45, Andrew Vasquez wrote:

> No.  We've had this IOWR problem since the inception of 5.x series
> driver.  Software (SMS 3.0) has been built on top of the this IOCTL

how about removing most if not all of these ioctls ?
The scsi layer has a *generic* "send passthrough" mechanism already for
example.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

On Thu, Nov 06, 2003 at 09:45:50AM -0800, Andrew Vasquez wrote:
> No.  We've had this IOWR problem since the inception of 5.x series
> driver.  Software (SMS 3.0) has been built on top of the this IOCTL
> interface.  We painfully discovered this problem when we began to look
> at other non-x86 platforms (ppc64).

I don't know what SMS is, but Linux provides a stable ABI for stable
kernel series and only those.  We already have anough garbage from mistakes
in older kernel releases that we won't accept more with new driver
submission.  If SMS didn't use a abstraction library it would have a problem,
yes.

> >  Also having different ioctl values for different plattforms is not
> >  an option for Linux.
> > 
> 
> The better (right) fix would be to push this interface change onto the
> caller of the IOCTLs where they can manage the differences there, and
> the driver could once and for all shed itself of this nagging problem.
> That is the consensus here.  The _BAD conversion was only done so the
> driver would compile.

Sorry, I don't parse that.  There's no reason different architectures
should have differences in the ioctl calling convention, so there shouldn't
be anything to deal with in the caller either.

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

Jens,

> > 	o Given the ISPs inability to handle data segments that cross
> > 	  32-bit page boundaries, and the overhead in defensive logic
> > 	  within the driver to prevent these cases (compare the 6.x
> > 	  code to the 8.x code and you will see what I mean), by
> > 	  disabling clustering we guarantee that a single SG element
> > 	  never crosses a 4GB boundary.
> 
> That is not, however. Linux will never give you a segment crossing a
> 4GB boundary, exactly because most hardware cannot handle this.
> 
> > Now, in 2.6 there are some significant changes.  For one, with the
> > block layer rewrite and the ability to limit segment boundaries of a
> > block queue request with the blk_queue_segment_boundary() call, a LLDD
> > need not concern itself with any defensive fast-path logic to handle
> > the 4GB cross.
> 
> Ditto 2.4. See BH_PHYS_4G().
> 

Yes, but if I recall, these checks did not enter until late in the 2.4
branch.  The original (engineering) intent of the 6.x series branch
was to support all 2.4 kernels, though we are slowly (formally) moving
away from the restrictions.

> > So, we're left with the benefits of the overhead of this merge
> > process done by the block layer.  I'm certainly receptive to the
> > notion of reexamining the use of clustering given some solid data
> > points showing an (significant -- this is subjective) increase in
> > performance and/or a resounding 'yeah, enable it!' from those in
> > the block-layer 'know.'
> 
> In 2.6 clustering comes for free, so it would be silly not to enable
> it.
> 

Thanks for the clarification, I'll add the parameter back into the
template for the 8.x series driver.

Regards,
Andrew Vasquez

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Jens Axboe]

On Thu, Nov 06 2003, Andrew Vasquez wrote:
> Jens,
> 
> > > 	o Given the ISPs inability to handle data segments that cross
> > > 	  32-bit page boundaries, and the overhead in defensive logic
> > > 	  within the driver to prevent these cases (compare the 6.x
> > > 	  code to the 8.x code and you will see what I mean), by
> > > 	  disabling clustering we guarantee that a single SG element
> > > 	  never crosses a 4GB boundary.
> > 
> > That is not, however. Linux will never give you a segment crossing a
> > 4GB boundary, exactly because most hardware cannot handle this.
> > 
> > > Now, in 2.6 there are some significant changes.  For one, with the
> > > block layer rewrite and the ability to limit segment boundaries of a
> > > block queue request with the blk_queue_segment_boundary() call, a LLDD
> > > need not concern itself with any defensive fast-path logic to handle
> > > the 4GB cross.
> > 
> > Ditto 2.4. See BH_PHYS_4G().
> > 
> 
> Yes, but if I recall, these checks did not enter until late in the 2.4
> branch.  The original (engineering) intent of the 6.x series branch
> was to support all 2.4 kernels, though we are slowly (formally) moving
> away from the restrictions.

They were there at the same time as Linux supported > 1GB IO at all. So
that is incorrect, it's been there all along.

> > > So, we're left with the benefits of the overhead of this merge
> > > process done by the block layer.  I'm certainly receptive to the
> > > notion of reexamining the use of clustering given some solid data
> > > points showing an (significant -- this is subjective) increase in
> > > performance and/or a resounding 'yeah, enable it!' from those in
> > > the block-layer 'know.'
> > 
> > In 2.6 clustering comes for free, so it would be silly not to enable
> > it.
> > 
> 
> Thanks for the clarification, I'll add the parameter back into the
> template for the 8.x series driver.

Cool, thanks.

-- 
Jens Axboe

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

On Thu, 2003-11-06 at 18:14, Jens Axboe wrote:

> They were there at the same time as Linux supported > 1GB IO at all. So
> that is incorrect, it's been there all along.

.... ia64


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Jens Axboe]

On Thu, Nov 06 2003, Arjan van de Ven wrote:
> On Thu, 2003-11-06 at 18:14, Jens Axboe wrote:
> 
> > They were there at the same time as Linux supported > 1GB IO at all. So
> > that is incorrect, it's been there all along.
> 
> .... ia64

Yeah you are right, on 64-bit platforms that could have happened. I
actually thought that CONTIGUOUS_BUFFERS took care of 4GB, but on
checking it does not.

So clustering should have been disabled then (which in the in-kernel
drive it is not). Now both 2.4 and 2.6 make that guarentee. The only
argument for disabling clustering would be for 2.4 for CPU cycle
reasons.

-- 
Jens Axboe

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Arjan van de Ven]

On Thu, Nov 06, 2003 at 06:50:32PM +0100, Jens Axboe wrote:
> drive it is not). Now both 2.4 and 2.6 make that guarentee. The only
> argument for disabling clustering would be for 2.4 for CPU cycle
> reasons.

yep; on 2.4 it's rather visible in the profiles during some benchmark
test I did (like 5% of total CPU cycles)

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Jens Axboe]

On Thu, Nov 06 2003, Arjan van de Ven wrote:
> 
> On Thu, Nov 06, 2003 at 06:50:32PM +0100, Jens Axboe wrote:
> > drive it is not). Now both 2.4 and 2.6 make that guarentee. The only
> > argument for disabling clustering would be for 2.4 for CPU cycle
> > reasons.
> 
> yep; on 2.4 it's rather visible in the profiles during some benchmark
> test I did (like 5% of total CPU cycles)

yeah so I gather, although 5% seems extremely excessive (512-byte raw
io, on tons of disks?).

-- 
Jens Axboe

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

Christoph,

> > 	o Slab cache allocations for driver SRBs.
> 
> this one is broken.  You kall kmem_cache_alloc and kmem_cache_free
> under a spinlock which is not wrong (at least with GFP_ATOMIC) but
> useless.  You don't have a mempool so you will deadlock under load.
> You have a kmem_cache_t per host which is unessecary, one in the
> driver is enough.
> 

Thanks, I'll take a look at this.

> > the source-type tarball format.  It's not worth the extra effort
> > of building a separate drop-in kernel tarball, given the varying
> > release periods of the driver.  Similar drop-in-kernel-tarball
> > results can be had by:
> 
> Would be nice to unpack into a directory instead of CWD, like almost
> any tarball does..
> 

Ok.

> > Failover functionality is present in this distribution, at this
> > time I do not foresee a policy change regarding its presence in
> > the 8.x series driver.  The next beta release of 8.x will be
> > failover-feature resync'd with our latest 6.x beta (6.07.xx).
> 
> So it won't get merged into 2.6.x.  Not that it would be likely
> given your complete ignorance of the other TODO items.
>

Hmm, looking through your email from July 18th, I note three main
objections that have not been addressed:

	o Build process -- (three module interface for the ISPs), I
	  personally like the idea of a shared library module used
	  between the different ISP drivers.  Many others have voiced
	  their frustrations with the single driver-binary for each
	  ISP type that the directive from management is to have a
	  single binary for all *future* products including the 
	  ISP23xx (ISP2300/ISP2310/ISP2312/ISP2322) chips.

	  That unfortunately leaves ISP2100 and ISP2200 on the
	  periphery of development efforts.

	o Failover -- DM and the fastfail infrastructure appear to be
	  in the early stages of adoption (implementation?).  There's
	  no question that QLogic will support the interfaces once
	  they mature.  Let's just agree to disagree...

	o Structure and Form -- we're slowly chipping away at much of
	  the cruft.

Thank for the feedback.

> Sometimes I really wish to get some funding to work on a mergeable
> qla2xxx driver as it seems impossible to get one from qlogic..
> 

Regards,
Andrew Vasquez
Es wird nichts so heiß gegessen, wie es gekocht wird.

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Matthew Wilcox]

On Thu, Nov 06, 2003 at 09:02:28AM -0800, Andrew Vasquez wrote:
> Hmm, looking through your email from July 18th, I note three main
> objections that have not been addressed:
> 
> 	o Build process -- (three module interface for the ISPs), I
> 	  personally like the idea of a shared library module used
> 	  between the different ISP drivers.  Many others have voiced
> 	  their frustrations with the single driver-binary for each
> 	  ISP type that the directive from management is to have a
> 	  single binary for all *future* products including the 
> 	  ISP23xx (ISP2300/ISP2310/ISP2312/ISP2322) chips.
> 
> 	  That unfortunately leaves ISP2100 and ISP2200 on the
> 	  periphery of development efforts.

I wouldn't see a problem with having a structure like this:

ql2100.c
ql2200.c
ql23xx.c
qllib.c

and linking in whichever files are selected.  But you definitely only want
to build qllib.c once.

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

On Thu, Nov 06, 2003 at 05:53:06PM +0000, Matthew Wilcox wrote:
> > 
> > 	o Build process -- (three module interface for the ISPs), I
> > 	  personally like the idea of a shared library module used
> > 	  between the different ISP drivers.  Many others have voiced
> > 	  their frustrations with the single driver-binary for each
> > 	  ISP type that the directive from management is to have a
> > 	  single binary for all *future* products including the 
> > 	  ISP23xx (ISP2300/ISP2310/ISP2312/ISP2322) chips.
> > 
> > 	  That unfortunately leaves ISP2100 and ISP2200 on the
> > 	  periphery of development efforts.
> 
> I wouldn't see a problem with having a structure like this:
> 
> ql2100.c
> ql2200.c
> ql23xx.c
> qllib.c
> 
> and linking in whichever files are selected.  But you definitely only want
> to build qllib.c once.

Well, if you look at the driver you'll need all files except the firmware
anyway.  And now that we have the request_firmware interface anyway I'd
bebetter to move that out of the module, at least once initramfs settles down
a bit.

The issue is more lots of tiny ifdefs - those in the C source could be easily
hidden using pdev->device comparisms (and I think I'm gonna submit a patch
for that soon, the driver already does that for ISP23XX variants without
a proper abstraction).  Those in the headers are a bit more difficult as the
register layout is a bit different sometimes.  But with doing these as unions
instead of ifdefs and splitted subroutines this should be managæble as well,
the feral driver already does this nicely.

-- 
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

> 	o Failover -- DM and the fastfail infrastructure appear to be
> 	  in the early stages of adoption (implementation?).  There's
> 	  no question that QLogic will support the interfaces once
> 	  they mature.  Let's just agree to disagree...

Hey, there's no one saying you have to kill all your failover stuff.  Just
make it clearly optional and don't include it if_/when you submit the driver
for inclusion.  We just can't have a failover implementation for each vendor.
Not to mention that the LLDD really is the wrong place for it..

RE: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

Mike,

> > A new version of the 8.x series driver for Linux 2.6.x kernels has
> > been uploaded to SourceForge:
> 
> Thanks for the update.
> 
> Can you give more information on why clustering was turned off
> starting in b5?
> 

This change was part of a relatively large performance-patch that was
originally against the 6.x series driver.  There were two main reasons
for disabling clustering support:

	o Given the ability of the ISP to efficiently DMA to and from
	  a significant number of data segment descriptors (via
	  command/continuation type IOCBs), there were some questions
	  regarding the benefits that clustering provided given the
	  overhead incurred by the mid-layer attempting merge the
	  requests.

	o Given the ISPs inability to handle data segments that cross
	  32-bit page boundaries, and the overhead in defensive logic
	  within the driver to prevent these cases (compare the 6.x
	  code to the 8.x code and you will see what I mean), by
	  disabling clustering we guarantee that a single SG element
	  never crosses a 4GB boundary.

Now, in 2.6 there are some significant changes.  For one, with the
block layer rewrite and the ability to limit segment boundaries of a
block queue request with the blk_queue_segment_boundary() call, a LLDD
need not concern itself with any defensive fast-path logic to handle
the 4GB cross.

So, we're left with the benefits of the overhead of this merge process
done by the block layer.  I'm certainly receptive to the notion of
reexamining the use of clustering given some solid data points showing
an (significant -- this is subjective) increase in performance and/or a
resounding 'yeah, enable it!' from those in the block-layer 'know.'

Regards,
Andrew Vasquez

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Jens Axboe]

On Wed, Nov 05 2003, Andrew Vasquez wrote:
> Mike,
> 
> > > A new version of the 8.x series driver for Linux 2.6.x kernels has
> > > been uploaded to SourceForge:
> > 
> > Thanks for the update.
> > 
> > Can you give more information on why clustering was turned off
> > starting in b5?
> > 
> 
> This change was part of a relatively large performance-patch that was
> originally against the 6.x series driver.  There were two main reasons
> for disabling clustering support:
> 
> 	o Given the ability of the ISP to efficiently DMA to and from
> 	  a significant number of data segment descriptors (via
> 	  command/continuation type IOCBs), there were some questions
> 	  regarding the benefits that clustering provided given the
> 	  overhead incurred by the mid-layer attempting merge the
> 	  requests.

That is valid.

> 	o Given the ISPs inability to handle data segments that cross
> 	  32-bit page boundaries, and the overhead in defensive logic
> 	  within the driver to prevent these cases (compare the 6.x
> 	  code to the 8.x code and you will see what I mean), by
> 	  disabling clustering we guarantee that a single SG element
> 	  never crosses a 4GB boundary.

That is not, however. Linux will never give you a segment crossing a 4GB
boundary, exactly because most hardware cannot handle this.

> Now, in 2.6 there are some significant changes.  For one, with the
> block layer rewrite and the ability to limit segment boundaries of a
> block queue request with the blk_queue_segment_boundary() call, a LLDD
> need not concern itself with any defensive fast-path logic to handle
> the 4GB cross.

Ditto 2.4. See BH_PHYS_4G().

> So, we're left with the benefits of the overhead of this merge process
> done by the block layer.  I'm certainly receptive to the notion of
> reexamining the use of clustering given some solid data points showing
> an (significant -- this is subjective) increase in performance and/or a
> resounding 'yeah, enable it!' from those in the block-layer 'know.'

In 2.6 clustering comes for free, so it would be silly not to enable it.

-- 
Jens Axboe

[ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Andrew Vasquez]

All,

A new version of the 8.x series driver for Linux 2.6.x kernels has
been uploaded to SourceForge:

	http://sourceforge.net/projects/linux-qla2xxx/

Changes from previous release (8.00.00b5) include:

	o Intelligent RSCN handling.
	o Slab cache allocations for driver SRBs.
	o Support larger numbers of targets.
	o Initial rework of debug logging facilities.

Review the revision notes for further details of the changes in
8.00.00b6.

Beginning with this beta, distributions of the driver will only be in
the source-type tarball format.  It's not worth the extra effort of
building a separate drop-in kernel tarball, given the varying release
periods of the driver.  Similar drop-in-kernel-tarball results can be
had by:

	driver source in /tmp/qla8_b6

	# cd <kernel source>
	# patch -p1 < /tmp/qla8_b6/add_to_kernel.diff
	# mkdir drivers/scsi/qla2xxx
	# cp /tmp/qla8_b6/* drivers/scsi/qla2xxx
	# make ...

Failover functionality is present in this distribution, at this time I
do not foresee a policy change regarding its presence in the 8.x
series driver.  The next beta release of 8.x will be failover-feature
resync'd with our latest 6.x beta (6.07.xx).

Regards,
Andrew Vasquez

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Mike Anderson]

Andrew Vasquez [andrew.vasquez@qlogic.com] wrote:
> All,
> 
> A new version of the 8.x series driver for Linux 2.6.x kernels has
> been uploaded to SourceForge:

Thanks for the update.

Can you give more information on why clustering was turned off starting
in b5?

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

> 	o Slab cache allocations for driver SRBs.

this one is broken.  You kall kmem_cache_alloc and kmem_cache_free
under a spinlock which is not wrong (at least with GFP_ATOMIC) but useless.
You don't have a mempool so you will deadlock under load.
You have a kmem_cache_t per host which is unessecary, one in the driver is
enough.

> the source-type tarball format.  It's not worth the extra effort of
> building a separate drop-in kernel tarball, given the varying release
> periods of the driver.  Similar drop-in-kernel-tarball results can be
> had by:


Would be nice to unpack into a directory instead of CWD, like almost any
tarball does..

> Failover functionality is present in this distribution, at this time I
> do not foresee a policy change regarding its presence in the 8.x
> series driver.  The next beta release of 8.x will be failover-feature
> resync'd with our latest 6.x beta (6.07.xx).

So it won't get merged into 2.6.x.  Not that it would be likely given your
complete ignorance of the other TODO items.  Sometimes I really wish to
get some funding to work on a mergeable qla2xxx driver as it seems impossible
to get one from qlogic..

--
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: [ANNOUNCE] QLogic qla2xxx driver update available (v8.00.00b6).

[Christoph Hellwig]

More comments:

 - qla_vendor.c is still unused and should be killed
 - your ioctl API gets worse and worse.  You don't expect this huge
   dungpile of ioctls all marked _BAD to be merged, do you?
   Also having different ioctl values for different plattforms is
   not an option for Linux.