Messages in this thread | | | Subject | Re: [PATCH 0/7] Uprobes: Support SDT markers having reference count (semaphore) | From | Ravi Bangoria <> | Date | Fri, 8 Jun 2018 07:59:38 +0530 |
| |
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@linux.ibm.com> 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. 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?
[...]
>> >> - 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.
> (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.
Let me know your thoughts.
Thanks, Ravi
| |