Re: f_ops flag to speed up compatible ioctls in linux kernel

Re: f_ops flag to speed up compatible ioctls in linux kernel

[filia]

Hi! 

 Stop being arrogant.
 Can you please elaborate on how to make Linux kernel support e.g. motion 
controllers? They do not fit *any* known to me driver interface. They have 
several axes, they have about twenty parameters (float or integer), and they 
have several commands, a-la start, graceful stop, abrupt stop. Plus 
obviously diagnostics - about ten another commands with absolutely different 
parameters. And about ten motion monitoring commands. And this is one 
example I were need to program. 

 Or take any other freaky device we might got on market tomorrow. 

 As ioctl() opponent, be kind and give some advice what to do in that kind 
of situations. 

Al Viro written:
>> Hello!
>> Currently, on the x86_64 architecture, its quite tricky to make
>> a char device ioctl work for an x86 executables.
>> In particular,
>>    1. there is a requirement that ioctl number is unique -
>>       which is hard to guarantee especially for out of kernel modules
>
>Too bad. 
>
>>    2. there's a performance huge overhead for each compat call - there's
>>       a hash lookup in a global hash inside a lock_kernel -
>>       and I think compat performance *is* important. 
>> 
>> Further, adding a command to the ioctl suddenly requires changing
>> two places - registration code and ioctl itself.
>
> So don't add them.  Adding a new ioctl is *NOT* a step to be taken lightly -
> we used to be far too accepting in that area and as somebody who'd waded
> through the resulting dungpiles over the last months I can tell you that
> result is utterly revolting. 
> 
> Excuse me, but I have zero sympathy to people who complain about obstacles
> to dumping more into the same piles - it should be hard.

Arjan van de Ven written:
>> Further, adding a command to the ioctl suddenly requires changing
>> two places - registration code and ioctl itself.
>
>adding ioctls SHOULD be painful. Really painful. It's similar to adding
>syscalls; you'll have to keep compatibility basically forever so adding
>should not be an easy thing.

 --- with respect. best regards.
***    Philips @ Saarbruecken. 

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Arjan van de Ven]

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

On Wed, Sep 01, 2004 at 03:50:11AM -0600, filia@softhome.net wrote:
> Hi! 
> 
> Stop being arrogant.
> Can you please elaborate on how to make Linux kernel support e.g. motion 
> controllers? They do not fit *any* known to me driver interface. They have 
> several axes, they have about twenty parameters (float or integer), and 

parameters nicely fit in sysfs.

> they have several commands, a-la start, graceful stop, abrupt stop. Plus 
> obviously diagnostics - about ten another commands with absolutely 
> different parameters. And about ten motion monitoring commands. And this is 
> one example I were need to program. 

a write() interface doesn't work???
Hard to believe, you even call them commands.
fd = open("/dev/funkydevice");
write(fd, "start");

doesn't sound insane to me


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

Re: f_ops flag to speed up compatible ioctls in linux kernel

[filia]

Arjan van de Ven writes: 

> On Wed, Sep 01, 2004 at 03:50:11AM -0600, filia@softhome.net wrote:
>> Hi!  
>> 
>> Stop being arrogant.
>> Can you please elaborate on how to make Linux kernel support e.g. motion 
>> controllers? They do not fit *any* known to me driver interface. They have 
>> several axes, they have about twenty parameters (float or integer), and 
> 
> parameters nicely fit in sysfs. 
> 

 What about errors?
 "set di 200000" might fail for lots of reasons. 

>> they have several commands, a-la start, graceful stop, abrupt stop. Plus 
>> obviously diagnostics - about ten another commands with absolutely 
>> different parameters. And about ten motion monitoring commands. And this is 
>> one example I were need to program. 
> 
> a write() interface doesn't work???
> Hard to believe, you even call them commands.
> fd = open("/dev/funkydevice");
> write(fd, "start"); 
> 
> doesn't sound insane to me 
> 

 it doesn't, since you didn't tryed to make error handling. every thing can 
fail - this is control of mechanics - and it fails often and for a lot of 
reasons. Put here error handling (write(struct whatever) has to return 
another struct whatever2 filled with error description) and thread-safeness. 
Pluss some controllers do support multi-dimensional movements "start 
x,y,z,etc" - and you might have _several_ error structs. Atomicity is 
important for multi-dimensional moves too - move on given axes starts with 
single command. 

 hu? 

 I do not see much point in renaming ioctl() to write() all over the place - 
at least when people see ioctl() they understand that it is not standard 
functionality. write() will for sure confuse a lot of people. 

 --- with respect. best regards.
