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

[Masami Hiramatsu]

On Fri, 8 Jun 2018 12:04:25 +0530
Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx> wrote:

> Hi Masami,
> 
> >> 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.
> 
> Sure.
> 
> > 
> >> [...]
> >>
> >>>>
> >>>>  - 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.
> 
> Not ready as such.. it's making the code bit complicated. I'm working on it
> and will send the next series with those patches included.

OK, thanks!

> >>> (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?
> 
> Not really, it would be simpler if I can put it inside install_breakpoint().
> Consider an mmap() case. Probed instruction resides in the text section whereas
> reference counter resides in the data section. These sections gets mapped using
> separate mmap() calls. So, when process mmaps the text section we will change the
> instruction, but section holding the reference counter may not have been mapped
> yet in the virtual memory. If so, we will fail to update the reference counter.

Got it. 
In such case, maybe we can hook the target page mmapped and do install_breakpoint()
at that point. Since the instruction is protected by a refctr, unless mmap the
page on where the refctr is, the program doesn't reach the tracepoint. Is that right?

Thank you,

-- 
Masami Hiramatsu <mhiramat@xxxxxxxxxx>