On Fri, 8 Jun 2018 12:04:25 +0530 Ravi Bangoria <ravi.bango...@linux.ibm.com> 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 <mhira...@kernel.org>