***    Philips @ Saarbruecken. 

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Ihar 'Philips' Filipau]

   Well. I given an example of device which doesn't fit into 
read()/write() interface.

   Your imagination is truely appreciated - and you are encouraged to 
try to implement it. With full error handling (every write must be 
checked)  and multithreading support (e.g. one user for movement, 
multiple for monitoring and diagnosis).

   I have tryed to some extent impelementing something like this - 
amount of code doubled for no real gain. (It was in those times when 
"ioctl()s are bad. period." topic poped up on lkml first time. long time 
ago)

   No one will ever design interface for sake of interface. what you are 
proposing - probably will look nice in academical papers, but no one 
will do it, and no one will do it in kernel space.

   And all this stuff - all this funcy xml-like comlications? - for 
what? just call one function of driver with single parameter - void*.
   _*Not more*_. Ridiculous indeed.

P.S. As I have said before, it seems to me that using read()/write() 
instead of ioctl() could be the only choice. I once hacked read(): user 
must pass two structures: first is input, second is output. It worked - 
but no one (including me) liked it. ioctl() even by its name fit better.

P.P.S. Hm. Why not implement ioctl2()? which will be linked directly to 
device by its driver? - numbering will be internal to driver, and 
provide entry point into driver for user space applications. No one 
likes mess with ioctl()s in Linux - no device driver developers, nor 
users. But what is really needed - is just call into driver. Paramenter 
- single pointer have being proved to be sufficient.

Helge Hafting wrote:

> filia@softhome.net wrote:
> 
>> Arjan van de Ven writes:
>>
>>> On Wed, Sep 01, 2004 at 03:50:11AM -0600, filia@softhome.net wrote:
>>>
>>>> Hi! Stop being arrogant.
>>>> Can you please elaborate on how to make Linux kernel support e.g. 
>>>> motion controllers? They do not fit *any* known to me driver 
>>>> interface. They have several axes, they have about twenty parameters 
>>>> (float or integer), and 
>>>
>>>
>>>
>>> parameters nicely fit in sysfs.
>>
>>
>>
>> What about errors?
>> "set di 200000" might fail for lots of reasons.
>>
>>>> they have several commands, a-la start, graceful stop, abrupt stop. 
>>>> Plus obviously diagnostics - about ten another commands with 
>>>> absolutely different parameters. And about ten motion monitoring 
>>>> commands. And this is one example I were need to program. 
>>>
>>>
>>>
>>> a write() interface doesn't work???
>>> Hard to believe, you even call them commands.
>>> fd = open("/dev/funkydevice");
>>> write(fd, "start");
>>> doesn't sound insane to me
>>
>>
>>
>> it doesn't, since you didn't tryed to make error handling. every thing 
>> can fail - this is control of mechanics - and it fails often and for a 
>> lot of reasons. Put here error handling (write(struct whatever)
> 
> 
> Well, how about a read-write interface?
> Write a command to the driver, read back the response. If the response 
> indicate
> an error, read more to get error details.  Keep reading out possibly 
> several errors
> until the driver have nothing more to report.
> 
>> has to return another struct whatever2 filled with error description) 
>> and thread-safeness. Pluss some controllers do support 
>> multi-dimensional movements "start x,y,z,etc" - and you might have 
>> _several_ error structs. Atomicity is important for multi-dimensional 
>> moves too - move on given axes
> 
> 
> The driver can handle atomicity - if it somehow receives a partial 
> command it should
> buffer it and wait for the rest.  If you need several commands for a 
> single movement,
> consider barriers, i.e.:
> write(fd, "barrier start");
> write(fd, "one kind of movement");
> write(fd, "other kind of movement");
> write(fd, "third kind of movement");
> write(fd, "barrier end");  /* Complex movement starts at this point */
> read(fd, &error_buffer, size); /* Did this work? */
> 
>> starts with single command.
>> hu?
>> I do not see much point in renaming ioctl() to write() all over the 
>> place - at least when people see ioctl() they understand that it is 
>> not standard functionality. write() will for sure confuse a lot of 
>> people. 
> 
> 
> Isn't motion "standard functionality" for this device?  If so, a write 
> interface fits
> even if the stuff written to it might be special (structs and such)
> 
> Helge Hafting
> 

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Olivier Galibert]

On Wed, Sep 01, 2004 at 04:16:59AM -0600, filia@softhome.net wrote:
> I do not see much point in renaming ioctl() to write() all over the place - 
> at least when people see ioctl() they understand that it is not standard 
> functionality. write() will for sure confuse a lot of people. 

