Re: [PATCH 0/7] Uprobes: Support SDT markers having reference count (semaphore)

[Masami Hiramatsu]

On Fri, 8 Jun 2018 07:59:38 +0530
Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx> wrote:

> Hi Masami,
> 
> On 06/08/2018 06:40 AM, Masami Hiramatsu wrote:
> > On Wed,  6 Jun 2018 14:03:37 +0530
> > Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx> wrote:
> > 
> >> Why RFC again:
> >>
> >> This series is different from earlier versions[1]. Earlier series
> >> implemented this feature in trace_uprobe while this has implemented
> >> the logic in core uprobe. Few reasons for this:
> >>  1. One of the major reason was the deadlock between uprobe_lock and
> >>  mm->mmap inside trace_uprobe_mmap(). That deadlock was not easy to fix
> >>  because mm->mmap is not in control of trace_uprobe_mmap() and it has
> >>  to take uprobe_lock to loop over trace_uprobe list. More details can
> >>  be found at[2]. With this new approach, there are no deadlocks found
> >>  so far.
> >>  2. Many of the core uprobe function and data-structures needs to be
> >>  exported to make earlier implementation simple. With this new approach,
> >>  reference counter logic is been implemented in core uprobe and thus
> >>  no need to export anything.
> > 
> > I agree with you. Moreover, since uprobe_register/unregister() are
> > exported to modules, this enablement would better be implemented
> > inside uprobe so that all uprobe users benefit from this.
> 
> 
> Sorry, I think you got me wrong. I meant, I don't need to expose all core
> uprobe _static_ functions to tarce_uprobe.
> 
> Now, about kernel modules, basically uprobe_register() takes three parameters:
>     inode, offset and consumer.
> There is no scope for the reference counter there. So I've created one more
> function: uprobe_register_refctr(). But this function is not exported as ABI
> to kernel module. i.e. kernel modules still does not have a way to create
> uprobe with reference counter.

OK, I got it from your patches. :)

> So for kernel modules,
> 
> is it fine to change current ABI from
>     uprobe_register(inode, offset, consumer)
> to
>     uprobe_register(inode, offset, ref_ctr_offset, consumer)
> 
> Or I should introduce new function for this:
>     uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer)
> and export it to kernel module?
> 
> What's your suggestion?

Latter is fine to me. Since the refctr is introduced totally in userspace
(for SDT) and free-address userspace probing doesn't need refctr, maybe
we should keep those separated.

> [...]
> 
> >> 
> >>  - This patches still has one issue. If there are multiple instances of
> >>    same application running and user wants to trace any particular
> >>    instance, trace_uprobe is updating reference counter in all instances.
> >>    This is not a problem on user side because instruction is not replaced
> >>    with trap/int3 and thus user will only see samples from his interested
> >>    process. But still this is more of a correctness issue. I'm working on
> >>    a fix for this.
> > 
> > Hmm, it sounds like not a correctness issue, but there maybe a performace
> > tradeoff. Tracing one particulear instance, other instances also will get
> > a performance loss
> 
> 
> Right, but it's temporary. I mean, putting everything in to this series was making
> it complex. So this is the initial one and I'll send followup patches which will
> optimize the reference counter update.

Ah, OK. If you have prepared the followup patches, could you also send it
with this series? Perhups it will help us to understand the issue clearer.

> 
> > (Only if the parameter preparation block is heavy,
> > because the heaviest part of probing - trap/int3 and recording data - isn't
> > executed.)
> >> BTW, why this happens? I thought the refcounter part is just a data which
> > is not shared among processes...
> > 
> 
> This happens because we are not calling consumer_filter function. consumer_filter
> is the one who decides whether to change the instruction to trap or not in a given
> mm. We also need to call it before updating reference counter.

Hmm, it sounds simple... maybe we can increment refctr in install_breakpoint/
remove_breakpoint?

Thank you,

> 
> Let me know your thoughts.
> 
> Thanks,
> Ravi
> 


-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>