Writable module parameters - should be volatile?

Writable module parameters - should be volatile?

[Duncan Sands]

I declare a writable module parameter as follows:

static unsigned int num_rcv_urbs = UDSL_DEFAULT_RCV_URBS;

module_param (num_rcv_urbs, uint, S_IRUGO | S_IWUSR);

Shouldn't I declare num_rcv_urbs volatile?  Otherwise compiler
optimizations could (for example) stick it in a register and miss
any changes made by someone writing to it...  However, if I do
declare it volatile then I get a warning:

In function `__check_num_rcv_urbs':
warning: return discards qualifiers from pointer target type

So what is the right thing to do?

Thanks,

Duncan.

Re: Writable module parameters - should be volatile?

[Arnd Bergmann]

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

On Sonntag, 12. September 2004 13:57, Duncan Sands wrote:
> I declare a writable module parameter as follows:
> 
> static unsigned int num_rcv_urbs = UDSL_DEFAULT_RCV_URBS;
> 
> module_param (num_rcv_urbs, uint, S_IRUGO | S_IWUSR);
> 
> Shouldn't I declare num_rcv_urbs volatile?  Otherwise compiler
> optimizations could (for example) stick it in a register and miss
> any changes made by someone writing to it...

Even worse, AFAICS there is no guarantee that writes are atomic,
which can give unpredictable results in case of strings or arrays.
Both problems can be solved by serializing access to writable
module parameters.

Maybe we could have a global module_param_rwsem. Making the
parameter volatile does not sound like the right solution, in
fact volatile is almost always a bad idea.

	Arnd <><

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

Re: Writable module parameters - should be volatile?

[Duncan Sands]

On Sunday 12 September 2004 14:52, Arnd Bergmann wrote:
> On Sonntag, 12. September 2004 13:57, Duncan Sands wrote:
> > I declare a writable module parameter as follows:
> > 
> > static unsigned int num_rcv_urbs = UDSL_DEFAULT_RCV_URBS;
> > 
> > module_param (num_rcv_urbs, uint, S_IRUGO | S_IWUSR);
> > 
> > Shouldn't I declare num_rcv_urbs volatile?  Otherwise compiler
> > optimizations could (for example) stick it in a register and miss
> > any changes made by someone writing to it...
> 
> Even worse, AFAICS there is no guarantee that writes are atomic,
> which can give unpredictable results in case of strings or arrays.
> Both problems can be solved by serializing access to writable
> module parameters.
> 
> Maybe we could have a global module_param_rwsem. Making the
> parameter volatile does not sound like the right solution, in
> fact volatile is almost always a bad idea.

Another possibility is for a module to make an explicit "flush" call when it
is happy to have parameters updated.  I'll call the parameter "p".  The
moduleparam code would need keep a copy "p_current" of p as well
as a pointer "&p" to the actual parameter p.  Writes to p would only update
p_current, not p, so would not immediately be seen by the module's code. 
A call to flush would then write p_current to the actual parameter *(&p).  This has
the advantage of centralizing locking in the common moduleparam code,
which needs to ensure that p_current is updated atomically.
It has the down side that driver writers will have to call flush at appropriate
times, which could be a maintenance burden (I'm thinking of
drivers/usb/core/devio.c which has a writeable parameter usbfs_snoop
which causes usbfs ioctls to be logged when set; it doesn't hurt if usbfs_snoop
changes at any time; but when adding a new use of usbfs_snoop it would be
easy to forget to put a flush call in every code path leading there).  Still, this
is much lighter weight that having drivers take a module_param_rwsem
whenever they access parameters.

All the best,

Duncan.

Re: Writable module parameters - should be volatile?

[Rusty Russell]

On Sun, 2004-09-12 at 21:57, Duncan Sands wrote:
> I declare a writable module parameter as follows:
> 
> static unsigned int num_rcv_urbs = UDSL_DEFAULT_RCV_URBS;
> 
> module_param (num_rcv_urbs, uint, S_IRUGO | S_IWUSR);
> 
> Shouldn't I declare num_rcv_urbs volatile?  Otherwise compiler
> optimizations could (for example) stick it in a register and miss
> any changes made by someone writing to it...

Sure.  If it really matters, you're going to need something stronger
than that, eg module_param_call().  For debugging, it's not usually a
problem.  For more complex parameters, you usually need locks anyway.

Cheers,
Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

Re: Writable module parameters - should be volatile?

[Duncan Sands]

Hi Rusty,

> > Shouldn't I declare num_rcv_urbs volatile?  Otherwise compiler
> > optimizations could (for example) stick it in a register and miss
> > any changes made by someone writing to it...
> 
> Sure.

except for the compiler warnings...

> If it really matters, you're going to need something stronger 
> than that, eg module_param_call().  For debugging, it's not usually a
> problem.  For more complex parameters, you usually need locks anyway.

Ah ha! - you can supply your own "set" and "get" methods.  Yes, that solves it.

Ciao,

Duncan.