Because you can encapsulate write while you can't encapsulate ioctl
for a start.  And because ascii is good and opaque binary is bad.  You
don't change the target position of your motion device millions of
time per second.  You can easily send your commands and replies
(you're allowed read, too) as text, and immediatly you make logging,
debugging and reproducing actions way easier.  Plus API portability to
64bits/other endian is free.

  OG.

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Ihar 'Philips' Filipau]

Ihar 'Philips' Filipau wrote:
> 
> P.P.S. Hm. Why not implement ioctl2()? which will be linked directly to 
> device by its driver? - numbering will be internal to driver, and 
> provide entry point into driver for user space applications. No one 
> likes mess with ioctl()s in Linux - no device driver developers, nor 
> users. But what is really needed - is just call into driver. Paramenter 
> - single pointer have being proved to be sufficient.
> 

   Ok. Now I recalled those mess with ioctl()s - someone have tryed to 
implement virtual methods from OO languages with file descriptors and 
miserably failed.

   I have never used ioctl()s for anything asides calling my drivers. I 
have seens people using ioctl codes for special functionality for block 
devices.

   Position "ioctl()s bad" is not related to drivers per se. People 
decided to not introduce another calls like eject_media(fd) or 
get_info(fd) - but instead implement ioctl() which will magically work 
on all fd's of block devices.
   That where mistake is. That the kind of ioctl()s, which are bad. 
ioctl() is for something what doesn't have interface. If something have 
stable interface - it must be moved over to sys/library calls.

   Instead of "painful ioctl()s" - I would rather start with solving 
this problem for standard devices: making a way to implement efficiently 
common functions for device classes. (terminals, block devices, network 
interfaces, etc). And start obsoleting/removing ioctl()s.

   I like aproach of *BSD - they routinely implement library/sys calls 
for things like that. I used if_*/getif* calls to find/operate network 
interfaces - it is much more usable & better documented, than Linux' 
bunch of magic ioctl()s (again) on _any_ network socket. Why on any? Why 
we cannot have special device to operate on list of interfaces?

   I believe people here on LKML identified problem incorrectly.

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Helge Hafting]

Ihar 'Philips' Filipau wrote:

>
>
>   Well. I given an example of device which doesn't fit into 
> read()/write() interface.
>
>   Your imagination is truely appreciated - and you are encouraged to 
> try to implement it. With full error handling (every write must be 
> checked)

Checking every write is up to the program using the device.  Not 
forgetting this is easy.
Never write directly to the device, use an interface function in your 
program that
both writes, reads status back, and returns that status.

>   and multithreading support (e.g. one user for movement, multiple for 
> monitoring and diagnosis).
>
In that case I'd use two device nodes.  One for the command/error stuff, 
and another for
monitoring.  The monitoring device could be read-only and support 
several simultaneous
openings.

>   I have tryed to some extent impelementing something like this - 
> amount of code doubled for no real gain. (It was in those times when 
> "ioctl()s are bad. period." topic poped up on lkml first time. long 
> time ago)

Of course you _can_ use ioctls.  Just do it if that's the only useable 
way.  Wether you can get code
like that merged depends on wether you can convince the right people.  
But your driver will work
fine even if you have to distribute it yourself, so it isn't that big a 
problem.

>
>   No one will ever design interface for sake of interface. what you 
> are proposing - probably will look nice in academical papers, but no 
> one will do it, and no one will do it in kernel space.
>
>   And all this stuff - all this funcy xml-like comlications? - for 
> what? just call one function of driver with single parameter - void*.
>   _*Not more*_. Ridiculous indeed.
>
I haven't suggested any xml, perhaps someone else did.  A write 
interface let you pass
almost anything into the driver.  My example used ascii messages, you 
might want to
pass structs instead.  I believe even pointers should work too.

> P.S. As I have said before, it seems to me that using read()/write() 
> instead of ioctl() could be the only choice. I once hacked read(): 
> user must pass two structures: first is input, second is output. It 
> worked - but no one (including me) liked it. ioctl() even by its name 
> fit better.

I have to agree it seems to fit.

Helge Hafting

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Hello!
It seems that you can disable MTD partitioning support 
but enable , e.g. NAND.
And, if you are upgrading from and older kernel without
the paritioning option, and do oldconfig and are not careful,
thats what you end up with.
But, lots of code is calling del_mtd_partitions now, so you get
unresolved del_mtd_partitions.

Its easy to work around this by enabling partitioning, but
should not the dependency be added in the relevant KConfig?

Cc me directly, I'm not on the list.
Thanks,
MST

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Chris> Trivial addition of allowing llseek to become transaction
    Chris> barrier increases this to ~300,000 transactions per second.
    Chris> Patch below gives you some basic idea.

Wow, great stuff.  Thanks a lot for taking the time to try this out.
Also thanks for the original suggestion -- the more I think about the
private filesystem idea the more I like it.

Do you think simple_transaction_llseek() should be merged upstream?
It would definitely make the simple_transaction stuff more useful for
my application.

Thanks,
  Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Quoting r. Roland Dreier (roland@topspin.com) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
>     Roland> Yes, this is exactly right.  One issue raised by this
>     Roland> thread is that the ioctl32 compatibility code only allows
>     Roland> one compatibility handler per ioctl number.  It seems that
>     Roland> this creates all sorts of possibilities for mayhem because
>     Roland> it makes the ioctl namespace global in scope in some
>     Roland> situations.  Does anyone have any thoughts on if/how this
>     Roland> should be addressed?
> 
>     Michael> Thats what my original patch attempts to address
>     Michael> http://www.uwsg.indiana.edu/hypermail/linux/kernel/0409.0/0025.html
>     Michael> What do you think?
> 
> That patch seems somewhat orthogonal to the issue I raised.  You're
> just fixing the problem for devices that don't use the ioctl32 compat
> layer.
> 

No I'm removing the need for ioctls to go through the compat layer.
No compat layer, no problem.

I see the compat layer as a hack to support legacy devices without touching
all of their code.
I dont see why a new driver would use the compat hacks instead of simply
building a 32/64 bit compatible interface, and legacy drivers have the
uniqueness for commands solved one way or another.

So the only interesting problem left is for new char devices.

MST

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Roland> Yes, this is exactly right.  One issue raised by this
    Roland> thread is that the ioctl32 compatibility code only allows
    Roland> one compatibility handler per ioctl number.  It seems that
    Roland> this creates all sorts of possibilities for mayhem because
    Roland> it makes the ioctl namespace global in scope in some
    Roland> situations.  Does anyone have any thoughts on if/how this
    Roland> should be addressed?

    Michael> Thats what my original patch attempts to address
    Michael> http://www.uwsg.indiana.edu/hypermail/linux/kernel/0409.0/0025.html
    Michael> What do you think?

That patch seems somewhat orthogonal to the issue I raised.  You're
just fixing the problem for devices that don't use the ioctl32 compat
layer.

 - Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Hello!
Quoting r. Roland Dreier (roland@topspin.com) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
>     Albert> Per-command parameters included?
> 
>     Albert> People really do want private syscalls. An ioctl is that,
>     Albert> in a namespace defined by the file descriptor. So ioctl()
>     Albert> provides local scope to something that would otherwise be
>     Albert> global.
> 
> Yes, this is exactly right.  One issue raised by this thread is that
> the ioctl32 compatibility code only allows one compatibility handler
> per ioctl number.  It seems that this creates all sorts of
> possibilities for mayhem because it makes the ioctl namespace global
> in scope in some situations.  Does anyone have any thoughts on if/how
> this should be addressed.
> 

Thats what my original patch attempts to address
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0409.0/0025.html
What do you think?

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Michael> Now that I look at the ioctl.c code, I see a several
    Michael> get_user/put_user inside the ioctl which are thus done
    Michael> while BKL is held.  But I thought get_user can block?

    Michael> Why is this not a bug?

It's fine to sleep while holding the BKL (it will be dropped and then
reacquired when the process wakes up).

 - Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Chris Wright]

* Michael S. Tsirkin (mst@mellanox.co.il) wrote:
> Now that I look at the ioctl.c code, I see a several get_user/put_user
> inside the ioctl which are thus done while BKL is held.
> But I thought get_user can block?
> 
> Why is this not a bug?

Look at release_kernel_lock, esp. in kernel/sched.c.
-chris

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Lee Revell]

On Wed, 2004-09-01 at 17:23, Michael S. Tsirkin wrote:
> Hello!
> Quoting r. Albert Cahalan (albert@users.sourceforge.net) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
> > Michael S. Tsirkin writes:
> > > Quoting Lee Revell [snip -- that was excessive]
> > 
> > >> By adding a new ioctl you are adding a new use of
> > >> the BKL. It has been suggested on dri-devel that
> > >> this should be fixed.  Is this even possible?
> > >
> > > I dont know - can the lock be released before the
> > > call to filp->f_op->ioctl ?
> > >
> > > I assume the reason its there is for legacy
> > > code - existing ioctls may be assuming the BKL
> > > is taken, but maybe there could be another flag
> > > in f_ops to let sys_ioctl release the lock before
> > > doing the call ...
> > >
> > > Like this - would that be safe?
> > 
> > Yes. It is proven to work.
> 
> Now that I look at the ioctl.c code, I see a several get_user/put_user
> inside the ioctl which are thus done while BKL is held.
> But I thought get_user can block?
> 
> Why is this not a bug?
> 

