* [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs
2012-08-19 16:40 [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Oleg Nesterov
@ 2012-08-19 16:40 ` Oleg Nesterov
2012-09-14 16:18 ` Srikar Dronamraju
2012-08-19 16:40 ` [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES Oleg Nesterov
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2012-08-19 16:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju
Cc: Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel
Nobody plays with uprobes_tree/uprobes_treelock in interrupt context,
no need to disable irqs.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/events/uprobes.c | 21 ++++++++-------------
1 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 25c0d74..abe077e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -413,11 +413,10 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
static struct uprobe *find_uprobe(struct inode *inode, loff_t offset)
{
struct uprobe *uprobe;
- unsigned long flags;
- spin_lock_irqsave(&uprobes_treelock, flags);
+ spin_lock(&uprobes_treelock);
uprobe = __find_uprobe(inode, offset);
- spin_unlock_irqrestore(&uprobes_treelock, flags);
+ spin_unlock(&uprobes_treelock);
return uprobe;
}
@@ -464,12 +463,11 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
*/
static struct uprobe *insert_uprobe(struct uprobe *uprobe)
{
- unsigned long flags;
struct uprobe *u;
- spin_lock_irqsave(&uprobes_treelock, flags);
+ spin_lock(&uprobes_treelock);
u = __insert_uprobe(uprobe);
- spin_unlock_irqrestore(&uprobes_treelock, flags);
+ spin_unlock(&uprobes_treelock);
/* For now assume that the instruction need not be single-stepped */
uprobe->flags |= UPROBE_SKIP_SSTEP;
@@ -707,11 +705,9 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vad
*/
static void delete_uprobe(struct uprobe *uprobe)
{
- unsigned long flags;
-
- spin_lock_irqsave(&uprobes_treelock, flags);
+ spin_lock(&uprobes_treelock);
rb_erase(&uprobe->rb_node, &uprobes_tree);
- spin_unlock_irqrestore(&uprobes_treelock, flags);
+ spin_unlock(&uprobes_treelock);
iput(uprobe->inode);
put_uprobe(uprobe);
atomic_dec(&uprobe_events);
@@ -969,7 +965,6 @@ static void build_probe_list(struct inode *inode,
struct list_head *head)
{
loff_t min, max;
- unsigned long flags;
struct rb_node *n, *t;
struct uprobe *u;
@@ -977,7 +972,7 @@ static void build_probe_list(struct inode *inode,
min = vaddr_to_offset(vma, start);
max = min + (end - start) - 1;
- spin_lock_irqsave(&uprobes_treelock, flags);
+ spin_lock(&uprobes_treelock);
n = find_node_in_range(inode, min, max);
if (n) {
for (t = n; t; t = rb_prev(t)) {
@@ -995,7 +990,7 @@ static void build_probe_list(struct inode *inode,
atomic_inc(&u->ref);
}
}
- spin_unlock_irqrestore(&uprobes_treelock, flags);
+ spin_unlock(&uprobes_treelock);
}
/*
--
1.5.5.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs
2012-08-19 16:40 ` [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs Oleg Nesterov
@ 2012-09-14 16:18 ` Srikar Dronamraju
0 siblings, 0 replies; 12+ messages in thread
From: Srikar Dronamraju @ 2012-09-14 16:18 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli,
Anton Arapov, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:37]:
> Nobody plays with uprobes_tree/uprobes_treelock in interrupt context,
> no need to disable irqs.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> kernel/events/uprobes.c | 21 ++++++++-------------
> 1 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 25c0d74..abe077e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -413,11 +413,10 @@ static struct uprobe *__find_uprobe(struct inode *inode, loff_t offset)
> static struct uprobe *find_uprobe(struct inode *inode, loff_t offset)
> {
> struct uprobe *uprobe;
> - unsigned long flags;
>
> - spin_lock_irqsave(&uprobes_treelock, flags);
> + spin_lock(&uprobes_treelock);
> uprobe = __find_uprobe(inode, offset);
> - spin_unlock_irqrestore(&uprobes_treelock, flags);
> + spin_unlock(&uprobes_treelock);
>
> return uprobe;
> }
> @@ -464,12 +463,11 @@ static struct uprobe *__insert_uprobe(struct uprobe *uprobe)
> */
> static struct uprobe *insert_uprobe(struct uprobe *uprobe)
> {
> - unsigned long flags;
> struct uprobe *u;
>
> - spin_lock_irqsave(&uprobes_treelock, flags);
> + spin_lock(&uprobes_treelock);
> u = __insert_uprobe(uprobe);
> - spin_unlock_irqrestore(&uprobes_treelock, flags);
> + spin_unlock(&uprobes_treelock);
>
> /* For now assume that the instruction need not be single-stepped */
> uprobe->flags |= UPROBE_SKIP_SSTEP;
> @@ -707,11 +705,9 @@ remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vad
> */
> static void delete_uprobe(struct uprobe *uprobe)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&uprobes_treelock, flags);
> + spin_lock(&uprobes_treelock);
> rb_erase(&uprobe->rb_node, &uprobes_tree);
> - spin_unlock_irqrestore(&uprobes_treelock, flags);
> + spin_unlock(&uprobes_treelock);
> iput(uprobe->inode);
> put_uprobe(uprobe);
> atomic_dec(&uprobe_events);
> @@ -969,7 +965,6 @@ static void build_probe_list(struct inode *inode,
> struct list_head *head)
> {
> loff_t min, max;
> - unsigned long flags;
> struct rb_node *n, *t;
> struct uprobe *u;
>
> @@ -977,7 +972,7 @@ static void build_probe_list(struct inode *inode,
> min = vaddr_to_offset(vma, start);
> max = min + (end - start) - 1;
>
> - spin_lock_irqsave(&uprobes_treelock, flags);
> + spin_lock(&uprobes_treelock);
> n = find_node_in_range(inode, min, max);
> if (n) {
> for (t = n; t; t = rb_prev(t)) {
> @@ -995,7 +990,7 @@ static void build_probe_list(struct inode *inode,
> atomic_inc(&u->ref);
> }
> }
> - spin_unlock_irqrestore(&uprobes_treelock, flags);
> + spin_unlock(&uprobes_treelock);
> }
>
> /*
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES
2012-08-19 16:40 [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Oleg Nesterov
2012-08-19 16:40 ` [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs Oleg Nesterov
@ 2012-08-19 16:40 ` Oleg Nesterov
2012-09-14 16:32 ` Srikar Dronamraju
2012-08-19 16:40 ` [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES Oleg Nesterov
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2012-08-19 16:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju
Cc: Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel
Add the new MMF_RECALC_UPROBES flag, it means that MMF_HAS_UPROBES
can be false positive after remove_breakpoint() or uprobe_munmap().
It is also set by uprobe_dup_mmap(), this is not optimal but simple.
We could add the new hook, uprobe_dup_vma(), to set MMF_HAS_UPROBES
only if the new mm actually has uprobes, but I don't think this
makes sense.
The next patch will use this flag to clear MMF_HAS_UPROBES.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/sched.h | 3 ++-
kernel/events/uprobes.c | 39 +++++++++++++++++++++++++++++++++++----
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c0fcfb7..d6899b0 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -441,7 +441,8 @@ extern int get_dumpable(struct mm_struct *mm);
#define MMF_VM_HUGEPAGE 17 /* set when VM_HUGEPAGE is set on vma */
#define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
-#define MMF_HAS_UPROBES 19 /* might have uprobes */
+#define MMF_HAS_UPROBES 19 /* has uprobes */
+#define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
#define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index abe077e..176de8c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -686,7 +686,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
set_bit(MMF_HAS_UPROBES, &mm->flags);
ret = set_swbp(&uprobe->arch, mm, vaddr);
- if (ret && first_uprobe)
+ if (!ret)
+ clear_bit(MMF_RECALC_UPROBES, &mm->flags);
+ else if (first_uprobe)
clear_bit(MMF_HAS_UPROBES, &mm->flags);
return ret;
@@ -695,6 +697,11 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
static void
remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
{
+ /* can happen if uprobe_register() fails */
+ if (!test_bit(MMF_HAS_UPROBES, &mm->flags))
+ return;
+
+ set_bit(MMF_RECALC_UPROBES, &mm->flags);
set_orig_insn(&uprobe->arch, mm, vaddr);
}
@@ -1027,6 +1034,25 @@ int uprobe_mmap(struct vm_area_struct *vma)
return 0;
}
+static bool
+vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long end)
+{
+ loff_t min, max;
+ struct inode *inode;
+ struct rb_node *n;
+
+ inode = vma->vm_file->f_mapping->host;
+
+ min = vaddr_to_offset(vma, start);
+ max = min + (end - start) - 1;
+
+ spin_lock(&uprobes_treelock);
+ n = find_node_in_range(inode, min, max);
+ spin_unlock(&uprobes_treelock);
+
+ return !!n;
+}
+
/*
* Called in context of a munmap of a vma.
*/
@@ -1038,10 +1064,12 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
return;
- if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags))
+ if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags) ||
+ test_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags))
return;
- /* TODO: unmapping uprobe(s) will need more work */
+ if (vma_has_uprobes(vma, start, end))
+ set_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags);
}
/* Slot allocation for XOL */
@@ -1147,8 +1175,11 @@ void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
{
newmm->uprobes_state.xol_area = NULL;
- if (test_bit(MMF_HAS_UPROBES, &oldmm->flags))
+ if (test_bit(MMF_HAS_UPROBES, &oldmm->flags)) {
set_bit(MMF_HAS_UPROBES, &newmm->flags);
+ /* unconditionally, dup_mmap() skips VM_DONTCOPY vmas */
+ set_bit(MMF_RECALC_UPROBES, &newmm->flags);
+ }
}
/*
--
1.5.5.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES
2012-08-19 16:40 ` [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES Oleg Nesterov
@ 2012-09-14 16:32 ` Srikar Dronamraju
0 siblings, 0 replies; 12+ messages in thread
From: Srikar Dronamraju @ 2012-09-14 16:32 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli,
Anton Arapov, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:40]:
> Add the new MMF_RECALC_UPROBES flag, it means that MMF_HAS_UPROBES
> can be false positive after remove_breakpoint() or uprobe_munmap().
> It is also set by uprobe_dup_mmap(), this is not optimal but simple.
> We could add the new hook, uprobe_dup_vma(), to set MMF_HAS_UPROBES
> only if the new mm actually has uprobes, but I don't think this
> makes sense.
>
> The next patch will use this flag to clear MMF_HAS_UPROBES.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
> include/linux/sched.h | 3 ++-
> kernel/events/uprobes.c | 39 +++++++++++++++++++++++++++++++++++----
> 2 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index c0fcfb7..d6899b0 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -441,7 +441,8 @@ extern int get_dumpable(struct mm_struct *mm);
> #define MMF_VM_HUGEPAGE 17 /* set when VM_HUGEPAGE is set on vma */
> #define MMF_EXE_FILE_CHANGED 18 /* see prctl_set_mm_exe_file() */
>
> -#define MMF_HAS_UPROBES 19 /* might have uprobes */
> +#define MMF_HAS_UPROBES 19 /* has uprobes */
> +#define MMF_RECALC_UPROBES 20 /* MMF_HAS_UPROBES can be wrong */
>
> #define MMF_INIT_MASK (MMF_DUMPABLE_MASK | MMF_DUMP_FILTER_MASK)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index abe077e..176de8c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -686,7 +686,9 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
> set_bit(MMF_HAS_UPROBES, &mm->flags);
>
> ret = set_swbp(&uprobe->arch, mm, vaddr);
> - if (ret && first_uprobe)
> + if (!ret)
> + clear_bit(MMF_RECALC_UPROBES, &mm->flags);
> + else if (first_uprobe)
> clear_bit(MMF_HAS_UPROBES, &mm->flags);
>
> return ret;
> @@ -695,6 +697,11 @@ install_breakpoint(struct uprobe *uprobe, struct mm_struct *mm,
> static void
> remove_breakpoint(struct uprobe *uprobe, struct mm_struct *mm, unsigned long vaddr)
> {
> + /* can happen if uprobe_register() fails */
> + if (!test_bit(MMF_HAS_UPROBES, &mm->flags))
> + return;
> +
> + set_bit(MMF_RECALC_UPROBES, &mm->flags);
> set_orig_insn(&uprobe->arch, mm, vaddr);
> }
>
> @@ -1027,6 +1034,25 @@ int uprobe_mmap(struct vm_area_struct *vma)
> return 0;
> }
>
> +static bool
> +vma_has_uprobes(struct vm_area_struct *vma, unsigned long start, unsigned long end)
> +{
> + loff_t min, max;
> + struct inode *inode;
> + struct rb_node *n;
> +
> + inode = vma->vm_file->f_mapping->host;
> +
> + min = vaddr_to_offset(vma, start);
> + max = min + (end - start) - 1;
> +
> + spin_lock(&uprobes_treelock);
> + n = find_node_in_range(inode, min, max);
> + spin_unlock(&uprobes_treelock);
> +
> + return !!n;
> +}
> +
> /*
> * Called in context of a munmap of a vma.
> */
> @@ -1038,10 +1064,12 @@ void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, unsigned lon
> if (!atomic_read(&vma->vm_mm->mm_users)) /* called by mmput() ? */
> return;
>
> - if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags))
> + if (!test_bit(MMF_HAS_UPROBES, &vma->vm_mm->flags) ||
> + test_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags))
> return;
>
> - /* TODO: unmapping uprobe(s) will need more work */
> + if (vma_has_uprobes(vma, start, end))
> + set_bit(MMF_RECALC_UPROBES, &vma->vm_mm->flags);
> }
>
> /* Slot allocation for XOL */
> @@ -1147,8 +1175,11 @@ void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
> {
> newmm->uprobes_state.xol_area = NULL;
>
> - if (test_bit(MMF_HAS_UPROBES, &oldmm->flags))
> + if (test_bit(MMF_HAS_UPROBES, &oldmm->flags)) {
> set_bit(MMF_HAS_UPROBES, &newmm->flags);
> + /* unconditionally, dup_mmap() skips VM_DONTCOPY vmas */
> + set_bit(MMF_RECALC_UPROBES, &newmm->flags);
> + }
> }
>
> /*
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES
2012-08-19 16:40 [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Oleg Nesterov
2012-08-19 16:40 ` [PATCH 1/3] uprobes: uprobes_treelock should not disable irqs Oleg Nesterov
2012-08-19 16:40 ` [PATCH 2/3] uprobes: introduce MMF_RECALC_UPROBES Oleg Nesterov
@ 2012-08-19 16:40 ` Oleg Nesterov
2012-09-14 16:43 ` Srikar Dronamraju
2012-08-27 17:44 ` [PATCH 0/3] uprobes: teach uprobes " Oleg Nesterov
2012-09-14 17:12 ` [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Srikar Dronamraju
4 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2012-08-19 16:40 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju
Cc: Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel
The wrong MMF_HAS_UPROBES doesn't really hurt, just it triggers
the "slow" and unnecessary handle_swbp() path if the task hits
the non-uprobe breakpoint.
So this patch changes find_active_uprobe() to check every valid
vma and clear MMF_HAS_UPROBES if no uprobes were found. This is
adds the slow O(n) path, but it is only called in unlikely case
when the task hits the normal breakpoint first time after
uprobe_unregister().
Note the "not strictly accurate" comment in mmf_recalc_uprobes().
We can fix this, we only need to teach vma_has_uprobes() to return
a bit more more info, but I am not sure this worth the trouble.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/events/uprobes.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 176de8c..0b7918c 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1397,6 +1397,25 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
return false;
}
+static void mmf_recalc_uprobes(struct mm_struct *mm)
+{
+ struct vm_area_struct *vma;
+
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (!valid_vma(vma, false))
+ continue;
+ /*
+ * This is not strictly accurate, we can race with
+ * uprobe_unregister() and see the already removed
+ * uprobe if delete_uprobe() was not yet called.
+ */
+ if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))
+ return;
+ }
+
+ clear_bit(MMF_HAS_UPROBES, &mm->flags);
+}
+
static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
{
struct mm_struct *mm = current->mm;
@@ -1418,6 +1437,9 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
} else {
*is_swbp = -EFAULT;
}
+
+ if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
+ mmf_recalc_uprobes(mm);
up_read(&mm->mmap_sem);
return uprobe;
--
1.5.5.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES
2012-08-19 16:40 ` [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES Oleg Nesterov
@ 2012-09-14 16:43 ` Srikar Dronamraju
2012-09-15 14:41 ` Oleg Nesterov
0 siblings, 1 reply; 12+ messages in thread
From: Srikar Dronamraju @ 2012-09-14 16:43 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli,
Anton Arapov, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:42]:
> The wrong MMF_HAS_UPROBES doesn't really hurt, just it triggers
> the "slow" and unnecessary handle_swbp() path if the task hits
> the non-uprobe breakpoint.
>
> So this patch changes find_active_uprobe() to check every valid
> vma and clear MMF_HAS_UPROBES if no uprobes were found. This is
> adds the slow O(n) path, but it is only called in unlikely case
> when the task hits the normal breakpoint first time after
> uprobe_unregister().
>
> Note the "not strictly accurate" comment in mmf_recalc_uprobes().
> We can fix this, we only need to teach vma_has_uprobes() to return
> a bit more more info, but I am not sure this worth the trouble.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/events/uprobes.c | 22 ++++++++++++++++++++++
> 1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 176de8c..0b7918c 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1397,6 +1397,25 @@ static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
> return false;
> }
>
> +static void mmf_recalc_uprobes(struct mm_struct *mm)
> +{
> + struct vm_area_struct *vma;
> +
> + for (vma = mm->mmap; vma; vma = vma->vm_next) {
> + if (!valid_vma(vma, false))
> + continue;
> + /*
> + * This is not strictly accurate, we can race with
> + * uprobe_unregister() and see the already removed
> + * uprobe if delete_uprobe() was not yet called.
> + */
> + if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))
Should we set the MMF_RECALC_UPROBES here?
Its harmless but my thought was if we indeed saw a uprobe that was
already deleted, then the next time we hit a non uprobe breakpoint in
the same process context, we will not come here because
MMF_RECALC_UPROBES is cleared.
The rest looks fine.
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> + return;
> + }
> +
> + clear_bit(MMF_HAS_UPROBES, &mm->flags);
> +}
> +
> static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> {
> struct mm_struct *mm = current->mm;
> @@ -1418,6 +1437,9 @@ static struct uprobe *find_active_uprobe(unsigned long bp_vaddr, int *is_swbp)
> } else {
> *is_swbp = -EFAULT;
> }
> +
> + if (!uprobe && test_and_clear_bit(MMF_RECALC_UPROBES, &mm->flags))
> + mmf_recalc_uprobes(mm);
> up_read(&mm->mmap_sem);
>
> return uprobe;
> --
> 1.5.5.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES
2012-09-14 16:43 ` Srikar Dronamraju
@ 2012-09-15 14:41 ` Oleg Nesterov
0 siblings, 0 replies; 12+ messages in thread
From: Oleg Nesterov @ 2012-09-15 14:41 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli,
Anton Arapov, linux-kernel
On 09/14, Srikar Dronamraju wrote:
>
> * Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:42]:
>
> > Note the "not strictly accurate" comment in mmf_recalc_uprobes().
> > We can fix this, we only need to teach vma_has_uprobes() to return
> > a bit more more info, but I am not sure this worth the trouble.
> ...
> > + /*
> > + * This is not strictly accurate, we can race with
> > + * uprobe_unregister() and see the already removed
> > + * uprobe if delete_uprobe() was not yet called.
> > + */
> > + if (vma_has_uprobes(vma, vma->vm_start, vma->vm_end))
>
> Should we set the MMF_RECALC_UPROBES here?
Assuming uprobe was removed - yes.
> Its harmless but my thought was if we indeed saw a uprobe that was
> already deleted, then the next time we hit a non uprobe breakpoint in
> the same process context, we will not come here because
> MMF_RECALC_UPROBES is cleared.
Yes, this is what the comment/changelog tries to document.
vma_has_uprobes() needs to check uprobe->consumers under uprobes_treelock
and return no|yes|maybe. Or it can do rb_prev/rb_entry until it finds the
live uprobe. We can do this later to improve this logic. And we can do
other improvements, say, fork() can be much more accurate but with more
complications.
> Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
Thanks,
Oleg.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES
2012-08-19 16:40 [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Oleg Nesterov
` (2 preceding siblings ...)
2012-08-19 16:40 ` [PATCH 3/3] uprobes: teach find_active_uprobe() to clear MMF_HAS_UPROBES Oleg Nesterov
@ 2012-08-27 17:44 ` Oleg Nesterov
2012-08-29 17:54 ` [GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES) Oleg Nesterov
2012-09-14 17:12 ` [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Srikar Dronamraju
4 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2012-08-27 17:44 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju
Cc: Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel,
Sebastian Andrzej Siewior
Srikar, this is still waiting for your review ;)
FYI, I created the tree to accumulate the acked patches,
kernel.org:/pub/scm/linux/kernel/git/oleg/misc
Currently it has:
Oleg Nesterov (7):
uprobes: kill uprobes_state->count
uprobes: kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap
uprobes: change uprobe_mmap() to ignore the errors but check fatal_signal_pending()
uprobes: do not use -EEXIST in install_breakpoint() paths
uprobes: introduce MMF_HAS_UPROBES
uprobes: fold uprobe_reset_state() into uprobe_dup_mmap()
uprobes: remove "verify" argument from set_orig_insn()
Sebastian Andrzej Siewior (1):
uprobes: remove check for uprobe variable in handle_swbp()
Srikar Dronamraju (1):
uprobes: Remove redundant lock_page/unlock_page
You didn't ack Sebastian's patch, but it looks simple. If you
have any objections please let me know.
On 08/19, Oleg Nesterov wrote:
>
> Hello.
>
> On top of "kill uprobes_state->count, add MMF_HAS_UPROBES"
> series.
>
> Once again, I am not sure we really need this (except 1/3).
> Perhaps we could remove uprobe_munmap() instead and forget
> about the false-positive MMF_HAS_UPROBES's.
>
> OTOH, imho it looks a bit more clean this way. Please comment.
>
> diffstat reports the code bloat, but note that the previous
> series removed more code.
>
> Oleg.
>
> include/linux/sched.h | 3 +-
> kernel/events/uprobes.c | 82 +++++++++++++++++++++++++++++++++++++----------
> 2 files changed, 67 insertions(+), 18 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* [GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES)
2012-08-27 17:44 ` [PATCH 0/3] uprobes: teach uprobes " Oleg Nesterov
@ 2012-08-29 17:54 ` Oleg Nesterov
2012-08-30 8:18 ` Ingo Molnar
0 siblings, 1 reply; 12+ messages in thread
From: Oleg Nesterov @ 2012-08-29 17:54 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju
Cc: Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel,
Sebastian Andrzej Siewior
On 08/27, Oleg Nesterov wrote:
>
> Srikar, this is still waiting for your review ;)
I was informed that Srikar is travelling and can't review this seris.
So I think it doesn't make sense to delay the already acked patches,
the more testing the better ;) Yes, uprobe_munmap() looks strange,
we should either remove it or apply this series. But we can do this
later, and uprobes have other problems.
Ingo, please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes
Note: the patch from Sebastian wasn't acked by Srikar, but I hope
this cleanup is simple enough.
Oleg Nesterov (7):
uprobes: Kill uprobes_state->count
uprobes: Kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap
uprobes: Change uprobe_mmap() to ignore the errors but check fatal_signal_pending()
uprobes: Do not use -EEXIST in install_breakpoint() paths
uprobes: Introduce MMF_HAS_UPROBES
uprobes: Fold uprobe_reset_state() into uprobe_dup_mmap()
uprobes: Remove "verify" argument from set_orig_insn()
Sebastian Andrzej Siewior (1):
uprobes: Remove check for uprobe variable in handle_swbp()
Srikar Dronamraju (1):
uprobes: Remove redundant lock_page/unlock_page
include/linux/sched.h | 2 +
include/linux/uprobes.h | 13 ++--
kernel/events/uprobes.c | 159 +++++++++++++----------------------------------
kernel/fork.c | 6 +--
4 files changed, 54 insertions(+), 126 deletions(-)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES)
2012-08-29 17:54 ` [GIT PULL] (Was: uprobes: teach uprobes to clear MMF_HAS_UPROBES) Oleg Nesterov
@ 2012-08-30 8:18 ` Ingo Molnar
0 siblings, 0 replies; 12+ messages in thread
From: Ingo Molnar @ 2012-08-30 8:18 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Peter Zijlstra, Srikar Dronamraju,
Ananth N Mavinakayanahalli, Anton Arapov, linux-kernel,
Sebastian Andrzej Siewior
* Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/27, Oleg Nesterov wrote:
> >
> > Srikar, this is still waiting for your review ;)
>
> I was informed that Srikar is travelling and can't review this seris.
>
> So I think it doesn't make sense to delay the already acked patches,
> the more testing the better ;) Yes, uprobe_munmap() looks strange,
> we should either remove it or apply this series. But we can do this
> later, and uprobes have other problems.
>
> Ingo, please pull from
>
> git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes
>
> Note: the patch from Sebastian wasn't acked by Srikar, but I hope
> this cleanup is simple enough.
>
> Oleg Nesterov (7):
> uprobes: Kill uprobes_state->count
> uprobes: Kill dup_mmap()->uprobe_mmap(), simplify uprobe_mmap/munmap
> uprobes: Change uprobe_mmap() to ignore the errors but check fatal_signal_pending()
> uprobes: Do not use -EEXIST in install_breakpoint() paths
> uprobes: Introduce MMF_HAS_UPROBES
> uprobes: Fold uprobe_reset_state() into uprobe_dup_mmap()
> uprobes: Remove "verify" argument from set_orig_insn()
>
> Sebastian Andrzej Siewior (1):
> uprobes: Remove check for uprobe variable in handle_swbp()
>
> Srikar Dronamraju (1):
> uprobes: Remove redundant lock_page/unlock_page
>
> include/linux/sched.h | 2 +
> include/linux/uprobes.h | 13 ++--
> kernel/events/uprobes.c | 159 +++++++++++++----------------------------------
> kernel/fork.c | 6 +--
> 4 files changed, 54 insertions(+), 126 deletions(-)
Pulled into tip:perf/core for v3.7, thanks a lot Oleg!
Ingo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES
2012-08-19 16:40 [PATCH 0/3] uprobes: teach uprobes to clear MMF_HAS_UPROBES Oleg Nesterov
` (3 preceding siblings ...)
2012-08-27 17:44 ` [PATCH 0/3] uprobes: teach uprobes " Oleg Nesterov
@ 2012-09-14 17:12 ` Srikar Dronamraju
4 siblings, 0 replies; 12+ messages in thread
From: Srikar Dronamraju @ 2012-09-14 17:12 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Ingo Molnar, Peter Zijlstra, Ananth N Mavinakayanahalli,
Anton Arapov, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2012-08-19 18:40:08]:
> Hello.
>
> On top of "kill uprobes_state->count, add MMF_HAS_UPROBES"
> series.
>
> Once again, I am not sure we really need this (except 1/3).
> Perhaps we could remove uprobe_munmap() instead and forget
> about the false-positive MMF_HAS_UPROBES's.
>
> OTOH, imho it looks a bit more clean this way. Please comment.
I am agree with the current approach.
The other approach could be to forget about MMF_RECALC_UPROBES and
uprobe_munmap() (essentially patch 2). But in patch 3 we will end up
calling mmf_recalc_uprobes() everytime a process with MM_HAS_UPROBES
hits a non-uprobe breakpoint.
>
> diffstat reports the code bloat, but note that the previous
> series removed more code.
>
> Oleg.
>
> include/linux/sched.h | 3 +-
> kernel/events/uprobes.c | 82 +++++++++++++++++++++++++++++++++++++----------
> 2 files changed, 67 insertions(+), 18 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread