Re: updating COMPAT_LINUX for linux 2.6.x support (take 2)

[Chuck Silvers <chuq%chuq.com@localhost>]

On Sat, Jun 05, 2010 at 12:02:35AM +0100, Mindaugas Rasiukevicius wrote:
> Hello,
> 
> Chuck Silvers <chuq%chuq.com@localhost> wrote:
> > 
> > I rewrote my COMPAT_LINUX changes using the 1-proc-many-LWPs model.
> > the patch is at:
> > 
> > ftp://ftp.netbsd.org/pub/NetBSD/misc/chs/linux/diff.linux-nptl-take2.23
> > 
> 
> I like this patch a lot!  Especially good clean-ups, as a result.
> Few comments:
> 
> - Thanks for implementing ucas_*() for extra architectures.  This is
>   planned to be used for POSIX shared synchronisation primitives, thus
>   become a public API.  In the longer term, LINUX_UCAS_INT_UP should be
>   moved to generic code or, preferably, die.

sure, I was just being a bit lazy.


> - From proc_alloc():
> 
> -       /* allocate next free pid */
> +       p->p_pid = proc_alloc_pid(p);
> +       kdtrace_proc_ctor(NULL, p);
> +       mutex_exit(proc_lock);
> +       return p;
> 
> The kdtrace_proc_ctor() should not allocate memory with proc_lock held.
> Neither it needs to be called late - I fixed this on HEAD, so proc_lock
> can now be released by proc_alloc_pid().

ok, I like that better too.  I also changed proc_alloc_pid() to
set p->p_pid while proc_lock is still held (if it wasn't set already).


> - From linux_clone_nptl():
> 
> +       lwp_lock(l2);
> +       if ((l->l_flag & (LW_WREBOOT | LW_WSUSPEND | LW_WEXIT)) == 0) {
> +               if (p->p_stat == SSTOP || (p->p_sflag & PS_STOPPING) != 0)
> +                       l2->l_stat = LSSTOP;
> +               else {
> 
> This needs lwp_unlock_to(l2, spc->spc_lwplock), as LSSTOP state is protected
> by this lock.  This is also broken in sys__lwp_create() - will fix it shortly.

I'll take your word for it... I haven't learned all the scheduler goop,
I just copied it from sys__lwp_create() (as you noticed).
I've updated it with the new code.


> - From sys_obreak():
> 
> +       new = round_page((vaddr_t)SCARG(uap, nsize));
> +       if (new == 0) {
> +               return ENOMEM;
> +       }
> 
> IIRC, some architectures check for zero from assembly in libc stubs, before
> going to the syscall.

that check was there before, I just moved it earlier in the function.
it looks like all of the netbsd libc stubs check the value against
"end" or "_end" before calling the syscall, so we should be able to
remove it for native processes.  a bunch of other emulations use this
same function though, and I don't see a lot of benefit in trying to
make this more specific.

-Chuck