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

[Da Zheng]

Hi,

olafBuddenhagen@gmx.net 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