lkml.org 
[lkml]   [2009]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -tip 4/4] Atomic text_poke() with fixmap take2
* Ingo Molnar (mingo@elte.hu) wrote:
>
> * Masami Hiramatsu <mhiramat@redhat.com> wrote:
>
> > @@ -523,14 +526,17 @@ void *__kprobes text_poke(void *addr, co
> > pages[1] = virt_to_page(addr + PAGE_SIZE);
> > }
> > BUG_ON(!pages[0]);
> > - if (!pages[1])
> > - nr_pages = 1;
> > - vaddr = vmap(pages, nr_pages, VM_MAP, PAGE_KERNEL);
> > - BUG_ON(!vaddr);
> > - local_irq_disable();
> > + local_irq_save(flags);
> > + set_fixmap(FIX_TEXT_POKE0, page_to_phys(pages[0]));
> > + if (pages[1])
> > + set_fixmap(FIX_TEXT_POKE1, page_to_phys(pages[1]));
> > + vaddr = (char *)fix_to_virt(FIX_TEXT_POKE0);
> > memcpy(&vaddr[(unsigned long)addr & ~PAGE_MASK], opcode, len);
> > - local_irq_enable();
> > - vunmap(vaddr);
> > + clear_fixmap(FIX_TEXT_POKE0);
> > + if (pages[1])
> > + clear_fixmap(FIX_TEXT_POKE1);
> > + local_flush_tlb();
> > + local_irq_restore(flags);
> > sync_core();
>
> I'm not sure at all about this widening of the irq-atomic
> section and the idea of allowing non-locked access on single-CPU
> situations - we dont really want to micro-optimize any of this
> on such a level, holding the text lock is a robust rule all code
> should be listening to. (Creating locking assymetry always
> inserts a certain amount of fragility - adding to an already
> fragile concept here.)
>
> And note that there's no reason why text_poke could not be used
> in stop_machine_run() - the stop_machine_run() handler must not
> take the text_lock of course - but outside code calling
> stop_machine_run() can do it and can hence serialize properly.
>
> Note that even if we did this then your v2 patch is not fully
> correct: you need to move the sync_core() at the end of the
> sequence inside the critical section too. (right now this is
> mostly harmless because the INVLPG inside the clear_fixmap()
> happens to be serializing so it has an implicit sync_core()
> property - but nevertheless we better do this straight away to
> not cause problems later down the line.)
>
> Ingo

Agreed. The alternatives_smp_lock/alternatives_smp_unlock specific case
does not bring us much if it has no perceivable performance impact. It's
better to keep a standard interface and clear requirements.

Mathieu


--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68


\
 
 \ /
  Last update: 2009-03-06 20:23    [W:0.065 / U:0.308 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site