lkml.org 
[lkml]   [1999]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: PATCH for 2.3.29: block device setup cleanup.
Andries.Brouwer@cwi.nl wrote:
>
> > I hope you understand my intentions a bit better now?
>
> Yes, I see what you want.
> But I am unhappy with your work. Look again at
>
> > - read_ahead[MAJOR(dev)] = arg;
> > + blk_dev[MAJOR(dev)].read_ahead = arg;
>
> If the final goal is
>
> dev->majordev->read_ahead = arg;
>
> or so, then it seems to me you do not make progress.
> On the other hand, if you do
>
> - read_ahead[MAJOR(dev)] = arg;
> + READ_AHEAD(dev) = arg;
>
> then you can have today
>
> #define READ_AHEAD(x) read_ahead[MAJOR(x)]
>
> and tomorrow
>
> #define READ_AHEAD(x) (x)->majordev->read_ahead
>
> (And this may be further hidden inside functions get_read_ahead()
> and set_read_ahead(), so that generic versions of such functions
> can be replaced by more interesting ones for certain devices, etc.)
>
> Such changes are real progress, because (i) they generate the same
> code as we have today, so no errors can be introduced, not even
> any inefficiencies, and (ii) using a 1-line change this READ_AHEAD()
> suddenly gets its new meaning.
>
> It is this same mechanism that allows one today to switch between
>
> #define MINOR(x) ((x) & MINORMASK)
>
> and
>
> #define MINOR(x) ((x)->minor)
>
> In other words, your current approach requires changes all over
> the place to reach an intermediate stage, and then again changes
> all over for the final work. While the version I would suggest
> requires a change only once, and people running the kernel with
> one set of defines can coexist with people running the kernel
> with another set of defines.
>
> This allows one to play with several possible ways to setup
> device structs for free, without having to do major editing.

You miss several points:

1. One can't get over without any intermediate state just simple for
the sake of code testing and overall stability.

2. What you are proposing is just a way to make the linux source base
contain different operating systems.

3. I don't actually like the above overencapsulation of the interface,
since
this does hide the actuall semantics from the "user", read driver
developer.

4. Making new features an "option" is the best way to not encourage
anybody
to switch to a new interface.

5. It's not this level of the interface which is going to change.
Wherever possible I'm going to pass the pointer to the blk_dev_struct
instead of kdev_t to functions which don't really need the full
device
information. Ideally I'm going to have just a lookup and allocation
of a blk_dev_struct on kernel entry based on the kdev_t number passed
from the user space. Thereafter only a pointer to this struct should
revolve
around inside the kernel. This way one will not have to do the double
derefference you get by just changing the content
of the kdev_t. Look at this at cutting the tree from the bottom
(blk_dev)
instead of starting from the top piece by piece (changing kdev_t in
first place.)

6. At least you can use my patches as a documentation about what and
where needs to
get changed.

BTW. If you have doubts whatever there isn't any simplification, then
please have a look at the attached newer patch version.
There I had been able, by carfully analyzing the actual usage and
supposed semantics,
to eliminate tons of per minor arrays in blk_dev_struct, which where
previously
there and which where used only as arrays of equal per-major constants.
This ammounts for significan conde simiplification (one doesn't need to
administer dynamically allocated arrays, and yes there where leaks
previously there)
as well as a quite significant dynamic space usage lowerage. As well as
of course
less confusion inside the drivers about what those values should
contain.
(It took quite time for me before I guessed correctly the semantics of
blk_size and
blksize_size, in esp. the second is a wonderfull name.)

As of the space saving lets see. I have 5 disks in my homebox. And about
6 different
partitions on them (some are mounted only on demand).
So 6*4*3*256 = 18k bytes. OK this isint going to make anybody jail, but
it's not
an negligable amount ;-).

> And Linus might accept such a change even during code-freeze
> times since the generated code remains the same.
>
> Andries

I doubt this stuff will be finished soon. However the attached patch
could
very well just get in as it is.

The next thing I'm going to do is to really look where one could pass
blk_dev_struct pointer
instead of kdev_t on the hardware related part (mainly ll_rw_block).

--
Marcin Dalecki[unhandled content-type:application/x-gzip]
\
 
 \ /
  Last update: 2005-03-22 13:55    [W:0.103 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site