Re: [CFT] more kdev_t-ectomy

Re: [CFT] more kdev_t-ectomy

[Andries.Brouwer]

>>> We do need a dev32_t for NFSv2 et al, though.

>>  don't know why.

> So we can have (k)dev_t_to_dev32() for NFSv2 et al

That is something private to the NFS code.
The standard leaves the structure undefined, so whatever we do is OK.
It seems reasonable to allow some mount option to specify
whether the other side is Solaris-like, with 8:8 / 14:18,
or FreeBSD-like, with 8:24, or Linux like ...

Filesystems that are offered more dev_t bits than they can handle
must just return -EOVERFLOW.

Andries

Re: [CFT] more kdev_t-ectomy

[H. Peter Anvin]

Andries.Brouwer@cwi.nl wrote:
> 
> That is something private to the NFS code.
> The standard leaves the structure undefined, so whatever we do is OK.
> It seems reasonable to allow some mount option to specify
> whether the other side is Solaris-like, with 8:8 / 14:18,
> or FreeBSD-like, with 8:24, or Linux like ...
> 
> Filesystems that are offered more dev_t bits than they can handle
> must just return -EOVERFLOW.
> 

Yes.  It seems useful to centralize this, though.

	-hpa

Re: [CFT] more kdev_t-ectomy

[Andries.Brouwer]

    > Of course it may be possible to avoid kernel-internal numbers altogether.
    > Sometimes that is an improvement, sometimes not. Pointers are more
    > complicated than numbers - they point at something that must be allocated
    > and freed and reference counted. A number is like a pointer without the
    > reference counting.

    I guess the question is: is there any point to have three forms --
    with necessary conversions between them -- or is it simpler to have
    two forms and just use the more awkward dev_t form everywhere?

It doesnt matter much. I would not have introduced kdev_t just
for slightly more efficient dev_t handling. But we have it already.
It seems meaningless to go and replace it by something more awkward
and less efficient.

[But should anyone want: globally s/kdev_t/dev_t/ and a small edit
of kdev_t.h suffices.]

    We do need a dev32_t for NFSv2 et al, though.

I don't know why.

Andries

Re: [CFT] more kdev_t-ectomy

[H. Peter Anvin]

Andries.Brouwer@cwi.nl wrote:
> 
> It doesnt matter much. I would not have introduced kdev_t just
> for slightly more efficient dev_t handling. But we have it already.
> It seems meaningless to go and replace it by something more awkward
> and less efficient.
> 

It isn't, though, if it makes things cleaner and avoids double conversions.

> [But should anyone want: globally s/kdev_t/dev_t/ and a small edit
> of kdev_t.h suffices.]
> 
>     We do need a dev32_t for NFSv2 et al, though.
> 
> I don't know why.

So we can have (k)dev_t_to_dev32() for NFSv2 et al, which only allows 
for 32 bits in device numbers.  This is 12:20 <-> 32:32 conversions.

	-hpa

Re: [CFT] more kdev_t-ectomy

[viro]

On Mon, Apr 21, 2003 at 01:56:34AM +0200, Andries.Brouwer@cwi.nl wrote:
 
> [But should anyone want: globally s/kdev_t/dev_t/ and a small edit
> of kdev_t.h suffices.]

Please, don't.  Right now kdev_t instances serve as a useful indicator:
"it's still a mess, FIXME".  There are valid uses of dev_t and global
s/kdev_t/dev_t/ would achieve only one thing - mix them with junk.

Re: [CFT] more kdev_t-ectomy

[Andries.Brouwer]

> MKDEV(<constant>,<constant>) is a valid thing, as far as I'm concerned.

Yes. I was tempted to change the first argument of blk_register_region
into a pair, killing some MKDEV occurrences, but then I noticed that
almost all are of the form MKDEV(<constant>,<constant>), and that
is not so bad.

Still, the fact that every single call of blk_register_region
has a first argument MKDEV(ma,mi) suggests that one might
consider leaving these parameters separate.

Andries


[Now that we are talking anyway, let me ask about something.
You wrote blk_register_region so that subregions override
superregions. At the bottom there is the full region.
Was this just a general good idea, or do you have definite
applications in mind? I ask this mostly because the hash
lookup becomes more complicated in the general case.
You may have noticed that I wrote

static inline int major_to_index(int major)
{
        return major % MAX_PROBE_HASH;
}
static inline int dev_to_index(dev_t dev)
{
        return major_to_index(MAJOR(dev));
}

and that is OK for regions with constant major.
For multimajor regions a hash does not work very well, and
a tree looks better.]

Re: [CFT] more kdev_t-ectomy

[viro]

On Sun, Apr 20, 2003 at 11:58:18PM +0200, Andries.Brouwer@cwi.nl wrote:
 
> [Now that we are talking anyway, let me ask about something.
> You wrote blk_register_region so that subregions override
> superregions. At the bottom there is the full region.
> Was this just a general good idea, or do you have definite
> applications in mind? I ask this mostly because the hash

A lot of them.  Consider, e.g., ide code.  Or any other driver with
subdrivers - anything that can't be just done by blind "here's device
number, that's enough to know what should be loaded".

In case of ide we have regions corresponding to hwifs (== IDE cables)
and each of them can carry up to two devices.  We can see that 22:43
is to be handled by IDE, no problems with that.  But then we get to
decide whether it's ide-disk or ide-cd or ide-floppy.  IDE code knows
what is needed - it had probed devices already and it knows that
master on that cable is a disk.  So attempt to open that sucker
should refer to IDE code, which, in turn, should know that we need
to load a high-level driver (ide-disk.o).  After that we have a
normal range (64 numbers) for master.  But not necessary for slave -
if it is an ide-cd, we still might have no driver loaded.

We could play splitting these ranges and merging them (i.e. original range
shrinks and subrange is added as the first-level one), but that gets very
nasty when you try to get it right (you need to merge these suckers during
cleanup, etc.)
 
> and that is OK for regions with constant major.
> For multimajor regions a hash does not work very well, and
> a tree looks better.]

Tree certainly looks better.  I'd played with route cache code (after
all, that's exactly the same problem), but it looked like an overkill.
I'm porting genhd.c code for character devices, if that will show up
in profiles we'll always be able to optimize that stuff.  It is fairly
isolated, so changes of lookup data structures should not affect anything
else.

Re: [CFT] more kdev_t-ectomy

[Andries.Brouwer]

    From: viro@parcelfarce.linux.theplanet.co.uk
     
    > So, the interface with filesystems and with userspace has dev_t.
    > For kernel-internal numbers kdev_t is better than dev_t.
    > 
    > Of course it may be possible to avoid kernel-internal numbers altogether.
    > Sometimes that is an improvement, sometimes not. Pointers are more
    > complicated than numbers - they point at something that must be allocated
    > and freed and reference counted. A number is like a pointer without the
    > reference counting.

    Sigh...   And what, pray tell, do we do with these numbers?  That's
    right, at some point(s) we use them to obtain <drumroll> pointers
    to driver-supplied objects.  And that's where the lack of refcounting,
    locking, etc. bites you.

Ha, Al - did you try to understand?

Given a number, one can copy it around freely, without the need
to obtain a reference each time one copies it.
Many of these numbers are just copied around and never used.
In such a case, refcounting is a real waste of time.
But if one really wants to use the number, then a lookup is done,
and one gets a properly refcounted pointer.

So, instead of the dogmatic "numbers are bad", one might try
to estimate the cost of always refcounting versus the cost of
sometimes doing a lookup.

    Let's sort that mess out for good.  Papering over this stuff won't do
    us any good and will only bring more kludgy interfaces.  $DEITY witness,
    we already have enough of those.

[this sounds like the canonical content-free diatribe]

    It's not about pointers vs. numbers - ...

[stuff I completely agree with deleted]

    By now all uses of mk_kdev()/major()/minor()/MAJOR()/MINOR() in the drivers
    are either trivially removable or represent very real problems.  And it's
    not that there was a lot of them - in my current tree there's ~85 instances
    of kdev_t in the source.  And only one of them (->i_rdev) is widely used -
    ~500 instances, most of them go away as soon as CIDR patch gets merged.
    The rest is part noise, part real bugs that need to be fixed anyway
    (~40--80 of those).

Yes, I tend to agree. Funny that you do not mention MKDEV - that was
the thing I worked on eliminating long ago.


Andries

Re: [CFT] more kdev_t-ectomy

[viro]

On Sun, Apr 20, 2003 at 11:17:17PM +0200, Andries.Brouwer@cwi.nl wrote:
 
> Given a number, one can copy it around freely, without the need
> to obtain a reference each time one copies it.
> Many of these numbers are just copied around and never used.
> In such a case, refcounting is a real waste of time.

Excuse me, but "copied around and never used" is by definition a real
waste of time.  And proper fix is neither "keep it" nor "replace with
pointer".  It's "kill it".

>     By now all uses of mk_kdev()/major()/minor()/MAJOR()/MINOR() in the drivers
>     are either trivially removable or represent very real problems.  And it's
>     not that there was a lot of them - in my current tree there's ~85 instances
>     of kdev_t in the source.  And only one of them (->i_rdev) is widely used -
>     ~500 instances, most of them go away as soon as CIDR patch gets merged.
>     The rest is part noise, part real bugs that need to be fixed anyway
>     (~40--80 of those).
> 
> Yes, I tend to agree. Funny that you do not mention MKDEV - that was
> the thing I worked on eliminating long ago.

