Re: [PATCH] Fix target_set tapset.

[Przemysław Pawełczyk <przemyslaw at pawelczyk dot it>]

2009/6/17 Josh Stone <jistone@redhat.com>:
> On 06/16/2009 04:13 PM, Przemysław Pawełczyk wrote:
>> On Mon, Jun 15, 2009 at 21:11, Josh Stone<jistone@redhat.com> wrote:
>>> On 06/13/2009 04:10 PM, Przemyslaw Pawelczyk wrote:
>>>> Add pid removal on exit syscall. Use dwarfless syscall probe aliases.
>>>> Correct formatting.
>>>> ---
>>> [...]
>>>> -probe syscall.fork.return
>>>> +probe nd_syscall.fork.return
>>>
>>> What do you think about preferring process.begin for utrace-enabled
>>> kernels? ?That should be lower overhead than a kprobe trap.
>>
>> This sounds good, however it leads to different path-execution on
>> various kernels and that is not good. IMHO better would be creating
>> another target_set-like tapset, but utrace-based only.
>
> Why is that not good? ?As long as the semantics are the same, it should
> be fine. ?What problems do you foresee with using different paths?

Probing in user-space is not the same as probing in kernel-space. The
only problem I foresee are different results from similar kernels
depending on having (or not) utrace-patch.

> I definitely don't want to split them, as that hurts portability and
> confuses users.

True point.

>>>> +probe nd_syscall.exit
>>>> +{
>>>> + ? ? delete _target_set[pid()]
>>>> ?}
>>>
>>> The problem is that this makes target_set_report() useless after the
>>> processes have exited (e.g. in an end probe). ?I think we should just
>>> trust that the target process won't beget more than MAXMAPENTRIES
>>> children. ?At the default 2048, that will probably be fine most of the time.
>>
>> It should be useless after the precesses have exited, because then
>> target_set contains only target() or is even empty (depends on what we
>> mean by the processes), right?
>> If it is really target_set, then as name suggests it should be valid
>> all the time. Pid collisions are rare, but not impossible.
>
> Pid collisions are a valid point. ?Remember too that we're storing the
> ppid() as the array value. ?If the parent dies before the child, and the
> ppid is reused, then you could have a confusing ancestry. ?There may
> even be loops.

You're right once again. Parent-child relation also should be fixed
during execution. You put me to shame, because I forgot about it...

> Anyway, my worry was that it may be seen as a regression from the old
> code. ?When I tested this patch, I used a script like:
>
> ?probe end { target_set_report() }
>
> With the old code, I saw a list of "x begat y". ?With your patch, I saw
> nothing -- because you deleted the pids when they exited. ?We can make
> arguments that this may be more correct, as long as we're ok with the
> changed semantics.

I see that I lost part of my previous mail (accidental delete?), where
I suggested introducing some global switch to define behavior -- old
(by default = 0) vs proper one (= 1). What you think about it?

>>> If you really want this, perhaps we could instead add the pid to a death
>>> array, and then have a function to clear those out. ?The clear could
>>> either be explicitly called, or perhaps it would be an implicit call at
>>> the end of target_set_report. ?Then the calling script can do periodic
>>> clear/reports if it knows there will be more than MAXMAPENTRIES children.
>>
>> Death pid array is other solution, but clearing routine definitely
>> shouldn't be located in target_set_report. Moreover target_set_report
>> shouldn't even print death pids, but new function could do this
>> (target_set_already_dead_report?). And if we're after this option,
>> then I can agree only if there is a separate clear function without
>> implicit calls in reporting functions with exception for functions
>> clearly pointing this out (target_set_already_dead_report_and_clear?).
>
> Now I think you're just messing with me, but ok, I see that death arrays
> are making this overly complex. ?We should just decide whether the
> records of dead pids should be kept around.

I didn't want to sound rough and really sorry if it is how you read
it. I always strongly oppose to hidden yet not obvious duties of
functions.

> Frank, you wrote this tapset -- any opinion?

Indeed. Frank, please write what you think.

> Josh

Regards.

-- 
Przemysław Pawełczyk