This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: [PATCH] Fix target_set tapset.
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