No, I do not mention it.  And for a good reason.  It's the only constructor
for constant values of dev_t.  You could keep every such value in two
fields, but then you get all their uses go in pairs and starting with
MKDEV().  Or have lookup code play with MAJOR()/MINOR() for no good
reason whatsoever.

MKDEV(<constant>,<constant>) is a valid thing, as far as I'm concerned.

[CFT] more kdev_t-ectomy

[viro]

	Patchset is on ftp.linux.org.uk/pub/people/viro/T*
So far it kills kdev_t crap in tty and console layers; more to follow.
The goal is to kill kdev_t completely and switch the character device
side of things to real objects.  Fortunately it's nowhere near the
nastiness of corresponding block device work.

	Please, test.  Patches are against 2.5.68 and It Works Here(tm).
If there will be no screams, it goes to Linus.

	Summary of patches follows:

1)	tty.driver turned into a pointer
	* we never modify any fields of tty.driver
	* we initialize it with a copy of registered struct tty_driver
	* we never use any fields that might be modified in original
	* we pin the original down until we free tty
ergo, we can store a reference instead of copying the damn thing.  Aside
of shrinking tty_struct, it allows for much saner refcounting and a bunch
of cleanups later.

2)	/proc/tty/drivers converted to seq_file

3)	removed fake drivers
	instead of registering "drivers" for /dev/tty, /dev/vc/0, /dev/ptmx
and /dev/console (they are never looked up since tty_open() special-cases
them and they should not be looked up - these devices are remapped on open)
we register corresponding chrdev ranges and devfs nodes directly.
	/proc/tty/drivers code updated to keep the contents unchanged

4)	tty->tty_name added
	new field; initialized to <driver->name><tty index+driver->base_name>
when we allocate tty_struct.  Drivers code switched to use of that beast (in
debugging printks, mostly).

5)	tty->tty_index added
	tty->tty_index added; we initialize it with minor(tty->device) -
tty->driver->minor_start.  Majority of remaining tty->device uses had
that form and are switched to use of tty->index.

6)	driver->driver_name cleanup
	sanitized driver->driver_name initialization and use

7)	misc tty cleanups
	* generic_serial.c typo fix (->driver used instead of correct
->driver_data)
	* tubio cleaned up

