This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix target_set tapset.


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?

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

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

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.

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

Frank, you wrote this tapset -- any opinion?

Josh


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]