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

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

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

> Josh

Regards.

-- 
Przemysław Pawełczyk


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