kern_threads.c.. upcall question..

[David Xu]

----- Original Message ----- 
From: "Julian Elischer" <julian at elischer.org>
To: "David Xu" <davidxu at freebsd.org>
Cc: <threads at freebsd.org>; "Daniel Eischen" <eischen at pcnet1.pcnet.com>
Sent: Tuesday, May 06, 2003 3:30 PM
Subject: Re: kern_threads.c.. upcall question..


> 
> 
> On Tue, 6 May 2003, David Xu wrote:
> 
> > 
> > ----- Original Message ----- 
> > From: "Julian Elischer" <julian at elischer.org>
> > To: "David Xu" <davidxu at freebsd.org>
> > Cc: "Daniel Eischen" <eischen at pcnet1.pcnet.com>; <threads at freebsd.org>
> > Sent: Tuesday, May 06, 2003 3:10 PM
> > Subject: Re: kern_threads.c.. upcall question..
> > 
> > 
> > > 
> > > 
> > > On Tue, 6 May 2003, David Xu wrote:
> > > 
> > > > I think the following patch is enough:
> > > 
> > > I agree that this seems enough from what I was reading.
> > > (I like patches that have most lines starting with '-' :-)
> > > 
> > > 
> > > > 
> > > > Index: kern_thread.c
> > > > ===================================================================
> > > > RCS file: /home/ncvs/src/sys/kern/kern_thread.c,v
> > > > retrieving revision 1.129
> > > > diff -u -r1.129 kern_thread.c
> > > > --- kern_thread.c 1 May 2003 12:16:06 -0000 1.129
> > > > +++ kern_thread.c 6 May 2003 06:32:10 -0000
> > > > @@ -721,41 +721,6 @@
> > > [...]
> > > >  void
> > > > @@ -962,27 +927,25 @@
> > > >   uintptr_t mbx;
> > > >   void *addr;
> > > >   int error,temp;
> > > > - ucontext_t uc;
> > > > + mcontext_t mc;
> > > >  
> > > >   p = td->td_proc;
> > > >   kg = td->td_ksegrp;
> > > >  
> > > >   /* Export the user/machine context. */
> > > > - addr = (void *)(&td->td_mailbox->tm_context);
> > > > - error = copyin(addr, &uc, sizeof(ucontext_t));
> > > > - if (error) 
> > > > -  goto bad;
> > > > -
> > > > - thread_getcontext(td, &uc);
> > > > - error = copyout(&uc, addr, sizeof(ucontext_t));
> > > > - if (error) 
> > > > + get_mcontext(td, &mc, 0);
> > > 
> > > I think this could be optimised even more.
> > > (why copy the FP regs if they are not valid) (etc).
> > > but it is an improvement..
> > > 
> > 
> > Why need we an intermediate mcontext_t, why not
> > direct copy the context in trap frame to userland space?
> > This should be fastest. :-)
> 
> this is what I was thinking..
> get_mcontext_user(td, addr)
> [...]
> 
> 
> > 
> > 
> > > > + addr = (void *)(&td->td_mailbox->tm_context.uc_mcontext);
> > > > + error = copyout(&mc, addr, sizeof(mcontext_t));
> > > > + if (error)
> > > >    goto bad;
> > > >  
> > > >   /* Exports clock ticks in kernel mode */
> > > >   addr = (caddr_t)(&td->td_mailbox->tm_sticks);
> > > >   temp = fuword(addr) + td->td_usticks;
> > > > - if (suword(addr, temp))
> > > > + if (suword(addr, temp)) {
> > > > +  error = EFAULT;
> > > >    goto bad;
> > > > + }
> > > >  
> > > >   /* Get address in latest mbox of list pointer */
> > > >   addr = (void *)(&td->td_mailbox->tm_next);
> > > > 
> > > > 
> > > > And should we disable single threading testing or do
> > > > double checking in thread_user_enter()? I think per-syscall
> > > > PROC_LOCK is too expensive for us.
> > > 
> > > I am not sure which one you refer too.. Which single_threading
> > > test?
> > > 
> > Single threading testing in thread_user_enter(), someone put 
> > a PROC_LOCK, quoted here:
> >         /*              
> >          * First check that we shouldn't just abort.
> >          * But check if we are the single thread first!
> >          */
> >         PROC_LOCK(p);
> >         if ((p->p_flag & P_SINGLE_EXIT) && (p->p_singlethread != td)) {
> >                 mtx_lock_spin(&sched_lock);
> >                 thread_stopped(p);
> >                 thread_exit();
> >                 /* NOTREACHED */
> >         }
> >         PROC_UNLOCK(p);
> > 
> > > BTW, I am a little unsure about the calling of thread_user_enter()
> > > from thread_userret().
> > > 
> > 
> > This is quantum preemptive for userland, it is used when statclock
> > interrupt hit in userland, if thread quantum is exhausted,
> > it must unbind upcall from current thread, and schedule an upcall,
> > you can think it is an implicit syscall triggered by CPU automatically
> > which just means to swap out current thread.
> 
> 
> ah..
> I was thinking that we could delay most of thread_user_enter()
> until we need it..
> i.e why read the mailbox until we need it..
> 

I think fuword is very fast, at least in i386, there is
few instructions in fuword, please read its code in
/sys/i386/i386/support.s, optimizing it may be not worth to do.
Beside this, you can not fetch mailbox pointer on damand, 
thread_schedule_upcall is protected under sched lock, it 
can not handle page fault at that time, and kse_thr_interrupt
need a mailbox pointer to indentify thread in kernel, delaying
it will cause problem.