lkml.org 
[lkml]   [2007]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRE: [patch 05/24] Text Edit Lock - Architecture Independent Code
    Date

    > > ===================================================================
    > > --- linux-2.6-lttng.orig/kernel/kprobes.c 2007-12-12
    > > 18:10:32.000000000 -0500
    > > +++ linux-2.6-lttng/kernel/kprobes.c 2007-12-12
    > > 18:10:34.000000000 -0500
    > > @@ -644,7 +644,9 @@ valid_p:
    > > list_del_rcu(&p->list);
    > > kfree(old_p);
    > > }
    > > + mutex_lock(&kprobe_mutex);
    > > arch_remove_kprobe(p);
    > > + mutex_unlock(&kprobe_mutex);
    > > } else {
    > > mutex_lock(&kprobe_mutex);
    > > if (p->break_handler)
    >
    > I think "mutex_lock" and "mutex_unlock" shoud be in architecture code.
    > In "__register_kprobe" funtion, its implement
    > "arch_prepare_kprobe" and
    > "arch_arm_kprobe" is also depended on arch. So the remove
    > implement is not
    > the same on the different architecture code.
    >
    > Maybe it doesn't need the mutex_lock in "arch_remove_kprobe"
    > on some embeded
    > system chips if linux can support the other embeded system
    > chips in future.

    Could we insert the "mutex_lock" and "mutex_unlock" into "free_insn_slot"
    instead of architecture code?

    modify as follows:

    void __kprobes free_insn_slot(kprobe_opcode_t * slot, int dirty)
    {
    struct kprobe_insn_page *kip;
    struct hlist_node *pos;

    + mutex_lock(&kprobe_mutex);
    hlist_for_each_entry(kip, pos, &kprobe_insn_pages, hlist) {
    if (kip->insns <= slot &&
    slot < kip->insns + (INSNS_PER_PAGE * MAX_INSN_SIZE)) {
    int i = (slot - kip->insns) / MAX_INSN_SIZE;
    if (dirty) {
    kip->slot_used[i] = SLOT_DIRTY;
    kip->ngarbage++;
    } else {
    collect_one_slot(kip, i);
    }
    break;
    }
    }

    if (dirty && ++kprobe_garbage_slots > INSNS_PER_PAGE)
    collect_garbage_slots();
    + mutex_unlock(&kprobe_mutex);
    }


    > -----Original Message-----
    > From: linux-kernel-owner@vger.kernel.org
    > [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of zhangxiliang
    > Sent: Friday, December 21, 2007 1:19 PM
    > To: 'Mathieu Desnoyers'; akpm@linux-foundation.org; 'Ingo
    > Molnar'; linux-kernel@vger.kernel.org
    > Cc: 'Andi Kleen'
    > Subject: RE: [patch 05/24] Text Edit Lock - Architecture
    > Independent Code
    >
    > hello,
    > I have some questions for your patches.
    >
    > > Paravirt and alternatives are always done when SMP is
    > > inactive, so there is no
    > > need to use locks.
    >
    > > -#ifndef CONFIG_KPROBES
    > > -#ifdef CONFIG_HOTPLUG_CPU
    > > - /* It must still be possible to apply SMP alternatives. */
    > > - if (num_possible_cpus() <= 1)
    > > -#endif
    > > - {
    > > - change_page_attr(virt_to_page(start),
    > > - size >> PAGE_SHIFT, PAGE_KERNEL_RX);
    > > - printk("Write protecting the kernel text:
    > > %luk\n", size >> 10);
    > > - }
    > > -#endif
    > > + change_page_attr(virt_to_page(start),
    > > + size >> PAGE_SHIFT, PAGE_KERNEL_RX);
    > > + printk(KERN_INFO "Write protecting the kernel text: %luk\n",
    > > + size >> 10);
    > > +
    >
    > Why "mark_rodata_ro" doesn't consider smp instance? Maybe it
    > will be appied in
    > future.
    >
    >
    > > ===================================================================
    > > --- linux-2.6-lttng.orig/kernel/kprobes.c 2007-12-12
    > > 18:10:32.000000000 -0500
    > > +++ linux-2.6-lttng/kernel/kprobes.c 2007-12-12
    > > 18:10:34.000000000 -0500
    > > @@ -644,7 +644,9 @@ valid_p:
    > > list_del_rcu(&p->list);
    > > kfree(old_p);
    > > }
    > > + mutex_lock(&kprobe_mutex);
    > > arch_remove_kprobe(p);
    > > + mutex_unlock(&kprobe_mutex);
    > > } else {
    > > mutex_lock(&kprobe_mutex);
    > > if (p->break_handler)
    >
    > I think "mutex_lock" and "mutex_unlock" shoud be in architecture code.
    > In "__register_kprobe" funtion, its implement
    > "arch_prepare_kprobe" and
    > "arch_arm_kprobe" is also depended on arch. So the remove
    > implement is not
    > the same on the different architecture code.
    >
    > Maybe it doesn't need the mutex_lock in "arch_remove_kprobe"
    > on some embeded
    > system chips if linux can support the other embeded system
    > chips in future.
    >
    >
    > > -----Original Message-----
    > > From: linux-kernel-owner@vger.kernel.org
    > > [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
    > > Mathieu Desnoyers
    > > Sent: Friday, December 21, 2007 9:55 AM
    > > To: akpm@linux-foundation.org; Ingo Molnar;
    > > linux-kernel@vger.kernel.org
    > > Cc: Mathieu Desnoyers; Andi Kleen
    > > Subject: [patch 05/24] Text Edit Lock - Architecture
    > Independent Code
    > >
    > > This is an architecture independant synchronization around
    > kernel text
    > > modifications through use of a global mutex.
    > >
    > > A mutex has been chosen so that kprobes, the main user of
    > > this, can sleep during
    > > memory allocation between the memory read of the instructions
    > > it must replace
    > > and the memory write of the breakpoint.
    > >
    > > Other user of this interface: immediate values.
    > >
    > > Paravirt and alternatives are always done when SMP is
    > > inactive, so there is no
    > > need to use locks.
    > >
    > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
    > > CC: Andi Kleen <andi@firstfloor.org>
    > > ---
    > > include/linux/memory.h | 7 +++++++
    > > mm/memory.c | 34 ++++++++++++++++++++++++++++++++++
    > > 2 files changed, 41 insertions(+)
    > >
    > > Index: linux-2.6-lttng/include/linux/memory.h
    > > ===================================================================
    > > --- linux-2.6-lttng.orig/include/linux/memory.h>
    > > 2007-11-07 11:11:26.000000000 -0500
    > > +++ linux-2.6-lttng/include/linux/memory.h 2007-11-07
    > > 11:13:48.000000000 -0500
    > > @@ -93,4 +93,11 @@ extern int memory_notify(unsigned long v
    > > #define hotplug_memory_notifier(fn, pri) do { } while (0)
    > > #endif
    > >
    > > +/*
    > > + * Take and release the kernel text modification lock, used
    > > for code patching.
    > > + * Users of this lock can sleep.
    > > + */
    > > +extern void kernel_text_lock(void);
    > > +extern void kernel_text_unlock(void);
    > > +
    > > #endif /* _LINUX_MEMORY_H_ */
    > > Index: linux-2.6-lttng/mm/memory.c
    > > ===================================================================
    > > --- linux-2.6-lttng.orig/mm/memory.c 2007-11-07
    > > 11:12:33.000000000 -0500
    > > +++ linux-2.6-lttng/mm/memory.c 2007-11-07
    > > 11:14:25.000000000 -0500
    > > @@ -50,6 +50,8 @@
    > > #include <linux/delayacct.h>
    > > #include <linux/init.h>
    > > #include <linux/writeback.h>
    > > +#include <linux/kprobes.h>
    > > +#include <linux/mutex.h>
    > >
    > > #include <asm/pgalloc.h>
    > > #include <asm/uaccess.h>
    > > @@ -84,6 +86,12 @@ EXPORT_SYMBOL(high_memory);
    > >
    > > int randomize_va_space __read_mostly = 1;
    > >
    > > +/*
    > > + * mutex protecting text section modification (dynamic code
    > > patching).
    > > + * some users need to sleep (allocating memory...) while
    > > they hold this lock.
    > > + */
    > > +static DEFINE_MUTEX(text_mutex);
    > > +
    > > static int __init disable_randmaps(char *s)
    > > {
    > > randomize_va_space = 0;
    > > @@ -2748,3 +2756,29 @@ int access_process_vm(struct task_struct
    > >
    > > return buf - old_buf;
    > > }
    > > +
    > > +/**
    > > + * kernel_text_lock - Take the kernel text modification lock
    > > + *
    > > + * Insures mutual write exclusion of kernel and modules text
    > > live text
    > > + * modification. Should be used for code patching.
    > > + * Users of this lock can sleep.
    > > + */
    > > +void __kprobes kernel_text_lock(void)
    > > +{
    > > + mutex_lock(&text_mutex);
    > > +}
    > > +EXPORT_SYMBOL_GPL(kernel_text_lock);
    > > +
    > > +/**
    > > + * kernel_text_unlock - Release the kernel text
    > modification lock
    > > + *
    > > + * Insures mutual write exclusion of kernel and modules text
    > > live text
    > > + * modification. Should be used for code patching.
    > > + * Users of this lock can sleep.
    > > + */
    > > +void __kprobes kernel_text_unlock(void)
    > > +{
    > > + mutex_unlock(&text_mutex);
    > > +}
    > > +EXPORT_SYMBOL_GPL(kernel_text_unlock);
    > >
    > > --
    > > 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/
    > >
    > >
    >
    >
    >
    > --
    > 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/
    >
    >




    \
     
     \ /
      Last update: 2007-12-21 07:05    [W:0.038 / U:121.472 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site