Messages in this thread | | | Date | Sun, 05 Dec 1999 14:48:55 +0100 | From | Martin Dalecki <> | Subject | Re: 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] | |