From: "Przemysław Pawełczyk" <przemyslaw@pawelczyk.it>
To: Josh Stone <jistone@redhat.com>
Cc: systemtap@sourceware.org
Subject: Re: [PATCH] Fix target_set tapset.
Date: Wed, 17 Jun 2009 19:05:00 -0000 [thread overview]
Message-ID: <40e92d5b0906171205k7c427f9ame9c0bfb401cf8292@mail.gmail.com> (raw)
In-Reply-To: <4A38449E.8060209@redhat.com>
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
next prev parent reply other threads:[~2009-06-17 19:05 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-06-13 23:46 Przemyslaw Pawelczyk
2009-06-15 19:11 ` Josh Stone
2009-06-16 23:13 ` Przemysław Pawełczyk
2009-06-17 1:19 ` Josh Stone
2009-06-17 19:02 ` Frank Ch. Eigler
2009-06-17 19:05 ` Przemysław Pawełczyk [this message]
2009-06-17 21:47 ` Josh Stone
2009-06-18 22:58 ` [PATCH 1/2] " Przemyslaw Pawelczyk
2009-06-19 1:01 ` Josh Stone
[not found] ` <076001c9f07e$e4a73a40$adf5aec0$@ac.cn>
[not found] ` <4A3AF41B.7090804@redhat.com>
2009-07-09 1:04 ` how to get one process's resource usage by systemtap tgh
2009-06-18 22:58 ` [PATCH 2/2] Add test for target_set tapset Przemyslaw Pawelczyk
2009-06-19 1:56 ` Josh Stone
2009-06-19 21:26 ` Przemysław Pawełczyk
2009-06-19 21:27 ` [PATCH v2] " Przemyslaw Pawelczyk
2009-06-20 1:00 ` Przemysław Pawełczyk
2009-06-20 0:33 ` [PATCH v2.5][DRAFT] " Przemyslaw Pawelczyk
2009-06-20 13:43 ` [PATCH v3] " Przemyslaw Pawelczyk
2009-06-22 21:06 ` Josh Stone
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=40e92d5b0906171205k7c427f9ame9c0bfb401cf8292@mail.gmail.com \
--to=przemyslaw@pawelczyk.it \
--cc=jistone@redhat.com \
--cc=systemtap@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).