You can sleep while holding the BKL, it is automatically dropped and
reacquired.  The BKL has some magic properties, it does not work like a
regular spinlock.

Lee

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Hello!
Quoting r. Albert Cahalan (albert@users.sourceforge.net) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
> Michael S. Tsirkin writes:
> > Quoting Lee Revell [snip -- that was excessive]
> 
> >> By adding a new ioctl you are adding a new use of
> >> the BKL. It has been suggested on dri-devel that
> >> this should be fixed.  Is this even possible?
> >
> > I dont know - can the lock be released before the
> > call to filp->f_op->ioctl ?
> >
> > I assume the reason its there is for legacy
> > code - existing ioctls may be assuming the BKL
> > is taken, but maybe there could be another flag
> > in f_ops to let sys_ioctl release the lock before
> > doing the call ...
> >
> > Like this - would that be safe?
> 
> Yes. It is proven to work.

Now that I look at the ioctl.c code, I see a several get_user/put_user
inside the ioctl which are thus done while BKL is held.
But I thought get_user can block?

Why is this not a bug?

MST

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Chris> You forgot a driver specific filesystem which exposes
    Chris> requests in a file per request type style.  Also, there's a
    Chris> simple_transaction type of file which can allow you
    Chris> send/recv data and should eliminate the need for tagging.
    Chris> Example, look at nfsd fs (fs/nfsd/nfsctl.c).

Thanks for the pointer -- I had a look at this stuff.  It seems that
using the simple_transaction stuff is fairly heavyweight -- if I
understand correctly, every operation requires userspace to do
open()-write()-read()-close(), and also uses a page of lowmem.  I'm
not sure if this is the best fit for our requirements with InfiniBand
drivers: although the user->kernel calls are not in the data path,
there can still be quite a few of them.

On the other hand, ioctl() holds the BKL through the whole operation
so that's suboptimal as well.

Thanks,
  Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[viro]

On Wed, Sep 01, 2004 at 11:12:46AM -0700, Roland Dreier wrote:
>     Chris> You forgot a driver specific filesystem which exposes
>     Chris> requests in a file per request type style.  Also, there's a
>     Chris> simple_transaction type of file which can allow you
>     Chris> send/recv data and should eliminate the need for tagging.
>     Chris> Example, look at nfsd fs (fs/nfsd/nfsctl.c).
> 
> Thanks, I'll take a look at this.  How does one handle the 32-bit
> userspace / 64-bit kernel compat layer in this setup?

The sane approach is to have your structure platform-independent and have
no compat code at all.  Which is what we do with on-disk and over-the-wire
structures anyway.  And before Albert starts usual wanksession, no, ASCII
is not the only way to do that...

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Chris> You forgot a driver specific filesystem which exposes
    Chris> requests in a file per request type style.  Also, there's a
    Chris> simple_transaction type of file which can allow you
    Chris> send/recv data and should eliminate the need for tagging.
    Chris> Example, look at nfsd fs (fs/nfsd/nfsctl.c).

Thanks, I'll take a look at this.  How does one handle the 32-bit
userspace / 64-bit kernel compat layer in this setup?

 - R.

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Bill Davidsen]

viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Sep 01, 2004 at 10:22:45AM +0300, Michael S. Tsirkin wrote:
> 
>>Hello!
>>Currently, on the x86_64 architecture, its quite tricky to make
>>a char device ioctl work for an x86 executables.
>>In particular,
>>   1. there is a requirement that ioctl number is unique -
>>      which is hard to guarantee especially for out of kernel modules
> 
> 
> Too bad.
> 
> 
>>   2. there's a performance huge overhead for each compat call - there's
>>      a hash lookup in a global hash inside a lock_kernel -
>>      and I think compat performance *is* important.
>>
>>Further, adding a command to the ioctl suddenly requires changing
>>two places - registration code and ioctl itself.
> 
> 
> So don't add them.  Adding a new ioctl is *NOT* a step to be taken lightly -
> we used to be far too accepting in that area and as somebody who'd waded
> through the resulting dungpiles over the last months I can tell you that
> result is utterly revolting.
> 
> Excuse me, but I have zero sympathy to people who complain about obstacles
> to dumping more into the same piles - it should be hard.

