[PATCH 0/3] uprobes: simplify _unregister paths

[PATCH 0/3] uprobes: simplify _unregister paths

[Oleg Nesterov]

On top of

	[PATCH v2 0/5] uprobes: misc cleanups/simplifications
	https://lore.kernel.org/all/20240729134444.GA12293@redhat.com/

I sent yesterday.

Oleg.
---

 kernel/events/uprobes.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

[PATCH 1/3] uprobes: change uprobe_register() to use uprobe_unregister() instead of __uprobe_unregister()

[Oleg Nesterov]

If register_for_each_vma() fails uprobe_register() can safely drop
uprobe->register_rwsem and use uprobe_unregister(). There is no worry
about the races with another register/unregister, consumer_add() was
already called so this case doesn't differ from _unregister() right
after the successful _register().

Yes this means the extra up_write() + down_write(), but this is the
slow and unlikely case anyway.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 974474680820..5ea0aabe8774 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1174,16 +1174,18 @@ struct uprobe *uprobe_register(struct inode *inode,
 	if (likely(uprobe_is_active(uprobe))) {
 		consumer_add(uprobe, uc);
 		ret = register_for_each_vma(uprobe, uc);
-		if (ret)
-			__uprobe_unregister(uprobe, uc);
 	}
 	up_write(&uprobe->register_rwsem);
 	put_uprobe(uprobe);
 
-	if (unlikely(ret == -EAGAIN))
-		goto retry;
+	if (ret) {
+		if (unlikely(ret == -EAGAIN))
+			goto retry;
+		uprobe_unregister(uprobe, uc);
+		return ERR_PTR(ret);
+	}
 
-	return ret ? ERR_PTR(ret) : uprobe;
+	return uprobe;
 }
 EXPORT_SYMBOL_GPL(uprobe_register);
 
-- 
2.25.1.362.g51ebf55

[PATCH 2/3] uprobes: fold __uprobe_unregister() into uprobe_unregister()

[Oleg Nesterov]

Fold __uprobe_unregister() into its single caller, uprobe_unregister().
A separate patch to simplify the next change.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 5ea0aabe8774..c06e1a5f1783 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1085,20 +1085,6 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
 	return err;
 }
 
-static void
-__uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
-{
-	int err;
-
-	if (WARN_ON(!consumer_del(uprobe, uc)))
-		return;
-
-	err = register_for_each_vma(uprobe, NULL);
-	/* TODO : cant unregister? schedule a worker thread */
-	if (!uprobe->consumers && !err)
-		delete_uprobe(uprobe);
-}
-
 /**
  * uprobe_unregister - unregister an already registered probe.
  * @uprobe: uprobe to remove
@@ -1106,9 +1092,18 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
  */
 void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
+	int err;
+
 	get_uprobe(uprobe);
 	down_write(&uprobe->register_rwsem);
-	__uprobe_unregister(uprobe, uc);
+	if (WARN_ON(!consumer_del(uprobe, uc)))
+		err = -ENOENT;
+	else
+		err = register_for_each_vma(uprobe, NULL);
+
+	/* TODO : cant unregister? schedule a worker thread */
+	if (!err && !uprobe->consumers)
+		delete_uprobe(uprobe);
 	up_write(&uprobe->register_rwsem);
 	put_uprobe(uprobe);
 }
-- 
2.25.1.362.g51ebf55

[PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Oleg Nesterov]

Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and
move the possibly final put_uprobe() from delete_uprobe() to its only
caller, uprobe_unregister().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/events/uprobes.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index c06e1a5f1783..f88b7ff20587 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe)
 	rb_erase(&uprobe->rb_node, &uprobes_tree);
 	write_unlock(&uprobes_treelock);
 	RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */
-	put_uprobe(uprobe);
 }
 
 struct map_info {
@@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 {
 	int err;
 
-	get_uprobe(uprobe);
 	down_write(&uprobe->register_rwsem);
 	if (WARN_ON(!consumer_del(uprobe, uc)))
 		err = -ENOENT;
@@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
 		err = register_for_each_vma(uprobe, NULL);
 
 	/* TODO : cant unregister? schedule a worker thread */
-	if (!err && !uprobe->consumers)
-		delete_uprobe(uprobe);
+	if (!err) {
+		if (!uprobe->consumers)
+			delete_uprobe(uprobe);
+		else
+			err = -EBUSY;
+	}
 	up_write(&uprobe->register_rwsem);
-	put_uprobe(uprobe);
+
+	if (!err)
+		put_uprobe(uprobe);
 }
 EXPORT_SYMBOL_GPL(uprobe_unregister);
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH 1/3] uprobes: change uprobe_register() to use uprobe_unregister() instead of __uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 5:34 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> If register_for_each_vma() fails uprobe_register() can safely drop
> uprobe->register_rwsem and use uprobe_unregister(). There is no worry
> about the races with another register/unregister, consumer_add() was
> already called so this case doesn't differ from _unregister() right
> after the successful _register().
>
> Yes this means the extra up_write() + down_write(), but this is the
> slow and unlikely case anyway.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>

