I believe that the following little change will enable
elimination of a kmalloc/kfree pair from many device drivers,
and, more importantly, eliminate the rarely tested often buggy
error leg for dealing with that memory allocation failure.
This patch allows device drivers to tell the generic device
code to handle allocating the per-device blob of memory that is
normally stored in device.driver_data. It does so by adding a new
field device_driver.drvdata_size, with the following semantics:
0 - Do not attempt to allocate or free device.driver_data.
This is compatible with previous behavior (although we
now initialize driver_data to NULL before calling the
probe routine).
>0 - Allocate the specified number of bytes, fill it with
all zeroes, and store the address in device.driver_data
before calling the device's probe routine. Abort the
probe with -ENOMEM if memory allocation fails. Free the
storage if the probe fails or when the driver is
removed. (The allocation and freeing mechanisms are
not specified, although the current mechanism uses
kmalloc/kfree).
-1 - Set device.driver_data to NULL before calling the probe
routine. When the probe routine returns failure and
when the driver is removed, check the value of
device.driver_data. If it is non-NULL, kfree it.
This is handy for drivers that want to do their own
memory allocation. This is handy if the driver needs
a variable sized block or really really really does not
want the allocated memory initialized to all zeroes.
You may wonder about the trade-off of initializing the
allocated memory to all zeroes (in the drvdata_size > 0 case). Driver
probes are executed relatively rarely (as opposed to an IO path that
might get executed a thousand times per second) and it is hard to
identify bugs due to uninitialized values. The structures that the
probles fill in often gain new parameters over time, so it is easy to
make initialization bugs that gcc cannot easily detect. It also makes
it easier to design new fields in these structures where a value of
zero will provide backward compatible behavior. A small bonus is that
driver initialization code only needs to fill in non-zero values
explicitly.
If there is a case where the performance hit from clearing the
memory is sustantial, you could use drvdata_size = -1. If it turns
out that the situation is common, we could have a flag to skip the
clearing of the memory, but I don't think that such cases will be
common.
I realize that many drivers store the result of some high
level allocate routine in their private data (for example,
scsi_register). Later, I intend to make versions of those routines
that take pointer to a block of zero-filled memory rather than calling
kmalloc themselves.
Anyhow, here is the patch. I would like to start writing
driver clean-ups that use this patch soon, so I would like to see this
addition integrated into the kernel as sson as possible. I am running
a kernel with this patch compiled in now, although I have not yet
changed any drivers to take advantage of it.
Pat, are you the person I should be submitting this patch
to? Is there someone else I should be submitting this patch to?
Please let me know. Thanks in advance.
--
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
On Sat, Nov 02, 2002 at 11:29:51AM -0800, Adam J. Richter wrote:
>
> This patch allows device drivers to tell the generic device
> code to handle allocating the per-device blob of memory that is
> normally stored in device.driver_data. It does so by adding a new
> field device_driver.drvdata_size, with the following semantics:
Ugh, have you tried to use this patch in real life with the busses that
use driver_data today? I didn't think so :)
In short, only the driver's individual probe function knows how big of a
data chunk it needs, and that probe function isn't known until probe()
is called. Hm, well ok, match() could set this, but then it would have
to call into the function found by match to get the size. By then it's
just really not worth adding this extra complexity to the code.
So in short, I don't like this, and don't really see where it buys you
anything.
But I did like your pci private data cleanup patch from the other day,
mind if I add that into the next round of pci cleanups I'm going to be
sending to Linus?
thanks,
greg k-h
>But I did like your pci private data cleanup patch from the other day,
>mind if I add that into the next round of pci cleanups I'm going to be
>sending to Linus?
By all means, please integrate it and get it into Linus's tree
if you can. Thanks for your assistance.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
On Sat, Nov 02, 2002 at 12:42:58PM -0800, Greg KH wrote:
> On Sat, Nov 02, 2002 at 11:29:51AM -0800, Adam J. Richter wrote:
> >
> > This patch allows device drivers to tell the generic device
> > code to handle allocating the per-device blob of memory that is
> > normally stored in device.driver_data. It does so by adding a new
> > field device_driver.drvdata_size, with the following semantics:
>
> Ugh, have you tried to use this patch in real life with the busses that
> use driver_data today? I didn't think so :)
>
> In short, only the driver's individual probe function knows how big of a
> data chunk it needs, and that probe function isn't known until probe()
> is called. Hm, well ok, match() could set this, but then it would have
> to call into the function found by match to get the size. By then it's
> just really not worth adding this extra complexity to the code.
Although I had not converted any drivers when I wrote my
previous message, I am now writing this message on a machine using a
via-rhine ethernet driver that uses this facility. The driver is
three lines smaller and, more importantly, has one less probably
untested error branch. I think this example should address you
concerns.
Note that because struct net_device may need to persist after
->remove() returns, I had to make a net_device.destructor function
(sharable with all similar network device drivers) rather than allow
drivers/base/bus.c to do the kfree. If the same turns out to be true
for Scsi_Host, gendisk, etc., then I will scrap the kfree part of my
proposal.
Drivers that really want to do a single variable-length
kmalloc for their private data will not use this facility, or will use
drvpriv_size = -1 if they want to just eliminate the kfree. I think
those drivers will be rare if the allocators for the underlying
software abstractions (Scsi_Host, net_device, gendisk, etc.) are made
available that can just take a pointer to zero-filled memory rather
than doing the kmalloc themselves.
This change may seem like small fry, but it's important to
understand that it will potentially eliminate a lot of untested error
branches and also that I plan some similar optional consolidation of
other driver resource allocations, especially those that can return
failure (DMA consistent memory, streaming DMA mappings, request_region
in ISA drivers, etc., but I'd like to do this incrementally).
Ultimately, this should produce smaller and more reliable drivers.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
On Sun, 2002-11-03 at 07:45, Adam J. Richter wrote:
> Note that because struct net_device may need to persist after
> ->remove() returns, I had to make a net_device.destructor function
> (sharable with all similar network device drivers) rather than allow
> drivers/base/bus.c to do the kfree. If the same turns out to be true
> for Scsi_Host, gendisk, etc., then I will scrap the kfree part of my
> proposal.
It is certainly true for scsi devices, it will be true (once I get
around to it) for IDE devices
Here is a new version of my patch that also allows drivers to
ask the generic driver code to allocate a fixed-size per-device blob of
consistent DMA memory. It includes an example patch to via-rhine.c,
removing another 14 lines, including another memory allocation failure
error branch. I am running the resulting via-rhine driver now.
If I can eliminate an average of 5 line from each of the 557
modules in my modules.pcimap (never mind other busses for the moment),
that would be over 2000 lines of code. More importantly, I'm sure it
would eliminate a lot of bugs in untested error branches.
Dave M.: I am cc'ing this to because you asked for a
bus-independent API for allocating DMA consistent memory when I
discusssed consolidating some other driver DMA mapping operations
about six months ago. This patch adds the wrapper functions
dma_{alloc,free}_consistent() for this purpose which are not optimized
right now, but provide an API and could be expanded to support more
the the DMA allocation API as it is actually used. The implementation
can be optimized later, but I think it addresses your concern about
propagating artificial PCI dependence.
--
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
From: "Adam J. Richter" <[email protected]>
Date: Sun, 3 Nov 2002 14:40:22 -0800
Dave M.: I am cc'ing this to because you asked for a
bus-independent API for allocating DMA consistent memory when I
discusssed consolidating some other driver DMA mapping operations
about six months ago.
I don't know how much I like the DMA memory being allocated
transparently based upon some structure initialization values.
I'd rather the DMA alloc/free be explicit in the drivers.
Otherwise, the ->ops->alloc_consistent et al. abstraction
looks ok.
Dave Miller writes:
>I don't know how much I like the DMA memory being allocated
>transparently based upon some structure initialization values.
>
>I'd rather the DMA alloc/free be explicit in the drivers.
I think I roughly understand your attitude. Certainly I am of
wary adding more abstraction. Programmers tend to get lost in it. In
spite of the best intentions, code is often made less readable and
maintainable, bugs less apparent.
However, at some point, abstraction can be worth it. The
resulting code actually shirnking, accelerating, being clearer,
when the abstraction results in the removal of bugs, are all
metrics that I would look at.
Lifting most of the memory allocation and DMA mapping out of
the drivers will remove thousands of lines from Linux drivers in
aggregate and remove hundreds of potentially buggy error branches. It
will be a little easier see the hardware from reading the driver.
In comparison, the CPU costs are small and may be negative if
some of that consolidation allows for additional optimizations.
I'd be interested in knowing how you quantify this trade-off
and what you think might persuade you to support or at least be
neutral toward this type of facility (results of converting drivers,
examples of buggy error branches?). Please keep in mind that not all
drivers necessarily need to use these facilities.
>Otherwise, the ->ops->alloc_consistent et al. abstraction
>looks ok.
Thanks.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."