lkml.org 
[lkml]   [2018]   [Jul]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v5 06/10] Uprobes: Support SDT markers having reference count (semaphore)
    Date
    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

    \
     
     \ /
      Last update: 2018-07-03 08:30    [W:4.752 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site