I've split the patch into two. Cheers Flavio
On Thu, 27 Aug 2015 at 11:53 Justus Winter <
4win...@informatik.uni-hamburg.de> wrote:
> Hi,
>
> Quoting Flávio Cruz (2015-08-27 00:26:13)
> > Em qua, 26 de ago de 2015 às 12:18, Justus Winter <
> > 4win...@informatik.uni-hamburg.de> escreveu:
> >
> >
> > > After looking around and using KDB, I've figured out that the
> following
> > loop in
> > > kern/exceptions.c/exception_raise_continue_slow was the culprit:
> >
> > Could you elaborate a little on your method?
> >
> >
> > I changed the code to chain all the allocated ports together in a linked
> list.
> > Then, I added new fields to ipc_port to track send-only rights and to
> > understand where those rights were being created and where the port was
> being
> > manipulated. Finally, I added a new KDB command to show all the inactive
> ports.
> > It was then easy to track exactly how the port was handled inside
> > exception_raise_continue_slow.
>
> Neat. Thanks for sharing. Fwiw, I've used Coccinelle for code
> instrumentations. Also for fixing up code in a mechanical way. It is
> an awesome tool :)
>
> > > while (mr == MACH_RCV_INTERRUPTED) {
> > > /*
> > > * Somebody is trying to force this thread
> > > * to a clean point. We must cooperate
> > > * and then resume the receive.
> > > */
> > >
> > > while (thread_should_halt(self)) {
> > > /* don't terminate while holding a reference */
> > > if (self->ast & AST_TERMINATE)
> > > ipc_port_release(reply_port);
> > > thread_halt_self();
> > > }
> > >
> > > ip_lock(reply_port);
> > > ....
> > > }
> > >
> > > The reply port of the target thread (from CLISP?) is not being
> released
> > since
> > > it enters the while(thread_should_halt(self)) loop and the thread
> > terminates
> > > inside thread_halt_self but the previous condition does not hold,
> which
> > fails
> > > to release the port. I also think that once the code enters
> > thread_halt_self,
> > > it never comes back since the stack is discarded (for both if cases
> > inside the
> > > function).
> >
> > While I agree that the function thread_halt_self does not return, it
> > only terminates the thread if the AST_TERMINATE flag is set.
> > Otherwise, it returns to userspace.
> >
> >
> > Yes, it returns to userspace using thread_exception_return, but notice
> that a
> > reference must be released anyway (check first statement after the while
> loop).
>
> Right. We have two references, one for the send-once right, and one
> for the port being saved in ith_port (see exception.c:386).
>
> > For the else case in thread_halt_self, the thread may throw away the
> current
> > stack and run thread_exception_return in thread_block, which is also
> executed
> > in the case of success in exception_raise_continue_slow.
>
> And indeed exception_raise_continue_fast releases two references too.
>
> > I find the use of `while' dodgy in the first place b/c if
> > thread_halt_self doesn't return, an if would do the trick as well and
> > don't imply the chance of that code looping...
> >
> > I think there might be some situations where it will loop around
> depending on
> > the scheduling order (see line 798 on sched_prim.c).
>
> Could be. I keep messing up the semantics of thread_invoke in my head >,<
>
> > I've added a continuation argument to thread_halt_self so that the port
> is
> > released if the thread resumes using the continuation (followed by
> > thread_exception_return). Otherwise, it may loop and it will eventually
> also
> > release the reference and call thread_exception_return.
>
> Yes. This looks like the clean solution.
>
> > I've also made some cosmetic changes to the code.
>
> Would be best to split the change up nevertheless.
>
> Thanks for looking into this :)
> Justus
>
port-leak.patch
Description: Binary data
no-continuation.patch
Description: Binary data
