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

* 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


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