Re: [PATCH 6/8] trace_uprobe/sdt: Fix multiple update of same reference counter

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

Hi Oleg,

On 03/16/2018 11:20 PM, Oleg Nesterov wrote:
> On 03/16, Ravi Bangoria wrote:
>> On 03/15/2018 08:19 PM, Oleg Nesterov wrote:
>>> On 03/13, Ravi Bangoria wrote:
>>>> For tiny binaries/libraries, different mmap regions points to the
>>>> same file portion. In such cases, we may increment reference counter
>>>> multiple times.
>>> Yes,
>>>
>>>> But while de-registration, reference counter will get
>>>> decremented only by once
>>> could you explain why this happens? sdt_increment_ref_ctr() and
>>> sdt_decrement_ref_ctr() look symmetrical, _decrement_ should see
>>> the same mappings?
> ...
>
>> A A A  # strace -o out python
>> A A  A A  mmap(NULL, 2738968, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7fff92460000
>> A A A A A  mmap(0x7fff926a0000, 327680, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x230000) = 0x7fff926a0000
>> A A A A A  mprotect(0x7fff926a0000, 65536, PROT_READ) = 0
> Ah, in this case everything is clear, thanks.
>
> I was confused by the changelog, I misinterpreted it as if inc/dec are not
> balanced in case of multiple mappings even if the application doesn't play
> with mmap/mprotect/etc.
>
> And it seems that you are trying to confuse yourself, not only me ;) Just
> suppose that an application does mmap+munmap in a loop and the mapped region
> contains uprobe but not the counter.

this is fine because ...

>
> And this all makes me think that we should do something else. Ideally,
> install_breakpoint() and remove_breakpoint() should inc/dec the counter
> if they do not fail...

The whole point of adding this logic in trace_uprobe is we wanted to
decouple the counter inc/dec logic from uprobe patching. If user is just
doing mmap+munmap region in a loop which contains uprobe, the
instruction will be patched by the core uprobe infrastructure. Whenever
application mmap the region that holds to counter, it will be incremented.

Our initial design was to increment counter in install_breakpoint() but
uprobed instruction gets patched in a very early stage of binary loading
and vma that holds the counter may not be mapped yet.

>
> Btw, why do we need a counter, not a boolean? Who else can modify it?
> Or different uprobes can share the same counter?

Yes, multiple SDT markers can share the counter. Ex, there can be multiple
implementation of same function and thus each individual implementation
may contain marker which share the same counter. From mysql,

A  # readelf -n /usr/lib64/mysql/libmysqlclient.so.18.0.0 | grep -A2 Provider
A A A  Provider: mysql
A A A  Name: net__write__start
A A A  Location: 0x000000000003caa0, ..., Semaphore: 0x0000000000333532
A  --
A A A  Provider: mysql
A A A  Name: net__write__start
A A A  Location: 0x000000000003cd5c, ..., Semaphore: 0x0000000000333532

Here, both the markers has same name, but different location. Also they
share the counter (semaphore).

Apart from that, counter allows multiple tracers to trace on a single marker,
which is difficult with boolean flag.

Thanks,
Ravi