From owner-freebsd-threads@FreeBSD.ORG Wed Mar 29 08:33:42 2006 Return-Path: X-Original-To: threads@freebsd.org 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 35E1916A41F; Wed, 29 Mar 2006 08:33:42 +0000 (UTC) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id E0B1943D48; Wed, 29 Mar 2006 08:33:41 +0000 (GMT) (envelope-from davidxu@freebsd.org) Received: from [127.0.0.1] (root@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id k2T8XcRK064708; Wed, 29 Mar 2006 08:33:39 GMT (envelope-from davidxu@freebsd.org) Message-ID: <442A466A.9040506@freebsd.org> Date: Wed, 29 Mar 2006 16:33:46 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.12) Gecko/20060302 X-Accept-Language: en-us, en MIME-Version: 1.0 To: =?ISO-8859-1?Q?Dag-Erling_Sm=F8rgrav?= References: <86slp1u4qb.fsf@xps.des.no> In-Reply-To: <86slp1u4qb.fsf@xps.des.no> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Cc: threads@freebsd.org, jeff@freebsd.org Subject: Re: libthr cleanup X-BeenThere: freebsd-threads@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Threading on FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Mar 2006 08:33:42 -0000 Dag-Erling Smørgrav wrote: > The attached patch brings libthr up to WARNS level 2. There is also a > small amount of style and whitespace changes mixed in, mostly because > I'm so conditioned to style(9) that my fingers sometimes do these > things automatically. > I have bunch of patches in hand which compiles at level 3, if you commit it, there will lots of conflict to my working copy. > Parts of the patch go a little further than level 2, but we're nowhere > near level 3 (or antything higher). The major obstacle is the umtx > interface, which in my eyes is fundamentally broken. > The _umtx_op() syscall is intended to replace _umtx_lock() and > _umtx_unlock(), and support other operations like sleep, wakeup etc. > > This is the wrong way to go. If we applied this kind of thinking > universally, we'd just define all our system calls as macro wrappers > for __syscall(). > > Beyond these purely philosophical aspects, _umtx_op() seems designed > to encourage poor coding practices. It's impossible to use in a type- > safe manner: its first argument is supposed to be a struct umtx *, but > I don't think there's a single instance in libthr where it is called > with an actual struct umtx *. Instead, libthr uses umtx_t, which is > defined as long. Sometimes, an umtx_t is passed as the third argument > to _umtx_op() as well. Various fields in struct pthread, struct > pthread_barrier and struct pthread_cond are declared as umtx_t. Some > are used purely as cookies for passing to _umtx_op(); some are used as > counters or state variables. > > It's a miracle libthr works at all. The kernel expects a pointer to a > struct umtx; instead, it gets (mostly) a pointer to long. Luckily, > struct umtx (which contains a single pointer) is the same size as long > on all our platforms. > > _umtx_op() needs to be split into four system calls: > > int _umtx_lock_timeout(struct umtx *mtx, > const struct timespec * __restrict timeout); > int _umtx_unlock(struct umtx *mtx); > int _umtx_wait_timeout(const void *cookie, struct umtx *mtx, > const struct timespec * __restrict timeout); > int _umtx_wake(const void *cookie); > > _umtx_unlock() already exists with the correct semantics; the rest are > new. Note that we can't just add a struct timespec to _umtx_lock(), > as that would break the upgrade path from RELENG_5; hence the _timeout > suffix. > When libthr was redesigned, things were not clear, I have figured out all semantics needed to implement libthr, the above functions are the interfaces current internally implemented by libthr, and functions in umtx.h is not used because it can generate bloat code than the versions in libthr, current libthr shared library only has 64K size. I really dont need struct umtx at all, an integer is enough, basic idea is using atomic operations to test in userland and wait in kernel. the above functions should be changed to: int _umtx_lock_timeout(umtx_t *, const struct timespec * __restrict timeout); int _umtx_unlock(umtx_t *mtx); int _umtx_wait_timeout(umtx_t *, umtx_t expect, const struct timespec * __restrict timeout); int _umtx_wake(umtx_t *i, int number); the umtx_t could be an integer type, but to maintain binary compatibility, it has to be a long integer type. > I'm not sure the current implementation of UMTX_OP_WAIT / UMTX_OP_WAKE > in the kernel is correct. Normally, a wait primitive (like msleep(), > pthread_cond_wait() etc.) takes a cookie and a mutex, and unlocks the > mutex while it's sleeping on the cookie. It looks like the umtx code > originally worked like this, but I'm not sure it does anymore; it's > hard to unravel, partly because I haven't quite figured out the queues > yet and partly because I haven't had breakfast. > > DES > Well, I am not sure you know the history of umtx, the orignal work only did a lock and unlock semantic, there is no general sleep and wait semantic, orignal umtx code has obscure race, I think in early days, valgrind suffered from this bug. I am not sure you understand how a userland synchronization works, these two operations are used to implement compare and wait for a integer to be changed, you should check source code before talking a lot, saying that you doubt the UMTX_OP_WAIT and UMTX_OP_WAKE's correctness is not professional, there are users using libthr in daily work and stress testing had been done. I have put lots of work and time on libthr, I know the problems, though the _umtx interface is a bit ulgly because it was unclear when it was being designed, but I don't think it really hurt you or other people, it can be fixed in few days, I just was hesitating to add the new interfaces. David Xu