Date:   Tue, 3 Jul 2018 11:59:26 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Cc:     oleg@...hat.com, rostedt@...dmis.org, mhiramat@...nel.org,
        peterz@...radead.org, mingo@...hat.com, acme@...nel.org,
        alexander.shishkin@...ux.intel.com, jolsa@...hat.com,
        namhyung@...nel.org, linux-kernel@...r.kernel.org, corbet@....net,
        linux-doc@...r.kernel.org, ananth@...ux.vnet.ibm.com,
        alexis.berlemont@...il.com, naveen.n.rao@...ux.vnet.ibm.com,
        linux-arm-kernel@...ts.infradead.org, linux-mips@...ux-mips.org,
        linux@...linux.org.uk, ralf@...ux-mips.org, paul.burton@...s.com,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH v5 06/10] Uprobes: Support SDT markers having reference
 count (semaphore)

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

Powered by blists - more mailing lists