I don't think he was complaining so much as providing a rationale for 
the patch he offered which is intended to make things better. Seemed to 
me like a good context for the patch.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Chris Wright]

* Roland Dreier (roland@topspin.com) wrote:
>  - read/write on char device:
>      OK, except requires some mechanism (tag #) for matching requests
>      and responses.  Nowhere clean to put 32/64 compatibility code.

You forgot a driver specific filesystem which exposes requests in a file
per request type style.  Also, there's a simple_transaction type of file
which can allow you send/recv data and should eliminate the need for
tagging.  Example, look at nfsd fs (fs/nfsd/nfsctl.c).

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

    Albert> Per-command parameters included?

    Albert> People really do want private syscalls. An ioctl is that,
    Albert> in a namespace defined by the file descriptor. So ioctl()
    Albert> provides local scope to something that would otherwise be
    Albert> global.

Yes, this is exactly right.  One issue raised by this thread is that
the ioctl32 compatibility code only allows one compatibility handler
per ioctl number.  It seems that this creates all sorts of
possibilities for mayhem because it makes the ioctl namespace global
in scope in some situations.  Does anyone have any thoughts on if/how
this should be addressed.

 - Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Roland Dreier]

This thread raises the issue of the best way for a driver to handle
commands from userspace.  The typical situation is where the driver
needs to process commands from multiple processes and return a status
for each command.

I happen to work on the same type of drivers as Michael (InfiniBand),
and there are a fairly large number of operations that userspace would
like to call into the kernel for.  User applications ask the kernel
driver to do things like "create completion queue."  One would like to
make this call in a clean, simple, efficient way.

I can think of four ways to do this:

 - ioctl on char device:
     Nice because it is synchronous and allows for the kernel to
     return a status value easily.  Has a well-defined mechanism for
     handling 32-bit/64-bit compatibility.  Unfortunately ioctl
     methods run under the BKL.

 - read/write on char device:
     OK, except requires some mechanism (tag #) for matching requests
     and responses.  Nowhere clean to put 32/64 compatibility code.

 - netlink:
     Similar to read/write except it adds the possibility of dropping
     messages.

 - syscall:
     Syscalls are great in some ways.  They are the most direct way
     into the kernel, they allow 

     However: syscalls can't be added from modules; it's (quite
     correctly) very hard to get new syscalls added to the kernel;
     every arch numbers its syscalls differently.  Let's forget about
     syscalls.

ioctls end up looking like the least bad solution, although I'm open
to other opinions and I'd love to hear better ideas.  I'd be happy
with a policy of only accepting ioctls that are sparse and 32/64 clean
and generally maintainable-looking, but I don't think driver authors
have much alternative to ioctl right now.

Thanks,
  Roland

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Albert Cahalan]

Michael S. Tsirkin writes:
> Quoting Lee Revell [snip -- that was excessive]

>> By adding a new ioctl you are adding a new use of
>> the BKL. It has been suggested on dri-devel that
>> this should be fixed.  Is this even possible?
>
> I dont know - can the lock be released before the
> call to filp->f_op->ioctl ?
>
> I assume the reason its there is for legacy
> code - existing ioctls may be assuming the BKL
> is taken, but maybe there could be another flag
> in f_ops to let sys_ioctl release the lock before
> doing the call ...
>
> Like this - would that be safe?

Yes. It is proven to work.

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Albert Cahalan]

Arjan van de Ven writes:
> On Wed, Sep 01, 2004 at 03:50:11AM -0600, filia@softhome.net wrote:

>> Stop being arrogant. Can you please elaborate on how
>> to make Linux kernel support e.g. motion controllers?
>> They do not fit *any* known to me driver interface.
>> They have several axes, they have about twenty
>> parameters (float or integer), and 
>
> parameters nicely fit in sysfs.

Per-command parameters included?

People really do want private syscalls. An ioctl
is that, in a namespace defined by the file
descriptor. So ioctl() provides local scope to
something that would otherwise be global.

The alternative is to add a zillion odd syscalls.

Life is messy.

>> they have several commands, a-la start, graceful stop,
>> abrupt stop. Plus obviously diagnostics - about ten
>> another commands with absolutely different parameters.
>> And about ten motion monitoring commands. And this is 
>> one example I were need to program. 
>
> a write() interface doesn't work???
> Hard to believe, you even call them commands.
> fd = open("/dev/funkydevice");
> write(fd, "start");
>
> doesn't sound insane to me

