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, 06 Jul 2009 14:26:10 +0800
User-agent: Thunderbird 2.0.0.22 (Macintosh/20090605)

Hi,

address@hidden wrote:
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, this is not a computer science class... It's not important to use
the ideal data structure for each individual case -- it's just important
to have something that works well enough, and is robust and simple. I
think ihash fits the bill in many cases.

I *assume* that this was the reasoning of the original Hurd developers,
and I think we should stick to that, if only for consistency's sake...
Since you think it's not proper to wrap the first thread at the place where trace exec_startup_get_info() is called, I now wrap the first thread of a task when the first RPC request comes. So now I have to use ihash to contain all task ports and the argument whether I should use ihash here is over:-)
In that case, I have to always use error() to check whether RPCs return successfully?

Well, strictly speaking you should.

However, as rpctrace already uses it wrongly all over the place, I'm not
sure what the best approach is. I tend to think it's still better to do
it right in the new code...
I think we can do it in this way. I can keep all assert_perror() for now and provide another patch to replace them with error().
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.

Well, AIUI, there are only two ways how a receive right could be used:
either mach_port_insert_right() (or mach_port_extract_right()) is
invoked, in which case we can look at the parameters, so we know exactly
where it is.

The other option is that it's sent to another task via an RPC. In this
case indeed we don't know what name it ends up as, so I guess we
actually have to search for it -- but at least we know in which task.

Unless I'm missing something essential, there is never any need to
search through all the tasks...
It should work.
+           /* 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.

No, not really. The actual *reason* is what I wrote above: keeping the
reference count consistent. What you desribe is just what happens if we
fail to do so... But I don't think this is useful. The important point
is that we need to keep the reference count consistent at all times, not
what specific problem we get if we fail to do so in this particular
instance.
OK.
Do you think I should just say "mach_port_deallocate() must be called before ports_import_port()" in the comment?

No, you should say that we need to drop a reference, as we won't be
using the forward port anymore. Because that's what really happens here
AIUI.

(Also, it's not actually necessary to do it before the
ports_import_port(), is it?... It seems to me that this whole code block
could be made clearer by reordering some lines -- though I'm not sure
exactly how...)

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.

Yeah, I do understand that the wrapper should not be reused here. My
point was that I find the whole idea of reusing wrappers questionable
anyways... But again, it is not really related to your changes.

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:-)

I know, I actually worked on proc code a couple of days ago, and used
munmap() there for consistency with the rest of the proc code... But
rpctrace uses vm_deallocate() already in other places, so it's more
consistent to stick to that.
OK

Zheng Da




reply via email to

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