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

[olafBuddenhagen]

Hi,

On Mon, Jun 22, 2009 at 05:22:48PM +0800, Da Zheng wrote:
> olafBuddenhagen@gmx.net wrote:

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

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

The man page clearly states this. It actually has an explicit warning
about using it to check return codes...

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

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

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

You are right: in the original code they were obviously also used
outside the IF. Sorry, my bad.

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

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

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

Not really. I didn't realize that the situation is totally different
anyways, as new_send_wrapper() now does something completely different
despite the same name... I hope the refactoring will improve this :-)

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

That would be great :-)

-antrik-