Re: [PATCH v3] libsas: move ATA bits into a separate module

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Darrick J. Wong]

Jeff Garzik wrote:

> I disagree completely with this approach.
> 
> You don't need a table of hooks for the case where libata is disabled in
> .config.  Thus, it's only useful for the case where libsas is loaded as
> a module, but libata is not.

Indeed, I misunderstood what James Bottomley wanted, so I reworked the
patch.  It has the same functionality as before, but this module uses
the module loader/symbol resolver for all the functions in libata, and
allows libsas to (optionally) call into sas_ata with weak references by
pushing a table of the necessary function pointers into libsas at
sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
unless it actually finds a SATA device.

> The libsas code should directly call libata functions.  If ATA support
> in libsas is disabled in .config, then those functions will never be
> called, thus never loaded the libata module.

I certainly can (and now do) call libata functions from sas_ata.
However, there are a few cases where libsas needs to call libata (ex.
sas_ioctl); for those cases, I think the function pointers are still
necessary because I don't want to make libsas require libata.  In any
case, if ATA support is disabled in .config, sata_is_dev always returns
0, so the dead-code eliminator should zap that out of libsas entirely.

As usual, thank you for any feedback that you have.

Link to version 3:
http://sweaglesw.net/~djwong/docs/sas-ata_3.patch

--D

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Jeff Garzik]

Darrick J. Wong wrote:
> Jeff Garzik wrote:
> 
>> I disagree completely with this approach.
>>
>> You don't need a table of hooks for the case where libata is disabled in
>> .config.  Thus, it's only useful for the case where libsas is loaded as
>> a module, but libata is not.
> 
> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
> patch.  It has the same functionality as before, but this module uses
> the module loader/symbol resolver for all the functions in libata, and
> allows libsas to (optionally) call into sas_ata with weak references by
> pushing a table of the necessary function pointers into libsas at
> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
> unless it actually finds a SATA device.
> 
>> The libsas code should directly call libata functions.  If ATA support
>> in libsas is disabled in .config, then those functions will never be
>> called, thus never loaded the libata module.
> 
> I certainly can (and now do) call libata functions from sas_ata.
> However, there are a few cases where libsas needs to call libata (ex.
> sas_ioctl); for those cases, I think the function pointers are still
> necessary because I don't want to make libsas require libata.  In any
> case, if ATA support is disabled in .config, sata_is_dev always returns
> 0, so the dead-code eliminator should zap that out of libsas entirely.

Looks MUCH better to me, and eliminates my objection to the 
libata-related hooks.

There remains the issue that I poke James about on IRC, namely that 
there is no need to emulate the SATA phy registers.  libata permits a 
driver high level access to the ATA engine without needing SATA SCRs. 
Witness all the PATA drivers, which obviously do not have SCRs at all.

	Jeff

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Christoph Hellwig]

On Thu, Sep 14, 2006 at 03:40:55PM -0700, Darrick J. Wong wrote:
> Jeff Garzik wrote:
> 
> > I disagree completely with this approach.
> > 
> > You don't need a table of hooks for the case where libata is disabled in
> > .config.  Thus, it's only useful for the case where libsas is loaded as
> > a module, but libata is not.
> 
> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
> patch.  It has the same functionality as before, but this module uses
> the module loader/symbol resolver for all the functions in libata, and
> allows libsas to (optionally) call into sas_ata with weak references by
> pushing a table of the necessary function pointers into libsas at
> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
> unless it actually finds a SATA device.

NACK again.  Week references are bad.  Please change it back to normal
hard references so that it works like everything else in the kernel.

Re: [PATCH v3] libsas: move ATA bits into a separate module

[Jeff Garzik]

Christoph Hellwig wrote:
> On Thu, Sep 14, 2006 at 03:40:55PM -0700, Darrick J. Wong wrote:
>> Jeff Garzik wrote:
>>
>>> I disagree completely with this approach.
>>>
>>> You don't need a table of hooks for the case where libata is disabled in
>>> .config.  Thus, it's only useful for the case where libsas is loaded as
>>> a module, but libata is not.
>> Indeed, I misunderstood what James Bottomley wanted, so I reworked the
>> patch.  It has the same functionality as before, but this module uses
>> the module loader/symbol resolver for all the functions in libata, and
>> allows libsas to (optionally) call into sas_ata with weak references by
>> pushing a table of the necessary function pointers into libsas at
>> sas_ata load time.  Thus, libsas doesn't need to load libata/sas_ata
>> unless it actually finds a SATA device.
> 
> NACK again.  Week references are bad.  Please change it back to normal
> hard references so that it works like everything else in the kernel.

I strongly agree.

The kernel code will bloat, and performance will suffer, if we did weak 
refs and jump tables everywhere.

I just don't see the overhead of loading libata, and not using it, as a 
huge penalty, when looking at the alternatives.

Consider the common use cases:  (a) normal distro usage, often servers 
where libata loading will be common anyway due to SATA presence on 
motherboard, and (b) embedded use, where ATA support can be .config'd 
out at compile time.

Thus, the use cases where end users really will care about libata being 
loaded, but not used, are slim to none.

	Jeff