Yep, makes total sense, in my local patches I basically already done
that as well.

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 974474680820..5ea0aabe8774 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1174,16 +1174,18 @@ struct uprobe *uprobe_register(struct inode *inode,
>         if (likely(uprobe_is_active(uprobe))) {
>                 consumer_add(uprobe, uc);
>                 ret = register_for_each_vma(uprobe, uc);
> -               if (ret)
> -                       __uprobe_unregister(uprobe, uc);
>         }
>         up_write(&uprobe->register_rwsem);
>         put_uprobe(uprobe);
>
> -       if (unlikely(ret == -EAGAIN))
> -               goto retry;
> +       if (ret) {
> +               if (unlikely(ret == -EAGAIN))
> +                       goto retry;
> +               uprobe_unregister(uprobe, uc);
> +               return ERR_PTR(ret);
> +       }
>
> -       return ret ? ERR_PTR(ret) : uprobe;
> +       return uprobe;
>  }
>  EXPORT_SYMBOL_GPL(uprobe_register);
>
> --
> 2.25.1.362.g51ebf55
>

Re: [PATCH 2/3] uprobes: fold __uprobe_unregister() into uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 5:35 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Fold __uprobe_unregister() into its single caller, uprobe_unregister().
> A separate patch to simplify the next change.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>

LGTM

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 5ea0aabe8774..c06e1a5f1783 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1085,20 +1085,6 @@ register_for_each_vma(struct uprobe *uprobe, struct uprobe_consumer *new)
>         return err;
>  }
>
> -static void
> -__uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> -{
> -       int err;
> -
> -       if (WARN_ON(!consumer_del(uprobe, uc)))
> -               return;
> -
> -       err = register_for_each_vma(uprobe, NULL);
> -       /* TODO : cant unregister? schedule a worker thread */
> -       if (!uprobe->consumers && !err)
> -               delete_uprobe(uprobe);
> -}
> -
>  /**
>   * uprobe_unregister - unregister an already registered probe.
>   * @uprobe: uprobe to remove
> @@ -1106,9 +1092,18 @@ __uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>   */
>  void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
> +       int err;
> +
>         get_uprobe(uprobe);
>         down_write(&uprobe->register_rwsem);
> -       __uprobe_unregister(uprobe, uc);
> +       if (WARN_ON(!consumer_del(uprobe, uc)))
> +               err = -ENOENT;
> +       else
> +               err = register_for_each_vma(uprobe, NULL);
> +
> +       /* TODO : cant unregister? schedule a worker thread */
> +       if (!err && !uprobe->consumers)
> +               delete_uprobe(uprobe);
>         up_write(&uprobe->register_rwsem);
>         put_uprobe(uprobe);
>  }
> --
> 2.25.1.362.g51ebf55
>

Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 5:35 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Kill the extra get_uprobe() + put_uprobe() in uprobe_unregister() and
> move the possibly final put_uprobe() from delete_uprobe() to its only
> caller, uprobe_unregister().
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/events/uprobes.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>

LGTM as well.

BTW, do you have anything against me changing refcounting so that
uprobes_tree itself doesn't hold a refcount, and all the refcounting
is done based on consumers holding implicit refcount and whatever
temporary get/put uprobe is necessary for runtime uprobe/uretprobe
functioning.

I can do that with a simple refcount_t and refcount_inc_not_zero(), no
fancy custom refcounting. This schema makes most sense to me, it will
also simplify registration by avoiding that annoying
is_uprobe_active() check + retry. Thoughts?

All that would be additional on top of your change. BTW, do you plan
any more clean ups like this? It's a bit of a moving target because of
your refactoring, so I'd appreciate some stability to build upon :)

