Re: [PATCH v6 0/4] scsi: ufs & ums-* & esp_scsi: fix module reference counting

James Bottomley
2015-05-05T18:05:26Z

On Tue, 2015-05-05 at 10:25 -0400, Alan Stern wrote:
> On Mon, 4 May 2015, James Bottomley wrote:
>
> > On Mon, 2015-05-04 at 16:09 -0400, Alan Stern wrote:
> > > On Mon, 4 May 2015, James Bottomley wrote:
> > >
> > > > However, it does also strike me that these three drivers have problems
> > > > because they're using the wrong initialisation pattern: the template is
> > > > supposed to be in the bus connector for compound drivers not in the
> > > > core.
> > >
> > > Why is it supposed to be done that way? Isn't that less efficient? It
> > > means you have to have a separate copy of the template for each bus
> > > connector driver, instead of letting them all share a common template
> > > in the core.
> >
> > Well, no it doesn't. The way 53c700 implements it is that there is a
> > common template in the core. The drivers just initialise their variant
> > fields (for 53c700 it's name, proc_name and this_id) and the core fills
> > in the rest. Admittedly wd33c93 doesn't quite get this right, that's
> > why I cited 53c700.
>
> You seem to be agreeing with me, even though you think you are
> disagreeing.
>
> "... there is a common template in the core." -- that's one
> scsi_host_template structure.
>
> "The drivers just initialize their variant fields ... and the
> core fills in the rest." -- that's an additional
> scsi_host_template structure for each driver.
>
> The total comes to N+1 scsi_host_templates, where N is the number of
> drivers.
>
> Now consider the way usb-storage does things.
>
> There is a common template in the core. That's one
> scsi_host_template structure.
>
> The drivers use the core's template. They have _no_ variant
> fields other than .owner. That's why I thought Akinobu's idea
> of putting the owner field in the Scsi_Host structure was a
> good idea.
>
> The total comes to 1 scsi_host_template. Is that not better than N+1?
>
> Consider the patch Akinobu just posted. In addition to a whole bunch
> of new code, it adds this:
>
> > --- a/drivers/usb/storage/alauda.c
> > +++ b/drivers/usb/storage/alauda.c
> ...
> > @@ -1232,6 +1233,8 @@ static int alauda_transport(struct scsi_cmnd *srb, struct us_data *us)
> > return USB_STOR_TRANSPORT_FAILED;
> > }
> >
> > +static struct scsi_host_template alauda_host_template;
>
> alauda.c was the only driver modified in that patch, and it gained an
> new scsi_host_template.
>
> For the case where the variants are identical in all respects other
> than .owner, doesn't it make sense to allow them to share a single
> scsi_host_template?
>
> The original design of the SCSI stack envisioned multiple drivers, each
> in control of multiple SCSI hosts. The idea was that
> scsi_host_template would be associated with the driver and Scsi_Host
> with the individual host.
>
> Now the kernel has evolved, and we have multiple drivers, some of which
> contain multiple subdrivers, each in control of multiple SCSI hosts.
> In this situation we should be flexible enough to allow the
> scsi_host_template to be associated with either the driver or the
> subdriver (decision to be made by the driver). When the only variant
> field is .owner, it makes sense to associate the scsi_host_template
> with the driver, not force it to be associated with the subdriver.
>
> The cost is duplication of the .owner field in every Scsi_Host. The
> savings is a reduction in the number of scsi_host_templates.

So your essential objection is the host template duplication? I know
it's a couple of hundred bytes, but surely its dwarfed by all the other
stuff you have to duplicate ... the module size of each of these is
around 0.25MB, so a couple of hundred bytes would seem a bit
insignificant.

James