8)	rio cleanups and fixes
	* drivers/char/rio/* supports up to 4 boards, each with up to 128
lines.  It used to share termios for 1st/3rd and 2nd/4th boards,  Fixed.
	* cleanups and kdev_t removals - we pass tty instead of tty->device
in a couple of helper functions and instead of comparisons on major(tty->device)
we check where does tty->driver point to.

9)	tty_io.c::init_dev() change
	Preparations to cleanup:
	* call of get_tty_driver() moved from init_dev() to its callers
	* instead of kdev_t dev we pass struct tty_struct *driver and int index

10)	tty->device switched to dev_t
	There are very few uses of tty->device left by now; most of
them actually want dev_t (process accounting, proc/<pid>/stat, several
ioctls, slip.c logics, etc.) and the rest will go away shortly.

11)	Big ranges for tty_driver
	* we allow tty_driver to cover more than 256 devices
	* pty.c cleaned up - now we only one driver for UNIX98 masters and
only one driver for UNIX98 slaves, so a lot of ugliness can be killed.
	* get_tty_driver() became an analog of get_gendisk() - it does
a lookup by device number and gives (pointer to tty_driver,index).
	* registration/unregistration of tty_driver updated
	* /proc/tty/drivers code updated (now one structure can be responsible
for several lines)

The next two patches deal with interaction between tty and console.

12)	uart_console_device() helper
	console drivers that come from drivers/serial switched to common
->device() method.

13)	console->device() change
	instead of returning kdev_t console->device() returns tty_driver and
index.  Caller updated.

14)	fbdev->node kdev_t removal
	All places that use fbdev->node are interested only in one thing -
number of framebuffer (i.e. minor(fbdev->node)).  The place setting that
beast finds an unused number and sets ->node to mk_kdev(FB_MAJOR, number).
Obvious cleanup: stop messing with kdev_t and just store what we really
want to store - an integer.

15)	ppc boot partition cleanup
	gratitious uses of kdev_t when we are actually dealing with
device numbers (as visible by userland) removed.

Re: [CFT] more kdev_t-ectomy

[Muli Ben-Yehuda]

[-- Attachment #1: Type: text/plain, Size: 626 bytes --]

On Sun, Apr 20, 2003 at 02:31:43PM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> 	Patchset is on ftp.linux.org.uk/pub/people/viro/T*
> So far it kills kdev_t crap in tty and console layers; more to follow.
> The goal is to kill kdev_t completely and switch the character device
> side of things to real objects.  Fortunately it's nowhere near the
> nastiness of corresponding block device work.

Applied cleanly to 2.5.68-cvs, compiled and booted fine. Running it
now on IBM Thinkpad R-30, debian unstable. X works fine, emacs works,
anything special to test? 
-- 
Muli Ben-Yehuda
http://www.mulix.org


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [CFT] more kdev_t-ectomy

[Andries Brouwer]

On Sun, Apr 20, 2003, viro@parcelfarce.linux.theplanet.co.uk
wrote:

[lots of useful stuff].

Happy Easter!

Concerning kdev_t vs dev_t, probably I said this before, but just to be sure:

Long ago the purpose of kdev_t was to become a pointer. Roughly speaking
we got that pointer, only it is called struct gendisk * today.

Today the purpose of kdev_t is to be a form of dev_t: taking the minor
of a kdev_t is just taking the lower 32 bits, no tests, no branches;
taking the minor of a dev_t requires tests and branches - more code,
slower code.

So, the interface with filesystems and with userspace has dev_t.
For kernel-internal numbers kdev_t is better than dev_t.

Of course it may be possible to avoid kernel-internal numbers altogether.
Sometimes that is an improvement, sometimes not. Pointers are more
complicated than numbers - they point at something that must be allocated
and freed and reference counted. A number is like a pointer without the
reference counting.

Andries

Re: [CFT] more kdev_t-ectomy

[viro]

[cc fixed - my apologies]

On Sun, Apr 20, 2003 at 06:00:34PM +0200, Andries Brouwer wrote:
 
> So, the interface with filesystems and with userspace has dev_t.
> For kernel-internal numbers kdev_t is better than dev_t.
> 
> Of course it may be possible to avoid kernel-internal numbers altogether.
> Sometimes that is an improvement, sometimes not. Pointers are more
> complicated than numbers - they point at something that must be allocated
> and freed and reference counted. A number is like a pointer without the
> reference counting.

Sigh...   And what, pray tell, do we do with these numbers?  That's
right, at some point(s) we use them to obtain <drumroll> pointers
to driver-supplied objects.  And that's where the lack of refcounting,
locking, etc. bites you.

Let's sort that mess out for good.  Papering over this stuff won't do
us any good and will only bring more kludgy interfaces.  $DEITY witness,
we already have enough of those.

It's not about pointers vs. numbers - we certainly have enough cases when
we really deal with the latter.  However, I'd rather have clear separation
between "32bit value presented to/by userland to identify device node"
and "value in range 0--7, representing the number of channel on a multiport
card FooLink-8X".  The latter makes perfect sense for a driver.  As does
"structure that represents given instance of FooLink-8X".  The former belongs
to interaction with userland and using it outside of that context is a kludge.
Dangerous kludge in case if it masks the aforementioned complications.
It also breeds all sorts of ugliness in the code - see the crap around
reassigning ->f_op and problems with clean implementation of revoke(2)
analogs for instance.  Or a buttload of fun induced by (completely
artificial) separation into major and minor - see the mess around UNIX98
ptys implementation, etc.

By now all uses of mk_kdev()/major()/minor()/MAJOR()/MINOR() in the drivers
are either trivially removable or represent very real problems.  And it's
not that there was a lot of them - in my current tree there's ~85 instances
of kdev_t in the source.  And only one of them (->i_rdev) is widely used -
~500 instances, most of them go away as soon as CIDR patch gets merged.
The rest is part noise, part real bugs that need to be fixed anyway (~40--80
of those).

Re: [CFT] more kdev_t-ectomy

[H. Peter Anvin]

Followup to:  <20030420160034.GA20123@win.tue.nl>
By author:    Andries Brouwer <aebr@win.tue.nl>
In newsgroup: linux.dev.kernel
> 
> Of course it may be possible to avoid kernel-internal numbers altogether.
> Sometimes that is an improvement, sometimes not. Pointers are more
> complicated than numbers - they point at something that must be allocated
> and freed and reference counted. A number is like a pointer without the
> reference counting.
> 

I guess the question is: is there any point to have three forms --
with necessary conversions between them -- or is it simpler to have
two forms and just use the more awkward dev_t form everywhere?  The
only use for dev_t's in the kernel should be getting them from or
shipping them off to userspace at some point, so it might be just as
easily to do the conversion directly -- macroized, of course.

We do need a dev32_t for NFSv2 et al, though.

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
Architectures needed: ia64 m68k mips64 ppc ppc64 s390 s390x sh v850 x86-64