From owner-freebsd-threads@FreeBSD.ORG Sun Aug 17 11:50:17 2003 Return-Path: Delivered-To: freebsd-threads@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 233C637B401 for ; Sun, 17 Aug 2003 11:50:17 -0700 (PDT) Received: from ns1.xcllnt.net (209-128-86-226.BAYAREA.NET [209.128.86.226]) by mx1.FreeBSD.org (Postfix) with ESMTP id 28D2B43F3F for ; Sun, 17 Aug 2003 11:50:16 -0700 (PDT) (envelope-from marcel@xcllnt.net) Received: from dhcp42.pn.xcllnt.net (dhcp42.pn.xcllnt.net [192.168.4.242]) by ns1.xcllnt.net (8.12.9/8.12.9) with ESMTP id h7HIoFwO097756 for ; Sun, 17 Aug 2003 11:50:15 -0700 (PDT) (envelope-from marcel@piii.pn.xcllnt.net) Received: from dhcp42.pn.xcllnt.net (localhost [127.0.0.1]) by dhcp42.pn.xcllnt.net (8.12.9/8.12.9) with ESMTP id h7HIoFR6001242 for ; Sun, 17 Aug 2003 11:50:15 -0700 (PDT) (envelope-from marcel@dhcp42.pn.xcllnt.net) Received: (from marcel@localhost) by dhcp42.pn.xcllnt.net (8.12.9/8.12.9/Submit) id h7HIoFRB001241 for threads@FreeBSD.org; Sun, 17 Aug 2003 11:50:15 -0700 (PDT) (envelope-from marcel) Date: Sun, 17 Aug 2003 11:50:15 -0700 From: Marcel Moolenaar To: threads@FreeBSD.org Message-ID: <20030817185015.GB928@dhcp42.pn.xcllnt.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="sm4nu43k4a2Rpi4c" Content-Disposition: inline User-Agent: Mutt/1.5.4i Subject: First draft: rewrite of {get|set|swap}context(3) X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 17 Aug 2003 18:50:17 -0000 --sm4nu43k4a2Rpi4c Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Gang, Please review the attached patch and let me know to what extend it introduces breakages. It is the result of "aggressive engineering" to provoke comments and further fuel discussion. The reason for this approach is that we currently do not initialize any fields other than the mcontext and sigmask and only partially copyin() and copyout() the contexts (typically only the mcontext and sigmask fields). This generally is very sensitive to breakages, especially when we intro- duce flags or need to use any of the spare fields. getcontext(3): the whole ucontext is zeroed before (partially) filled and we copyout() all of it. This means that uc_link can only be set/defined after calling setcontext(). setcontext(3): we copyin() the whole ucontext but partially restore its fields. The sigmask is restored conditionally based upon the UCF_SIGMASK flag. swapcontext(3): we preserve uc_link by doing a partial copyin(). The context is otherwise zeroed and partially filled. we copyout() the whole context. The patch has only been compile-tested. Shoot... -- Marcel Moolenaar USPA: A-39004 marcel@xcllnt.net --sm4nu43k4a2Rpi4c Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="context.diff" Index: kern/kern_context.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_context.c,v retrieving revision 1.6 diff -u -r1.6 kern_context.c --- kern/kern_context.c 11 Jun 2003 00:56:55 -0000 1.6 +++ kern/kern_context.c 17 Aug 2003 18:34:51 -0000 @@ -38,13 +38,6 @@ #include #include -/* - * The first two fields of a ucontext_t are the signal mask and - * the machine context. The next field is uc_link; we want to - * avoid destroying the link when copying out contexts. - */ -#define UC_COPY_SIZE offsetof(ucontext_t, uc_link) - #ifndef _SYS_SYSPROTO_H_ struct getcontext_args { struct __ucontext *ucp; @@ -65,18 +58,17 @@ getcontext(struct thread *td, struct getcontext_args *uap) { ucontext_t uc; - int ret; if (uap->ucp == NULL) - ret = EINVAL; - else { - get_mcontext(td, &uc.uc_mcontext, 1); - PROC_LOCK(td->td_proc); - uc.uc_sigmask = td->td_sigmask; - PROC_UNLOCK(td->td_proc); - ret = copyout(&uc, uap->ucp, UC_COPY_SIZE); - } - return (ret); + return (EINVAL); + + bzero(&uc, sizeof(uc)); + get_mcontext(td, &uc.uc_mcontext, 1); + PROC_LOCK(td->td_proc); + uc.uc_sigmask = td->td_sigmask; + PROC_UNLOCK(td->td_proc); + uc.uc_flags |= UCF_SIGMASK; + return (copyout(&uc, uap->ucp, sizeof(uc))); } /* @@ -86,51 +78,59 @@ setcontext(struct thread *td, struct setcontext_args *uap) { ucontext_t uc; - int ret; + int error; if (uap->ucp == NULL) - ret = EINVAL; - else { - ret = copyin(uap->ucp, &uc, UC_COPY_SIZE); - if (ret == 0) { - ret = set_mcontext(td, &uc.uc_mcontext); - if (ret == 0) { - SIG_CANTMASK(uc.uc_sigmask); - PROC_LOCK(td->td_proc); - td->td_sigmask = uc.uc_sigmask; - PROC_UNLOCK(td->td_proc); - } - } + return (EINVAL); + + error = copyin(uap->ucp, &uc, sizeof(uc)); + if (error) + return (error); + error = set_mcontext(td, &uc.uc_mcontext); + if (error) + return (error); + if (uc.uc_flags & UCF_SIGMASK) { + SIG_CANTMASK(uc.uc_sigmask); + PROC_LOCK(td->td_proc); + td->td_sigmask = uc.uc_sigmask; + PROC_UNLOCK(td->td_proc); } - return (ret == 0 ? EJUSTRETURN : ret); + return (EJUSTRETURN); } int swapcontext(struct thread *td, struct swapcontext_args *uap) { ucontext_t uc; - int ret; + int error; if (uap->oucp == NULL || uap->ucp == NULL) - ret = EINVAL; - else { - get_mcontext(td, &uc.uc_mcontext, 1); + return (EINVAL); + + bzero(&uc, sizeof(uc)); + error = copyin(uap->oucp + offsetof(ucontext_t, uc_link), &uc.uc_link, + sizeof(uc.uc_link)); + if (error) + return (error); + get_mcontext(td, &uc.uc_mcontext, 1); + PROC_LOCK(td->td_proc); + uc.uc_sigmask = td->td_sigmask; + PROC_UNLOCK(td->td_proc); + uc.uc_flags |= UCF_SIGMASK; + error = copyout(&uc, uap->oucp, sizeof(uc)); + if (error) + return (error); + error = copyin(uap->ucp, &uc, sizeof(uc)); + if (error) + return (error); + error = set_mcontext(td, &uc.uc_mcontext); + if (error) + return (error); + if (uc.uc_flags & UCF_SIGMASK) { + SIG_CANTMASK(uc.uc_sigmask); PROC_LOCK(td->td_proc); - uc.uc_sigmask = td->td_sigmask; + td->td_sigmask = uc.uc_sigmask; PROC_UNLOCK(td->td_proc); - ret = copyout(&uc, uap->oucp, UC_COPY_SIZE); - if (ret == 0) { - ret = copyin(uap->ucp, &uc, UC_COPY_SIZE); - if (ret == 0) { - ret = set_mcontext(td, &uc.uc_mcontext); - if (ret == 0) { - SIG_CANTMASK(uc.uc_sigmask); - PROC_LOCK(td->td_proc); - td->td_sigmask = uc.uc_sigmask; - PROC_UNLOCK(td->td_proc); - } - } - } } - return (ret == 0 ? EJUSTRETURN : ret); + return (EJUSTRETURN); } Index: sys/ucontext.h =================================================================== RCS file: /home/ncvs/src/sys/sys/ucontext.h,v retrieving revision 1.10 diff -u -r1.10 ucontext.h --- sys/ucontext.h 25 Apr 2003 01:50:30 -0000 1.10 +++ sys/ucontext.h 17 Aug 2003 18:13:49 -0000 @@ -34,6 +34,18 @@ #include #include +/* + * A note about UCF_SIGMASK: Contexts are defined in ucontext(3) to include + * the signal mask (ie blocked signals). By default getcontext(3) creates + * contexts according to that definition and setcontext(3) will restore it. + * Our 1:1 and M:N threading libraries use the ucontext structure without + * taking the signal mask into account. They either not restore it, not + * define it or both. Since contexts in the M:N library are shared between + * the kernel and the threaded application, there's a need to mark contexts + * that do have a valid signal mask so that setcontext(3) knows when to + * restore it. + */ + typedef struct __ucontext { /* * Keep the order of the first two fields. Also, @@ -49,7 +61,7 @@ struct __ucontext *uc_link; stack_t uc_stack; int uc_flags; -#define UCF_SWAPPED 0x00000001 /* Used by swapcontext(3). */ +#define UCF_SIGMASK 0x00000001 /* Context has valid signal mask. */ int __spare__[4]; } ucontext_t; --sm4nu43k4a2Rpi4c--