Also, can you please push this and your previous patch set into some
branch somewhere I can pull from, preferably based on the latest
linux-trace's probes/for-next? Thanks!

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index c06e1a5f1783..f88b7ff20587 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -939,7 +939,6 @@ static void delete_uprobe(struct uprobe *uprobe)
>         rb_erase(&uprobe->rb_node, &uprobes_tree);
>         write_unlock(&uprobes_treelock);
>         RB_CLEAR_NODE(&uprobe->rb_node); /* for uprobe_is_active() */
> -       put_uprobe(uprobe);
>  }
>
>  struct map_info {
> @@ -1094,7 +1093,6 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>  {
>         int err;
>
> -       get_uprobe(uprobe);
>         down_write(&uprobe->register_rwsem);
>         if (WARN_ON(!consumer_del(uprobe, uc)))
>                 err = -ENOENT;
> @@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
>                 err = register_for_each_vma(uprobe, NULL);
>
>         /* TODO : cant unregister? schedule a worker thread */
> -       if (!err && !uprobe->consumers)
> -               delete_uprobe(uprobe);
> +       if (!err) {
> +               if (!uprobe->consumers)
> +                       delete_uprobe(uprobe);
> +               else
> +                       err = -EBUSY;

This bit is weird because really it's not an error... but this makes
this change simpler and moves put_uprobe outside of rwsem. With my
proposed change to refcounting schema this would be unnecessary, but
let's see what you think.

> +       }
>         up_write(&uprobe->register_rwsem);
> -       put_uprobe(uprobe);
> +
> +       if (!err)
> +               put_uprobe(uprobe);
>  }
>  EXPORT_SYMBOL_GPL(uprobe_unregister);
>
> --
> 2.25.1.362.g51ebf55
>

Re: [PATCH 0/3] uprobes: simplify _unregister paths

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 5:34 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On top of
>
>         [PATCH v2 0/5] uprobes: misc cleanups/simplifications
>         https://lore.kernel.org/all/20240729134444.GA12293@redhat.com/
>
> I sent yesterday.
>
> Oleg.
> ---
>

Both patch sets look good to me. It would be nice to get them applied
ASAP to have a stable uprobes code base to work on. Rebasing is
painful and error-prone.

>  kernel/events/uprobes.c | 47 ++++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
>

Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Oleg Nesterov]

Thanks for looking at this!

On 07/30, Andrii Nakryiko wrote:
>
> BTW, do you have anything against me changing refcounting so that
> uprobes_tree itself doesn't hold a refcount, and all the refcounting
> is done based on consumers holding implicit refcount and whatever
> temporary get/put uprobe is necessary for runtime uprobe/uretprobe
> functioning.

No, I have nothing against.

To be honest, I don't really understand the value of this change, but
a) this is probably because I didn't see a separate patch(es) which
does this and b) assuming that it doesn't complicate the code too much
I won't argue in any case ;)

And in fact I had your proposed change in mind when I did these cleanups.
I think that they can even simplify this change, at least I hope they can
not complicate it.

> BTW, do you plan
> any more clean ups like this? It's a bit of a moving target because of
> your refactoring, so I'd appreciate some stability to build upon :)

Well yes... may be this week.

I'd like to (try to) optimize/de-uglify register_for_each_vma() for
the multiple-consumers case. And, more importantly, the case when perf
does uprobe_register() + uprobe_apply().

But. All these changes will only touch the register_for_each_vma() paths,
so this is completely orthogonal to this series and your and/or Peter's
changes.

> Also, can you please push this and your previous patch set into some
> branch somewhere I can pull from, preferably based on the latest
> linux-trace's probes/for-next? Thanks!

Cough ;)

No, sorry, I can't. I lost my kernel.org account years ago (and this is
the second time this has happened!), but since I am a lazy dog I didn't
even bother to try to restore it.

> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks!

> > @@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> >                 err = register_for_each_vma(uprobe, NULL);
> >
> >         /* TODO : cant unregister? schedule a worker thread */
> > -       if (!err && !uprobe->consumers)
> > -               delete_uprobe(uprobe);
> > +       if (!err) {
> > +               if (!uprobe->consumers)
> > +                       delete_uprobe(uprobe);
> > +               else
> > +                       err = -EBUSY;
>
> This bit is weird because really it's not an error... but this makes
> this change simpler and moves put_uprobe outside of rwsem.

Agreed, uprobe->consumers != NULL is not an error. But we don't return
this error code, just we need to ensure that "err == 0" means that
"delete_uprobe() was actually called".

> With my
> proposed change to refcounting schema this would be unnecessary,

Yes.

Oleg.

Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 10:17 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Thanks for looking at this!
>
> On 07/30, Andrii Nakryiko wrote:
> >
> > BTW, do you have anything against me changing refcounting so that
> > uprobes_tree itself doesn't hold a refcount, and all the refcounting
> > is done based on consumers holding implicit refcount and whatever
> > temporary get/put uprobe is necessary for runtime uprobe/uretprobe
> > functioning.
>
> No, I have nothing against.
>
> To be honest, I don't really understand the value of this change, but
> a) this is probably because I didn't see a separate patch(es) which
> does this and b) assuming that it doesn't complicate the code too much
> I won't argue in any case ;)
>
> And in fact I had your proposed change in mind when I did these cleanups.
> I think that they can even simplify this change, at least I hope they can
> not complicate it.

