Re: [PATCHv2 bpf-next 1/9] uprobe: Add support for session consumer

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Jul 03, 2024 at 05:31:32PM +0200, Jiri Olsa wrote:
> On Tue, Jul 02, 2024 at 01:52:38PM -0700, Andrii Nakryiko wrote:
> > On Tue, Jul 2, 2024 at 9:11 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Tue, Jul 02, 2024 at 03:04:08PM +0200, Peter Zijlstra wrote:
> > > > On Mon, Jul 01, 2024 at 06:41:07PM +0200, Jiri Olsa wrote:
> > > >
> > > > > +static void
> > > > > +uprobe_consumer_account(struct uprobe *uprobe, struct uprobe_consumer *uc)
> > > > > +{
> > > > > +   static unsigned int session_id;
> > > > > +
> > > > > +   if (uc->session) {
> > > > > +           uprobe->sessions_cnt++;
> > > > > +           uc->session_id = ++session_id ?: ++session_id;
> > > > > +   }
> > > > > +}
> > > >
> > > > The way I understand this code, you create a consumer every time you do
> > > > uprobe_register() and unregister makes it go away.
> > > >
> > > > Now, register one, then 4g-1 times register+unregister, then register
> > > > again.
> > > >
> > > > The above seems to then result in two consumers with the same
> > > > session_id, which leads to trouble.
> > > >
> > > > Hmm?
> > >
> > > ugh true.. will make it u64 :)
> > >
> > > I think we could store uprobe_consumer pointer+ref in session_consumer,
> > > and that would make the unregister path more interesting.. will check
> > 
> > More interesting how? It's actually a great idea, uprobe_consumer
> 
> nah, got confused ;-)

actually.. the idea was to store uprobe_consumer pointers in 'return_instance'
and iterate them on return probe (not uprobe->consumers).. but that would
require the unregister code to somehow remove them (replace with null)

but we wouldn't need to do the search for matching consumer with session_id

also it probably regress current code, because we would execute only uprobe
return consumers that were registered at the time the function entry was hit,
whereas in current code the return uprobe executes all registered return
consumers

jirka

> 
> > pointer itself is a unique ID and 64-bit. We can still use lowest bit
> > for RC (see my other reply).
> 
> I used pointers in the previous version, but then I thought what if the
> consumer gets free-ed and new one created (with same address.. maybe not
> likely but possible, right?) before the return probe is hit
> 
> jirka