Hi Srikar, On 07/02/2018 09:31 PM, Srikar Dronamraju wrote: >> Implement the reference counter logic in core uprobe. User will be >> able to use it from trace_uprobe as well as from kernel module. New >> trace_uprobe definition with reference counter will now be: >> >> <path>:<offset>[(ref_ctr_offset)] >> >> where ref_ctr_offset is an optional field. For kernel module, new >> variant of uprobe_register() has been introduced: >> >> uprobe_register_refctr(inode, offset, ref_ctr_offset, consumer) >> > > Sorry for bringing this again, but I would actually think the ref_ctr is > a consumer property. i.e the ref_ctr_offset should be part of > uprobe_consumer. I agree that reference counter is a consumer property and that was the main reason my initial draft was to change trace_uprobe. But there were couple of issues with that approach too. I've already mentioned few of them here: https://lkml.org/lkml/2018/6/6/129. Apart from these, if I do it inside trace_uprobe, kernel module won't have a way to use reference counter. Now about adding ref_ctr_offset into uprobe_consumer. Actually, I didn't want to change the uprobe_consumer definition because it's already exported and tools like systemtap are using it. And thus, I haven't explored how difficult or easy it will be to implement it that way. > > The advantages of doing that would be > 1. Dont need to expose uprobe structure and just update our > uprobe_consumer which is already an exported structure. > - Exporting uprobe structure would expose some of our internal > implementation details, basically reduce the freedom of changing stuff > internally. I agree. We will loose the freedom to change stuff by exporting uprobe. > - we came up with uprobe_arch for the parts that we wanted to expose > to archs. exposing uprobe and uprobe_arch looks weird. Hmm, how about this ... set_swbp(arch_uprobe, ...) { uprobe_write_opcode(arch_uprobe, ...) { uprobe = container_of(arch_uprobe); ... } } Let me think on this. If this works, I won't need to export struct uprobe outside. > > 2. ref_ctr_offset is necessarily a consumer property, its not a uprobe > property at all. I agree. > > 3. We dont need to change/add new uprobe_register functions. Quite possible. I need to explore on that. > > The way I look at it is. > > Based on the ref_ctr_offset field in consumer, we update_ref_ctr() > around install_breakpoint/remove_breakpoint. > >> +static int delayed_uprobe_add(struct uprobe *uprobe, struct mm_struct *mm) >> +{ >> + struct delayed_uprobe *du; >> + >> + if (delayed_uprobe_check(uprobe, mm)) >> + return 0; >> + >> + du = kzalloc(sizeof(*du), GFP_KERNEL); >> + if (!du) >> + return -ENOMEM; >> + >> + du->uprobe = uprobe; >> + du->mm = mm; >> + list_add(&du->list, &delayed_uprobe_list); >> + return 0; >> +} >> + > > If I understood the delayed_uprobe stuff, its when we could insert a > breakpoint but the vma that has the ref_ctr_offset is not loaded. Is > that correct? That's correct. Thanks, Ravi
