Re: [patch 1/8] Immediate Values - Global Modules List and Module Mutex

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Rusty Russell (rusty@rustcorp.com.au) wrote:
> On Tue, 2007-09-18 at 09:41 -0400, Mathieu Desnoyers wrote:
> > * Rusty Russell (rusty@rustcorp.com.au) wrote:
> > > On Fri, 2007-09-14 at 11:32 -0400, Mathieu Desnoyers wrote:
> > > > * Rusty Russell (rusty@rustcorp.com.au) wrote:
> > > > > Alternatively, if you called it "immediate_init" then the semantics
> > > > > change slightly, but are more obvious (ie. only use this when the value
> > > > > isn't being accessed yet).  But it can't be __init then anyway.
> > > > > 
> > > > 
> > > > I think your idea is good. immediate_init() could be used to update the
> > > > immediate values at boot time _and_ at module load time, and we could
> > > > use an architecture specific arch_immediate_update_init() to support it.
> > > 
> > > Right.
> > > 
> > > > As for "when" to use this, it should be used at boot time when
> > > > interrupts are still disabled, still running in UP. It can also be used
> > > > at module load time before any of the module code is executed, as long
> > > > as the module code pages are writable (which they always are, for
> > > > now..). Therefore, the flag seems inappropriate for module load
> > > > arch_immediate_update_init. It cannot be put in __init section neither
> > > > though if we use it like this.
> > > 
> > > I think from a user's POV it would be nice to have a 1:1 mapping with
> > > normal initialization semantics (ie. it will work as long as you don't
> > > access this value until initialized).  And I think this would be the
> > > case.  eg:
> > > 
> > >         int foo_func(void)
> > >         {
> > >         	if (immediate_read(&some_immediate))
> > >         		return 0;
> > >         	...
> > >         }
> > >         
> > >         int some_init(void)
> > >         {
> > >         	immediate_init(some_immediate, 0);
> > >         	register_foo(foo_func);
> > >         	...
> > >         }
> > > 
> > 
> > There are other considerations that differs between the boot-time case
> > and the general "init" case: the write-protection flag must be
> > cleared-saved/restored when the kernel is running to patch read-only
> > text, but we don't want to modify cr0 at early boot on i386 because
> > paravirt is not executed yet (at boot time, pages are not
> > write-protected yet).
> > 
> > And I am not sure that it buys us anything to create an immediate_init()
> > when we can do exactly the same job with immediate_set. Yes, it might be
> > a bit slower, but we are not on a fast path.
> 
> Good points.  Well I'd say hiding it all behind a friendly
> "immediate_set()" interface is the best option then.
> 

Then we can't benefit of the __init section to have the code removed
after boot. I don't see the point in doing so.

> > > OK, but can you justify the use of immediates within the nmi or mce
> > > handlers?  They don't strike me as useful candidates for optimization.
> > 
> > Yes, immediate values are used by the Linux Kernel Markers, which
> > instrument many code paths, including functions called from nmi and mce
> > contexts (including printk).
> 
> Is this really worth worrying about?  Isn't there already a problem with
> printk() in nmi?
> 

printk() is just an example taken from my current instrumentation. Let's
say I plan to instrument kmalloc() (which will happen someday): it's
used in every context, including NMI. And it's not because printk is
broken that its instrumentation can afford to be broken even further,
that logic seems wrong to me. Somebody go fix printk then ;)

Mathieu

> Cheers,
> Rusty.
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68