That's just a double-crufty ioctl in disguise.
See also: "the /proc filesystem"

It looks like encouragement of choices that will
tend toward the creation of buffer overflows in
the kernel. Nearly all buffer overflows involve
parsing ASCII. Sure, nobody should make mistakes...

People won't do that though. Know what you'll
get if ioctl isn't used? Here you go:

///////////////////////  Look ma, no ioctl!  /////
struct ctldata scd;
struct ctldata *scdp = &scd;

scd.command = GET_MOTOR_TEMPERATURE;
scd.arg[0] = MOTOR_Z;

// Arjan wants it this way.
// (for Viro, must print the pointer in decimal)
write(fd, &scdp, sizeof ctldata);

motor_temp.current = scd.ret[0];
motor_temp.peak    = scd.ret[1];
/////////////////////////////////////////////////

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Arjan van de Ven]

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

On Wed, 2004-09-01 at 09:22, Michael S. Tsirkin wrote:
> Hello!
> Currently, on the x86_64 architecture, its quite tricky to make
> a char device ioctl work for an x86 executables.
> In particular,
>    1. there is a requirement that ioctl number is unique -
>       which is hard to guarantee especially for out of kernel modules

well... external modules thus should be really really careful with
ioctls then and not embrace and extend too much but just use existing
ones instead when reasonable

>    2. there's a performance huge overhead for each compat call - there's
>       a hash lookup in a global hash inside a lock_kernel -
>       and I think compat performance *is* important.

such is life

> 
> Further, adding a command to the ioctl suddenly requires changing
> two places - registration code and ioctl itself.

adding ioctls SHOULD be painful. Really painful. It's similar to adding
syscalls; you'll have to keep compatibility basically forever so adding
should not be an easy thing.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Quoting Lee Revell (rlrevell@joe-job.com) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
> By adding a new ioctl you are adding a new use of the BKL.  It has been
> suggested on dri-devel that this should be fixed.  Is this even
> possible?
> 
> Lee

I dont know - can the lock be released before the call to
filp->f_op->ioctl ?

I assume the reason its there is for legacy code - existing ioctls
may be assuming the BKL is taken, but maybe there could be another
flag in f_ops to let sys_ioctl release the lock before doing the call ...

Like this - would that be safe?


--- linux-2.6.8.1/fs/ioctl.c	2004-08-14 13:54:51.000000000 +0300
+++ linux-2.6.8.1-built/fs/ioctl.c	2004-09-01 11:14:59.944417160 +0300
@@ -126,6 +126,14 @@
 			error = -ENOTTY;
 			if (S_ISREG(filp->f_dentry->d_inode->i_mode))
 				error = file_ioctl(filp, cmd, arg);
+			else if (filp->f_op && filp->f_op->ioctl &&
+				(filp->f_op->fops_flags & FOPS_IOCTL_NOLOCK)) {
+			{
+				unlock_kernel();
+				error = filp->f_op->ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
+				goto out;
+
+			}
 			else if (filp->f_op && filp->f_op->ioctl)
 				error = filp->f_op->ioctl(filp->f_dentry->d_inode, filp, cmd, arg);
 	}

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Lee Revell]

On Wed, 2004-09-01 at 03:32, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Wed, Sep 01, 2004 at 10:22:45AM +0300, Michael S. Tsirkin wrote:
> > Hello!
> > Currently, on the x86_64 architecture, its quite tricky to make
> > a char device ioctl work for an x86 executables.
> > In particular,
> >    1. there is a requirement that ioctl number is unique -
> >       which is hard to guarantee especially for out of kernel modules
> 
> Too bad.
> 
> >    2. there's a performance huge overhead for each compat call - there's
> >       a hash lookup in a global hash inside a lock_kernel -
> >       and I think compat performance *is* important.
> > 
> > Further, adding a command to the ioctl suddenly requires changing
> > two places - registration code and ioctl itself.
> 
> So don't add them.  Adding a new ioctl is *NOT* a step to be taken lightly -
> we used to be far too accepting in that area and as somebody who'd waded
> through the resulting dungpiles over the last months I can tell you that
> result is utterly revolting.
> 

By adding a new ioctl you are adding a new use of the BKL.  It has been
suggested on dri-devel that this should be fixed.  Is this even
possible?

Lee

Re: f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Hello!
Please do not mail me directly.