I just find this logic to drop refcount if the consumer list is empty
super confusing and hard to intuitively reason about. It's just very
unconventional and seems problematic every time I think about this.

Either way, we can discuss once you see the code, it's not really
complicated if I stick to refcount_t instead of my fancy atomic-based
refcounting.

>
> > BTW, do you plan
> > any more clean ups like this? It's a bit of a moving target because of
> > your refactoring, so I'd appreciate some stability to build upon :)
>
> Well yes... may be this week.
>
> I'd like to (try to) optimize/de-uglify register_for_each_vma() for
> the multiple-consumers case. And, more importantly, the case when perf
> does uprobe_register() + uprobe_apply().
>
> But. All these changes will only touch the register_for_each_vma() paths,
> so this is completely orthogonal to this series and your and/or Peter's
> changes.
>

Ok, sounds good, it would be nice to keep the other part more or less
intact for the time being. Thanks!

> > Also, can you please push this and your previous patch set into some
> > branch somewhere I can pull from, preferably based on the latest
> > linux-trace's probes/for-next? Thanks!
>
> Cough ;)
>
> No, sorry, I can't. I lost my kernel.org account years ago (and this is
> the second time this has happened!), but since I am a lazy dog I didn't
> even bother to try to restore it.

It doesn't have to be kernel.org repo :-P Github would work just fine,
but ok, if it's too much trouble I'll just go download emails one by
one and apply them locally.

>
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> Thanks!
>
> > > @@ -1102,10 +1100,16 @@ void uprobe_unregister(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > >                 err = register_for_each_vma(uprobe, NULL);
> > >
> > >         /* TODO : cant unregister? schedule a worker thread */
> > > -       if (!err && !uprobe->consumers)
> > > -               delete_uprobe(uprobe);
> > > +       if (!err) {
> > > +               if (!uprobe->consumers)
> > > +                       delete_uprobe(uprobe);
> > > +               else
> > > +                       err = -EBUSY;
> >
> > This bit is weird because really it's not an error... but this makes
> > this change simpler and moves put_uprobe outside of rwsem.
>
> Agreed, uprobe->consumers != NULL is not an error. But we don't return
> this error code, just we need to ensure that "err == 0" means that
> "delete_uprobe() was actually called".
>

yep

> > With my
> > proposed change to refcounting schema this would be unnecessary,
>
> Yes.
>
> Oleg.
>

Re: [PATCH 0/3] uprobes: simplify _unregister paths

[Jiri Olsa]

On Tue, Jul 30, 2024 at 08:10:27AM -0700, Andrii Nakryiko wrote:
> On Tue, Jul 30, 2024 at 5:34 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On top of
> >
> >         [PATCH v2 0/5] uprobes: misc cleanups/simplifications
> >         https://lore.kernel.org/all/20240729134444.GA12293@redhat.com/
> >
> > I sent yesterday.
> >
> > Oleg.
> > ---
> >
> 
> Both patch sets look good to me. It would be nice to get them applied
> ASAP to have a stable uprobes code base to work on. Rebasing is
> painful and error-prone.

lgtm, +1 for having them applied soon

Reviewed-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Peter Zijlstra]

On Tue, Jul 30, 2024 at 08:08:49AM -0700, Andrii Nakryiko wrote:
> Also, can you please push this and your previous patch set into some
> branch somewhere I can pull from, preferably based on the latest
> linux-trace's probes/for-next? Thanks!

I can stick then in tip/perf/core if you want.

Re: [PATCH 3/3] uprobes: shift put_uprobe() from delete_uprobe() to uprobe_unregister()

[Andrii Nakryiko]

On Tue, Jul 30, 2024 at 1:55 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Jul 30, 2024 at 08:08:49AM -0700, Andrii Nakryiko wrote:
> > Also, can you please push this and your previous patch set into some
> > branch somewhere I can pull from, preferably based on the latest
> > linux-trace's probes/for-next? Thanks!
>
> I can stick then in tip/perf/core if you want.

Whichever tree you and Masami ultimately agreed on, I don't
particularly care. But yeah, this would be great, thanks!

Re: [PATCH 0/3] uprobes: simplify _unregister paths

[Masami Hiramatsu]

On Tue, 30 Jul 2024 14:34:21 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On top of
> 
> 	[PATCH v2 0/5] uprobes: misc cleanups/simplifications
> 	https://lore.kernel.org/all/20240729134444.GA12293@redhat.com/
> 
> I sent yesterday.
> 

OK, this series looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks,

> Oleg.
> ---
> 
>  kernel/events/uprobes.c | 47 ++++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>