Re: [PATCH] Fix target_set tapset.

[Josh Stone <jistone@redhat.com>]

On 06/17/2009 12:05 PM, PrzemysÂław PaweÂłczyk wrote:
> 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.

Well utrace is a kernel mechanism, and it shouldn't have too different
results, but that's ok.  We can use nd_syscall for now and perhaps
consider other enhancements later.

>> 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...

It's a tough call, because it's also not correct to say that the new
parent begat the child -- it's more like an adoption.

The more I look, the more I think the real value of this tapset is in
target_set_pid, and we shouldn't worry much about the intricacies of
target_set_report.

>> 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?

Since target_set_pid is more useful with what you call the proper mode,
I'm starting to think we should just go that way and forget the old mode.

>> 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.

I'm not hurt -- I meant that with a smile. :)

I'm going to rest my objection now and commit your patch.  We can create
Frank's suggested target_set_history_report() later if someone asks for it.

Josh