Quoting viro@parcelfarce.linux.theplanet.co.uk (viro@parcelfarce.linux.theplanet.co.uk) "Re: f_ops flag to speed up compatible ioctls in linux kernel":
> On Wed, Sep 01, 2004 at 10:22:45AM +0300, Michael S. Tsirkin wrote:
> >    2. there's a performance huge overhead for each compat call - there's
> >       a hash lookup in a global hash inside a lock_kernel -
> >       and I think compat performance *is* important.
> > 
> > Further, adding a command to the ioctl suddenly requires changing
> > two places - registration code and ioctl itself.
> 
> So don't add them.  Adding a new ioctl is *NOT* a step to be taken lightly -
> we used to be far too accepting in that area and as somebody who'd waded
> through the resulting dungpiles over the last months I can tell you that
> result is utterly revolting.

Why make it a bigger dungpile then?
And what about the performance overhead?

MST

Re: f_ops flag to speed up compatible ioctls in linux kernel

[viro]

On Wed, Sep 01, 2004 at 10:22:45AM +0300, Michael S. Tsirkin wrote:
> Hello!
> Currently, on the x86_64 architecture, its quite tricky to make
> a char device ioctl work for an x86 executables.
> In particular,
>    1. there is a requirement that ioctl number is unique -
>       which is hard to guarantee especially for out of kernel modules

Too bad.

>    2. there's a performance huge overhead for each compat call - there's
>       a hash lookup in a global hash inside a lock_kernel -
>       and I think compat performance *is* important.
> 
> Further, adding a command to the ioctl suddenly requires changing
> two places - registration code and ioctl itself.

So don't add them.  Adding a new ioctl is *NOT* a step to be taken lightly -
we used to be far too accepting in that area and as somebody who'd waded
through the resulting dungpiles over the last months I can tell you that
result is utterly revolting.

Excuse me, but I have zero sympathy to people who complain about obstacles
to dumping more into the same piles - it should be hard.

f_ops flag to speed up compatible ioctls in linux kernel

[Michael S. Tsirkin]

Hello!
Currently, on the x86_64 architecture, its quite tricky to make
a char device ioctl work for an x86 executables.
In particular,
   1. there is a requirement that ioctl number is unique -
      which is hard to guarantee especially for out of kernel modules
   2. there's a performance huge overhead for each compat call - there's
      a hash lookup in a global hash inside a lock_kernel -
      and I think compat performance *is* important.

Further, adding a command to the ioctl suddenly requires changing
two places - registration code and ioctl itself.
  
However, if all ioctl commands for a specific device
are not passing around pointers and long longs,
(relatively common case), and sometimes even
if they are, this ioctl is 64/32 bit compatible -
so why isnt there a simple way to declare this fact?

Here's a patch that simply adds a flag field to f_ops
that will cause all ioctls to get forwarded directly to the
64 bit call.

If all ioctls are compatible for a character device, a flag just
has to be set there, before the device is registered.

MST

diff -ruw linux-2.6.8.1/fs/compat.c linux-2.6.8.1-built/fs/compat.c
--- linux-2.6.8.1/fs/compat.c	2004-08-14 13:55:31.000000000 +0300
+++ linux-2.6.8.1-built/fs/compat.c	2004-09-01 09:52:10.126944256 +0300
@@ -392,7 +392,8 @@
 	if(!filp)
 		goto out2;
 
-	if (!filp->f_op || !filp->f_op->ioctl) {
+	if (!filp->f_op || !filp->f_op->ioctl ||
+          (filp->f_op->fops_flags & FOPS_IOCTL_COMPAT)) {
 		error = sys_ioctl (fd, cmd, arg);
 		goto out;
 	}
diff -ruw linux-2.6.8.1/include/linux/fs.h linux-2.6.8.1-built/include/linux/fs.h
--- linux-2.6.8.1/include/linux/fs.h	2004-08-14 13:55:09.000000000 +0300
+++ linux-2.6.8.1-built/include/linux/fs.h	2004-09-01 09:50:07.265622016 +0300
@@ -871,6 +871,7 @@
  */
 struct file_operations {
 	struct module *owner;
+	unsigned long fops_flags;		/* Flags, listed below. */
 	loff_t (*llseek) (struct file *, loff_t, int);
 	ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
 	ssize_t (*aio_read) (struct kiocb *, char __user *, size_t, loff_t);
@@ -896,6 +897,8 @@
 	int (*dir_notify)(struct file *filp, unsigned long arg);
 };
 
+#define FOPS_IOCTL_COMPAT 0x00000001 /* ioctl is 32/64 bit compatible */
+
 struct inode_operations {
 	int (*create) (struct inode *,struct dentry *,int, struct nameidata *);
 	struct dentry * (*lookup) (struct inode *,struct dentry *, struct nameidata *);