Messages in this thread | | | Date | Tue, 11 Sep 2007 10:27:33 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [patch 1/8] Immediate Values - Global Modules List and Module Mutex |
| |
* Rusty Russell (rusty@rustcorp.com.au) wrote: > On Mon, 2007-09-10 at 20:45 -0400, Mathieu Desnoyers wrote: > > Code patching of _live_ SMP code is allowed. This is why I went through > > all this trouble on i386. > > Oh, I was pretty sure it wasn't. OK. > > So now why three versions of immediate_set()? And why are you using my > lock for exclusion? Against what? >
If we need to patch code at boot time, when interrupts are still disabled (it happens when we parse the kernel arguments for instance), we cannot afford to use IPIs to call sync_core() on each cpu, using breakpoints/notifier chains could be tricky (because we are very early at boot and alternatives or paravirt may not have been applied yet).
So, for early boot code patching, immediate_set_early() is used. It presumes that variables are not used when the code is patched, that we are in UP context and it is really minimalistic.
On the other hand, immediate_set() updates kernel and module variables while the (potentially smp) system is running. It provides coherent variable updates even if the code referencing then is executing.
_immediate_set() has been introduced because of the way immediate values are used by markers: the linux kernel markers already hold the module mutex when they need to update the immediate values. Taking the mutex twice makes no sence, so _immediate_set() is used when the caller already holds the module mutex.
> Why not just have one immediate_set() which iterates through and fixes > up all the references?
(reasons explained above)
> It can use an internal lock if you want to avoid > concurrent immediate_set() calls. >
An internal lock won't protect against modules load/unload race. We have to iterate on the module list.
> I understand the need for a "module_immediate_fixup()" but that can also > use your internal lock.
It looks interesting.. can you elaborate a little bit more on this idea? If we can find a way to encapsulate in module.c everything that needs to touch the module list, I am all for it.
> > > > 2) immediate_if() needs an implementation before you introduce it. Your > > > assumption that it's always unlikely seems non-orthogonal. > > > > I could remove the unlikely(), no problem with that. Your point about > > this is valid. However, I would like to leave the immediate_if() there > > because it may become very useful if someday gcc permits to extract the > > address of a branch instruction (and to generate assembly that would not > > be reached without doing code patching). > > Why is it easier to patch the sites now than later? Currently it's just > churn. You could go back and find them when this mythical patch gets > merged into this mythical future gcc version. It could well need a > completely different macro style, like "cond_imm(var, code)". >
Maybe you're right. My though was that if we have a way to express a strictly boolean if() statement that can later be optimized further by gcc using a jump rather than a conditionnal branch and currently emulate it by using a load immediate/test/branch, we might want to do so right now so we don't have to do a second code transition from if (immediate_read(&var)) to immediate_if (&var) later. But you might be right in that the form could potentially change anyway when the implementation would come, although I don't see how.
Mathieu
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |