bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] fully enable rpctrace to trace multitask programs.


From: Da Zheng
Subject: Re: [PATCH] fully enable rpctrace to trace multitask programs.
Date: Mon, 22 Jun 2009 17:22:48 +0800
User-agent: Thunderbird 2.0.0.21 (Macintosh/20090302)

address@hidden wrote:
I think it is still useful even when rpctrace is tracing a single task
program. rpctrace can know whether a send right is to a traced task or
not and  thus, don't need to treat mach_port_insert_right() specially
and can  handle the case below properly.

Well, I'm not sure what the difference to the original rpctrace is
exactly... However, if there is a change in tracing of
source/destination that is meaningful even without actually supporting
multiple tasks, I guess that's an obvious candidate for putting it in a
separate patch :-)
It's useful to trace the source and destination of RPCs when tracing a single task, but the help is very limited.
The original rpctrace can trace a single task very well in the normal case.
I bet that no programs get a send right like this:
mach_port_extract_right (mach_task_self (), receive_right, MACH_MSG_TYPE_MAKE_SEND, &receive_right, &poly);
I provided a patch before to fix the bug that a traced task hangs when
it gets signals. see https://savannah.gnu.org/patch/?6830

Ah, nice.

In that case, please keep this change in a seperate patch (as it
originally was), instead of lumping it in with the main multitask patch
:-)
OK.
Actually, with all the custom code you need for handling the array, it's
*more* complex than using a standard data structure like ihash!

Of course, the amount of code actually used with ihash would be larger
-- but this code is maintained elsewhere, so it doesn't really figure in
the complexity comparision. What matters is the amount of code you need
to maintain here, and I don't think the custom array handling lowers
that...

I'm not even convinced on the performance point. ihash requires the CPU
to execute more individual instructions of course; but on modern
processors, that's not the major performance factor in most applications
anyways. What usually matters more is cache usage -- and this is
probably better with ihash, as that code is used in other places as
well, and thus it doesn't actually occupy additional space in the caches
when you use it here as well...

Admittedly, the last part is guesswork. But even if it is somewhat less
efficient, I think that avoiding the custom handling is worth it.
Though Hurd provides ihash, we cannot use it as a dynamic array or
linked list. They are very common structure. We should have a general
implementation and include them in some library such as
libshouldbeinlibc so Hurd developers don't need to implement them
again  and again when they need them.

Well, single-linked lists are very simple once you understand the idea;
so adding standard functions (or macros) for handling them is probably
considered hardly worthwhile...

Also note that in many cases (including the one above), actually you
*can* use ihash instead of linked lists... While it is a more complex
data structure, I don't think that really matters most of the time.
I'm not convinced to use ihash everywhere by that ihash is a library in Hurd and its code is maintained. I still think we need to use the hash table where it is needed, use the linked list where it is needed, etc. I guess you don't like the dynamic array:-). I prefer using the dynamic array to the linked list because the array is relatively static. The array is changed only when a task is created or destroyed.
Well, I think that keeping the code maintainable in the long run is more
important than keeping modifications minimal... It is quite normal that
non-trivial changes require some refactoring.

Note that if the refactoring changes can be split out into separate
patches, this helps keeping the main patch with the actual functional
changes smaller and easier to understand...
Separating the structure is already done.
+  err = mach_port_request_notification (mach_task_self (), right,
+                                       MACH_NOTIFY_DEAD_NAME, 1,
+                                       notify_pi->port_right,
+                                       MACH_MSG_TYPE_MAKE_SEND_ONCE, &foo);
+  assert_perror (err);
Are you sure that mach_port_request_notification() will only fail when
the program messed up, and never in normal use?...
The program might not work properly as I explained above (see the comments on notify_pi), but it depends on the traced task.
So I want rpctrace to stop and everyone knows where the problem is.

Well, the question really is whether the function can *only* fail if
there is some bug in rpctrace itself -- in that case, assert() would be
the right thing. However, if it is possible that the call fails even if
rpctrace itself is absolutely correct, you should use a normal error()
rather than assert(), as explained above.

According to the manual, mach_port_request_notification() with
MACH_NOTIFY_DEAD_NAME can actually fail with KERN_RESOURC_SHORTAGE or
KERN_UREFS_OVERFLOW -- the former at least is something that can happen
in normal operation, even if rpctrace is bug-free. (The latter probably
as well, though I'm not entirely sure about that one...)
I didn't know assert_perror() can also be disabled by NDEBUG as assert().
In that case, I have to always use error() to check whether RPCs return successfully?
@@ -302,6 +500,7 @@ new_send_once_wrapper (mach_port_t right,
mach_port_t *wrapper_right)
Your mail client wrapped this line (and many more like this), breaking
the patch :-(
I knew thunderbird had some problems so I used gmail directly.
It seems gmail isn't suitable for sending patches, either:-(

You might consider using a proper mail client, like mutt... ;-)

IIRC kernel.org has some tips on sending patches with various mail
clients, including Thunderbird and the gmail webmail interface.
I still like to use software with GUI:)
Thank you for your suggestion. I hope I can find the tips.
+  if (info->receive_right)
+    {
+      /* Remove it from the send right list. */
+      prev = &info->receive_right->next;
+      if (*prev)
+       {
+         while (*prev != info && *prev)
+           prev = &((*prev)->next);
+         assert (*prev);
+         *prev = info->next;
+       }
Another case of unnecessary outer IF...

This code is actually incorrect I believe: surely the assert() should
also trigger when the list is empty?
The outer IF here is necessary because of the code "prev = &info->receive_right->next;".

Sorry, I guess my wording was confusing. I actually meant the "if
(*prev)" bit, which is unnecessary, just like in the other places I
pointed out...
It is done:)
+struct traced_info *
+discover_receive_right (mach_port_t send)
+{
I'm rather sceptical about this whole function. Is it really necessary
to do such a brute-force search for the receive right? Shouldn't we
actually always know about all receive rights in all traced tasks, as we
are tracing their task ports?
No, we cannot. A receive right is created by mach_port_allocate(), mach_port_allocate_name() or mach_reply_port(). The caller of mach_port_allocate() creates the port, but it dosn't get the receive right but the port name in the target task. The caller of mach_port_allocate_name() is the same except that the port name is decided by the caller.

mach_reply_port() cannot be traced by the rpctrace at all since it's a
system trap.

I see... So there is no way to divert it? That seems strange :-(

Anyways, while this means that we won't see a port created by
mach_reply_port() immediately, we still can start fully tracking it as
soon as it is used in any way, can't we?...
yes, by using discover_receive_right(). Otherwise, I don't know how.
If we don't treat the first two RPCs specially, rpctrace dosen't know
a  port has been created and the target task has the receive right. Of
course, we can do it (understand them and get the receive right) as I
did before.

That's the right approach IMHO.

But it becomes unnecessary after discover_receive_right is
implemented.  This is a more general and elegant (I think:-) way to
handle the receive  right.

I don't think this is elegant at all. It means that we keep track of
some information, while other is only retreived with a brute-force
search upon need -- this mixup is rather messy.

One way to make it more consistent would be never to try keeping track
of stuff; always fetching the information as needed instead. This would
be very inefficient, though.

The other option is trying to track everything, maintaining a consistent
"world view" at all times.
I think there might be some misunderstanding.
All receive rights are discovered by discover_receive_right. Whenever rpctrace discovers a receive right, it stores it in receiver_info. All discovered receive rights form a "world view" though the view might be out of date. Whenever rpctrace sees a send right it doesn't know, it calls discover_receive_right to find more receive rights to get the "world view" updated.
Do I get it right, that this function will create forward ports for
any receive rights it finds along the way, as a side effect?

Seems questionable to me. At least it should be documented.
Yes, you are right. The code above does have two functions. I will add
the comment.

Well, when I said "questionable", I actually meant that I see a problem
with this behaviour. Creating the forward port will create a reference
to a port that otherwise might not have any references at present. You
destroy the forward port when the last external reference is dropped,
but you create a forward port before there is any external reference?
That seems just wrong.
You're right. I need to fix it.
But as I said, I don't like this whole brute-force search anyways :-)
Do you have any suggestion other than the "brute-force" search?
          {
-           /* This is a receive right that we have been tracing sends to.  */
You dropped the old comment, but haven't added a new one for this code
block at all...
I didn't just drop one comment. I dropped the comment and the code below it.
The new code is explained by other comments.

My point is that the original comment for the whole code block was
useful, and you should add such a comment for your new code block as
well. It's harder to understand the meaning of a whole code block only
from comments of individual pieces.
Ah, I see.
-           name = info->name;
-           rr = ports_claim_right (info);
-           /* That released the refs on INFO, so it's been freed now.  */
+           struct traced_info *send_wrapper2;
+           char *name;
+           mach_port_t rr;
Why did you move the declarations of these two variables down here?
These variables are only used in this IF clause.

Yes, but that was also the case in the original code...
No. These two variables were defined in the switch-case clause in the original rpctrace. But if you insist on it, I will put those two variables outside the IF clause.
Looks like another change that could go in an extra cleanup patch,
but better should not go into a patch that does non-trivial
changes...
I don't understand what you mean here.

While the change might be useful by itself, it is not strictly required
for the actual functional changes you did. Having such non-functional
changes in the same patch makes the actual functional changes harder to
follow.

If you want to move the declarations, do it in an extra patch, that
doesn't change other stuff at the same time.

+           /* We have to deallocate the send right in
+            * receive_wrapper->forward before we import the port to
+            * port_info. So the reference count in the port info will be 1,
+            * if it doesn't have any other send rights. */
+           mach_port_deallocate (mach_task_self (), receive_wrapper->forward);
This comment is very hard to understand; I don't think it is helpful at
all :-(

I can only *guess*, from my understanding of the code, that what you are
trying to say is: the task just gave up the receive right, so we won't
be forwarding to it anymore, and thus need to drop the send right for
the forward port.
[...]
If 'forward' has the send right before ports_import_port() is called, the reference count of the new created port info is 2. If I don't call mach_port_deallocate before ports_import_port, the reference count is always 2 because 'forward' always hold the send right to the port, and ports_port_deref will not be able to destroy the port info if the source task doesn't have the send right to the port.

Well, the way you describe it doesn't really hit the point IMHO.

You make it sound like the reference count being increased is some kind
of side effect, and we need to "fix" that.

What really happens -- in my understanding -- is that we don't need the
*original* reference, i.e. our send right to the forward port anymore.
That we will get a new reference through the import, and that this
should be our only reference at this point, is really besides the point
I believe.
My whole point in the comment is trying to make it clear that mach_port_deallocate() must be called before ports_import_port(). Otherwise, the program cannot work correctly. I explained the reason in the comment. Do you think I should just say "mach_port_deallocate() must be called before ports_import_port()" in the comment?
To put it a different way, we should never need to care whether a
reference count is "1" or "2" or anything at some particular time. The
whole point of reference counting is that we do *not* need to think
about this. All we need to do is make sure that we actually adjust the
reference count when we give up a reference... That's what we do here,
and that is all the comment should say.
+           /* The port has been removed from the send wrapper,
+            * so we cannot put it in the free list. */
I don't understand this comment at all :-(
The original rpctrace recycles send wrappers that lose all send rights by increasing the weak reference count of port info and adding the send wrapper in the free list when the reference count becomes 0.

Actually, this seems questionable to me anyways... It increases the
complexity of the code, and I'm not sure this is justified here. Surely
the performance gain can't matter that much?...

Of course, that's not really related to your changes.
It does. It's not the problem of performance. It's a problem of correctness.
ports_claim_right() resets port_right of port_info and returns the original port right. Thus, the body of this send wrapper should be destroyed instead of being put in the free list of wrappers. Fortunately, all send wrappers aren't put in the free list any more. This comment isn't needed any more.
+  /* If it's the request of exec_startup_get_info,
+   * it means that the traced process starts to run */
What's the purpose of this? I thought all task/thread creation is
intercepted anyways?...
No. The first thread is created by exec server, which cannot be traced by rpctrace. So we have to find this first thread. The best way I can find is observing exec_startup_get_info, which is called when the process starts to run. Of course, it has drawback. If the traced program doesn't use glibc (which I believe is really rare), it cannot work.

Hm, is there no other way? Seems a bit fragile to me...

The task might already have gained some ports that we know nothing
about, before we started tracing...
After we set the kernel task port of the traced task, we are ready to trace the task. So we start tracing the task when the task is started, not when the traced task calls exec_startup_get_info().
+      munmap (threads, nthreads * sizeof (thread_t));
Using vm_deallocate() is clearer.
I see proc use munmap.

Well, I have seen vm_deallocate() used in the original rpctrace code, so
there isn't even a consistency argument here against vm_deallocate()...
No, I mean proc deallocates the thread port array with munmap. grep task_threads in hurd/proc:-)
@@ -744,6 +1301,7 @@ trace_and_forward (mach_msg_header_t *inp,
mach_msg_header_t *outp)
           we are consuming its `forward' right in the message we send.  */
        free (info->name);
        info->name = 0;
+       info->forward = 0;
Is this related to tracing multiple tasks?
It might be unnecessary. But I just wanted to reset it to avoid some possible error.
Yes, you can think it as cleanup or something like that.

So probably also a candidate for an extra patch?...
It's unnecessary any more after traced_info is separated.
-  ti = new_send_wrapper (traced_task, &task_wrapper);/* consumes ref */
-  asprintf (&ti->name, "task%d", (int) pid);
+  ti = new_send_wrapper (receive_ti, traced_task, &task_wrapper);
Why did you drop the original comment from this line?...
What comment is dropped?

/* consumes ref */
Do you know what the meaning of the comment is?
how does new_send_wrapper consume a reference?
BTW, have you checked that no changes are required to the
documentation?...

The document in where? The only document I can find is
http://www.gnu.org/software/hurd/hurd/debugging/rpctrace.html, which
actually isn't document at all.

Well, I assumed rpctrace might have some kind of documentation in the
Hurd manual or something...
It's not in Hurd  reference manual. Maybe I should write something:)

Well, with the rework of the info structures I recommended, you
naturally get a split into three phases at least: first a patch that
untangles some code paths, to make sure different kinds of info
structures never need to be passed to the same function. This would be a
preparation for the second patch, which actually splits the info
structures. This in turn would be a preparation for the main changes,
allowing tracing of multiple tasks.

At least the last phase can be further split down in this fashion
(perhaps the others too) -- in fact, I pointed out several things that
can be split out, in my remarks above.

When doing major functional changes, it's quite normal that you find
many other things that need to be changed along the way, to make the
main change possible. These other changes usually don't make much sense
by themselfs; but nevertheless, it's often possible to do them in
advance, without breaking the existing code. Each such preparation
change can be done in an extra patch, to be commited before doing the
main changes depending on it.

When doing refactoring in netrik, I sometimes did like five preparation
commits, before doing the actual change I intended to do. (Sometimes the
preparations even required other preparations first.) The ultimate patch
doing the main change often boils down almost to a one-liner this way...

I'm aware that working like that is a bit more work than just writing
one large patch. However, it makes the individual patches simple and
clean -- IHMO, this pays off in the end.

Or perhaps it's just me being pedantic ;-)

I think I have to separate the huge patch into smaller ones. Otherwise, no one will give it some review except you:)
But I have something to concern about.
First, how do I manage these patches? for example, if the previous patches need to be changed, how do I do the corresponding changes in the later patches (line numbers, etc)? Second, when I create the preparation patches as you suggest, I have to rewrite some code in the original rpctrace and it's very possible that these code will be removed. For example, if traced_info is separated, the code of rewrite_right() has to be adapted. However, rewrite_right is almost completely rewritten as you see. The adapted code in rewrite_right in the preparation patch is very likely to be removed. Do you think it is necessary to separate patches like this?

Zheng Da





reply via email to

[Prev in Thread] Current Thread [Next in Thread]