* [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
@ 2013-11-09 19:03 Oleg Nesterov
2013-11-11 7:15 ` Srikar Dronamraju
2013-11-11 8:41 ` Ingo Molnar
0 siblings, 2 replies; 8+ messages in thread
From: Oleg Nesterov @ 2013-11-09 19:03 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Srikar Dronamraju, linux-kernel
1. Don't include asm/uprobes.h unconditionally, we only need
it if CONFIG_UPROBES.
2. Move the definition of "struct xol_area" into uprobes.c.
Perhaps we should simply kill struct uprobes_state, it buys
nothing.
3. Kill the dummy definition of uprobe_get_swbp_addr(), nobody
except handle_swbp() needs it.
4. Purely cosmetic, but move the decl of uprobe_get_swbp_addr()
up, close to other __weak helpers.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/uprobes.h | 31 ++++---------------------------
kernel/events/uprobes.c | 19 +++++++++++++++++++
2 files changed, 23 insertions(+), 27 deletions(-)
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index 366b8b2..fe0c0bb 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -33,10 +33,6 @@ struct mm_struct;
struct inode;
struct notifier_block;
-#ifdef CONFIG_ARCH_SUPPORTS_UPROBES
-# include <asm/uprobes.h>
-#endif
-
#define UPROBE_HANDLER_REMOVE 1
#define UPROBE_HANDLER_MASK 1
@@ -61,6 +57,8 @@ struct uprobe_consumer {
};
#ifdef CONFIG_UPROBES
+#include <asm/uprobes.h>
+
enum uprobe_task_state {
UTASK_RUNNING,
UTASK_SSTEP,
@@ -93,24 +91,7 @@ struct uprobe_task {
unsigned int depth;
};
-/*
- * On a breakpoint hit, thread contests for a slot. It frees the
- * slot after singlestep. Currently a fixed number of slots are
- * allocated.
- */
-struct xol_area {
- wait_queue_head_t wq; /* if all slots are busy */
- atomic_t slot_count; /* number of in-use slots */
- unsigned long *bitmap; /* 0 = free slot */
- struct page *page;
-
- /*
- * We keep the vma's vm_start rather than a pointer to the vma
- * itself. The probed process or a naughty kernel module could make
- * the vma go away, and we must handle that reasonably gracefully.
- */
- unsigned long vaddr; /* Page(s) of instruction slots */
-};
+struct xol_area;
struct uprobes_state {
struct xol_area *xol_area;
@@ -120,6 +101,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsign
extern int __weak set_orig_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long vaddr);
extern bool __weak is_swbp_insn(uprobe_opcode_t *insn);
extern bool __weak is_trap_insn(uprobe_opcode_t *insn);
+extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
extern int uprobe_write_opcode(struct mm_struct *mm, unsigned long vaddr, uprobe_opcode_t);
extern int uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer *uc);
extern int uprobe_apply(struct inode *inode, loff_t offset, struct uprobe_consumer *uc, bool);
@@ -131,7 +113,6 @@ extern void uprobe_end_dup_mmap(void);
extern void uprobe_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm);
extern void uprobe_free_utask(struct task_struct *t);
extern void uprobe_copy_process(struct task_struct *t, unsigned long flags);
-extern unsigned long __weak uprobe_get_swbp_addr(struct pt_regs *regs);
extern int uprobe_post_sstep_notifier(struct pt_regs *regs);
extern int uprobe_pre_sstep_notifier(struct pt_regs *regs);
extern void uprobe_notify_resume(struct pt_regs *regs);
@@ -187,10 +168,6 @@ static inline bool uprobe_deny_signal(void)
{
return false;
}
-static inline unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)
-{
- return 0;
-}
static inline void uprobe_free_utask(struct task_struct *t)
{
}
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index acc0317..a7239eb 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -86,6 +86,25 @@ struct return_instance {
};
/*
+ * On a breakpoint hit, thread contests for a slot. It frees the
+ * slot after singlestep. Currently a fixed number of slots are
+ * allocated.
+ */
+struct xol_area {
+ wait_queue_head_t wq; /* if all slots are busy */
+ atomic_t slot_count; /* number of in-use slots */
+ unsigned long *bitmap; /* 0 = free slot */
+ struct page *page;
+
+ /*
+ * We keep the vma's vm_start rather than a pointer to the vma
+ * itself. The probed process or a naughty kernel module could make
+ * the vma go away, and we must handle that reasonably gracefully.
+ */
+ unsigned long vaddr; /* Page(s) of instruction slots */
+};
+
+/*
* valid_vma: Verify if the specified vma is an executable vma
* Relax restrictions while unregistering: vm_flags might have
* changed after breakpoint was inserted.
--
1.5.5.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
2013-11-09 19:03 [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area Oleg Nesterov
@ 2013-11-11 7:15 ` Srikar Dronamraju
2013-11-11 8:41 ` Ingo Molnar
1 sibling, 0 replies; 8+ messages in thread
From: Srikar Dronamraju @ 2013-11-11 7:15 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, linux-kernel
* Oleg Nesterov <oleg@redhat.com> [2013-11-09 20:03:44]:
> 1. Don't include asm/uprobes.h unconditionally, we only need
> it if CONFIG_UPROBES.
>
> 2. Move the definition of "struct xol_area" into uprobes.c.
>
> Perhaps we should simply kill struct uprobes_state, it buys
> nothing.
>
> 3. Kill the dummy definition of uprobe_get_swbp_addr(), nobody
> except handle_swbp() needs it.
>
> 4. Purely cosmetic, but move the decl of uprobe_get_swbp_addr()
> up, close to other __weak helpers.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
2013-11-09 19:03 [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area Oleg Nesterov
2013-11-11 7:15 ` Srikar Dronamraju
@ 2013-11-11 8:41 ` Ingo Molnar
2013-11-11 19:58 ` Oleg Nesterov
1 sibling, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2013-11-11 8:41 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, Srikar Dronamraju, linux-kernel
* Oleg Nesterov <oleg@redhat.com> wrote:
> +++ b/kernel/events/uprobes.c
> @@ -86,6 +86,25 @@ struct return_instance {
> };
>
> /*
> + * On a breakpoint hit, thread contests for a slot. It frees the
> + * slot after singlestep. Currently a fixed number of slots are
> + * allocated.
> + */
> +struct xol_area {
So, my main complaint about the uprobes code isn't functional but
documentational, similar to what I outlined a few days ago: what this
comment does not explain is exactly what a 'XOL area' is.
You guys are changing code that reads like gobbledygook to people reading
it for the first time. It's understandable that you want to use
abbreviations and I don't object against that, but please explain key
concepts and data structures when they first come up - a very good place
to do that is in places where key structures are declared.
I didn't find any high level description of the XOL code, one which makes
clear that how we manage these out of line execution areas:
comet:~/tip> git grep -i 'out of line' $(find . -name '*uprobe*.[ch]')
arch/powerpc/kernel/uprobes.c: * arch_uprobe_pre_xol - prepare to execute out of line.
arch/x86/kernel/uprobes.c: * arch_uprobe_pre_xol - prepare to execute out of line.
arch/x86/kernel/uprobes.c: * address pushed by a call instruction executed out of line.
kernel/events/uprobes.c: * This area will be used for storing instructions for execution out of line.
kernel/events/uprobes.c:/* Prepare to single-step probed instruction out of line. */
The one that comes closest is:
* This area will be used for storing instructions for execution out of line.
... but that is a single sentence and deep inside the XOL code already.
Really, please make a better job of introducing other kernel hackers to
the code you are writing ...
Maybe even split the XOL code out into kernel/events/uprobes_xol.c or so?
That will give a natural place to explain yourselves at the beginning of
the file.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
2013-11-11 8:41 ` Ingo Molnar
@ 2013-11-11 19:58 ` Oleg Nesterov
2013-11-11 21:03 ` Ingo Molnar
0 siblings, 1 reply; 8+ messages in thread
From: Oleg Nesterov @ 2013-11-11 19:58 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Ingo Molnar, Srikar Dronamraju, linux-kernel
On 11/11, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > +++ b/kernel/events/uprobes.c
> > @@ -86,6 +86,25 @@ struct return_instance {
> > };
> >
> > /*
> > + * On a breakpoint hit, thread contests for a slot. It frees the
> > + * slot after singlestep. Currently a fixed number of slots are
> > + * allocated.
> > + */
> > +struct xol_area {
>
> So, my main complaint about the uprobes code isn't functional but
> documentational, similar to what I outlined a few days ago: what this
> comment does not explain is exactly what a 'XOL area' is.
>
> You guys are changing code that reads like gobbledygook to people reading
> it for the first time.
Not that I am trying to defense uprobes, but this is equally true for
any piece of kernel code, at least to me ;)
> It's understandable that you want to use
> abbreviations and I don't object against that, but please explain key
> concepts and data structures when they first come up
Well, this patch only move the definition with the comments, but:
> - a very good place
> to do that is in places where key structures are declared.
>
> I didn't find any high level description of the XOL code, one which makes
> clear that how we manage these out of line execution areas:
I have to agree, all these comments do not really help...
> The one that comes closest is:
>
> * This area will be used for storing instructions for execution out of line.
>
> ... but that is a single sentence and deep inside the XOL code already.
and even this comment should be probably moved up to the "struct xol_area",
> Really, please make a better job of introducing other kernel hackers to
> the code you are writing ...
>
> Maybe even split the XOL code out into kernel/events/uprobes_xol.c or so?
I do not really think a separate uprobes_xol.c makes sense. I think it would
be nice to have the high-level "uprobes design" doc in uprobetracer.txt, or
> That will give a natural place to explain yourselves at the beginning of
> the file.
or even in the beginning of uprobes.c, I agree.
Don't get me wrong, I am not volunteering ;) But at least I'll try to
pay more attention to the comments when I change the code next time.
Oleg.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area
2013-11-11 19:58 ` Oleg Nesterov
@ 2013-11-11 21:03 ` Ingo Molnar
2013-11-19 16:25 ` [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol Oleg Nesterov
0 siblings, 1 reply; 8+ messages in thread
From: Ingo Molnar @ 2013-11-11 21:03 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, Srikar Dronamraju, linux-kernel
* Oleg Nesterov <oleg@redhat.com> wrote:
> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > +++ b/kernel/events/uprobes.c
> > > @@ -86,6 +86,25 @@ struct return_instance {
> > > };
> > >
> > > /*
> > > + * On a breakpoint hit, thread contests for a slot. It frees the
> > > + * slot after singlestep. Currently a fixed number of slots are
> > > + * allocated.
> > > + */
> > > +struct xol_area {
> >
> > So, my main complaint about the uprobes code isn't functional but
> > documentational, similar to what I outlined a few days ago: what this
> > comment does not explain is exactly what a 'XOL area' is.
> >
> > You guys are changing code that reads like gobbledygook to people
> > reading it for the first time.
>
> Not that I am trying to defense uprobes, but this is equally true for
> any piece of kernel code, at least to me ;)
I'm really not suggesting to do overly much - only for some minimal blurb
like the scheduler has in most places:
/*
* This is the main, per-CPU runqueue data structure.
*
* Locking rule: those places that want to lock multiple runqueues
* (such as the load balancing or the thread migration code), lock
* acquire operations must be ordered by ascending &runqueue.
*/
struct rq {
/* runqueue lock: */
raw_spinlock_t lock;
But the apparent assumption that the reader knows what 'XOL' means
triggered my suggest :-)
> > Maybe even split the XOL code out into kernel/events/uprobes_xol.c or
> > so?
>
> I do not really think a separate uprobes_xol.c makes sense. [...]
Ok - it's your call really.
> [...] I think it would be nice to have the high-level "uprobes design"
> doc in uprobetracer.txt, or
Even better if the best parts are integrated into the source code!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
2013-11-11 21:03 ` Ingo Molnar
@ 2013-11-19 16:25 ` Oleg Nesterov
2013-11-19 19:24 ` Ingo Molnar
2013-11-20 11:03 ` Srikar Dronamraju
0 siblings, 2 replies; 8+ messages in thread
From: Oleg Nesterov @ 2013-11-19 16:25 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Ingo Molnar, Srikar Dronamraju, linux-kernel
On 11/11, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 11/11, Ingo Molnar wrote:
> > >
> > > You guys are changing code that reads like gobbledygook to people
> > > reading it for the first time.
> >
> > Not that I am trying to defense uprobes, but this is equally true for
> > any piece of kernel code, at least to me ;)
>
> I'm really not suggesting to do overly much - only for some minimal blurb
> like the scheduler has in most places:
OK. Let me try to make a first step to improve this a little bit...
How about the patch below? Srikar?
Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
Document xol_area and arch_uprobe.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/events/uprobes.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 51a7f53..b886a5e 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -73,6 +73,17 @@ struct uprobe {
struct inode *inode; /* Also hold a ref to inode */
loff_t offset;
unsigned long flags;
+
+ /*
+ * The generic code assumes that it has two members of unknown type
+ * owned by the arch-specific code:
+ *
+ * insn - copy_insn() saves the original instruction here for
+ * arch_uprobe_analyze_insn().
+ *
+ * ixol - potentially modified instruction to execute out of
+ * line, copied to xol_area by xol_get_insn_slot().
+ */
struct arch_uprobe arch;
};
@@ -86,6 +97,10 @@ struct return_instance {
};
/*
+ * Execute out of line area: anonymous executable mapping installed
+ * by the probed task to execute the copy of the original instruction
+ * mangled by set_swbp().
+ *
* On a breakpoint hit, thread contests for a slot. It frees the
* slot after singlestep. Currently a fixed number of slots are
* allocated.
--
1.5.5.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
2013-11-19 16:25 ` [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol Oleg Nesterov
@ 2013-11-19 19:24 ` Ingo Molnar
2013-11-20 11:03 ` Srikar Dronamraju
1 sibling, 0 replies; 8+ messages in thread
From: Ingo Molnar @ 2013-11-19 19:24 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, Srikar Dronamraju, linux-kernel
* Oleg Nesterov <oleg@redhat.com> wrote:
> On 11/11, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > On 11/11, Ingo Molnar wrote:
> > > >
> > > > You guys are changing code that reads like gobbledygook to people
> > > > reading it for the first time.
> > >
> > > Not that I am trying to defense uprobes, but this is equally true for
> > > any piece of kernel code, at least to me ;)
> >
> > I'm really not suggesting to do overly much - only for some minimal blurb
> > like the scheduler has in most places:
>
> OK. Let me try to make a first step to improve this a little bit...
>
> How about the patch below? Srikar?
>
>
> Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
>
> Document xol_area and arch_uprobe.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/events/uprobes.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 51a7f53..b886a5e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -73,6 +73,17 @@ struct uprobe {
> struct inode *inode; /* Also hold a ref to inode */
> loff_t offset;
> unsigned long flags;
> +
> + /*
> + * The generic code assumes that it has two members of unknown type
> + * owned by the arch-specific code:
> + *
> + * insn - copy_insn() saves the original instruction here for
> + * arch_uprobe_analyze_insn().
> + *
> + * ixol - potentially modified instruction to execute out of
> + * line, copied to xol_area by xol_get_insn_slot().
> + */
> struct arch_uprobe arch;
> };
>
> @@ -86,6 +97,10 @@ struct return_instance {
> };
>
> /*
> + * Execute out of line area: anonymous executable mapping installed
> + * by the probed task to execute the copy of the original instruction
> + * mangled by set_swbp().
> + *
> * On a breakpoint hit, thread contests for a slot. It frees the
> * slot after singlestep. Currently a fixed number of slots are
> * allocated.
Looks perfect to me!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
2013-11-19 16:25 ` [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol Oleg Nesterov
2013-11-19 19:24 ` Ingo Molnar
@ 2013-11-20 11:03 ` Srikar Dronamraju
1 sibling, 0 replies; 8+ messages in thread
From: Srikar Dronamraju @ 2013-11-20 11:03 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Ingo Molnar, Ingo Molnar, linux-kernel
>
> OK. Let me try to make a first step to improve this a little bit...
>
> How about the patch below? Srikar?
>
>
> Subject: [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol
>
> Document xol_area and arch_uprobe.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
Acked-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> kernel/events/uprobes.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 51a7f53..b886a5e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -73,6 +73,17 @@ struct uprobe {
> struct inode *inode; /* Also hold a ref to inode */
> loff_t offset;
> unsigned long flags;
> +
> + /*
> + * The generic code assumes that it has two members of unknown type
> + * owned by the arch-specific code:
> + *
> + * insn - copy_insn() saves the original instruction here for
> + * arch_uprobe_analyze_insn().
> + *
> + * ixol - potentially modified instruction to execute out of
> + * line, copied to xol_area by xol_get_insn_slot().
> + */
> struct arch_uprobe arch;
> };
>
> @@ -86,6 +97,10 @@ struct return_instance {
> };
>
> /*
> + * Execute out of line area: anonymous executable mapping installed
> + * by the probed task to execute the copy of the original instruction
> + * mangled by set_swbp().
> + *
> * On a breakpoint hit, thread contests for a slot. It frees the
> * slot after singlestep. Currently a fixed number of slots are
> * allocated.
> --
> 1.5.5.1
>
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-20 11:03 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-09 19:03 [PATCH] uprobes: Cleanup !CONFIG_UPROBES decls, unexport xol_area Oleg Nesterov
2013-11-11 7:15 ` Srikar Dronamraju
2013-11-11 8:41 ` Ingo Molnar
2013-11-11 19:58 ` Oleg Nesterov
2013-11-11 21:03 ` Ingo Molnar
2013-11-19 16:25 ` [PATCH] uprobes: Document xol_area and arch_uprobe->insn/ixol Oleg Nesterov
2013-11-19 19:24 ` Ingo Molnar
2013-11-20 11:03 ` Srikar Dronamraju
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.