From owner-cvs-all@FreeBSD.ORG Mon Mar 12 16:36:22 2007 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4580D16A404 for ; Mon, 12 Mar 2007 16:36:22 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from wx-out-0506.google.com (wx-out-0506.google.com [66.249.82.225]) by mx1.freebsd.org (Postfix) with ESMTP id F090213C4C3 for ; Mon, 12 Mar 2007 16:36:21 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: by wx-out-0506.google.com with SMTP id s18so1631798wxc for ; Mon, 12 Mar 2007 09:36:21 -0700 (PDT) DKIM-Signature: a=rsa-sha1; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=boo8zDmokXJKq0IKraygjNMxNoH4ntI0oRbqKiPzGIL3Mjhsrgd1txtVQ4ruuFwdwsqyfkDJd1M92F8U192MokOpuAl1gkyMr+6J605z7vKlTYcciYDAJZz+wzHgaHcxDyf/Eiuu4pXuKDO22DcOjM0uwKGiqZiU+MWigsbVqhI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references:x-google-sender-auth; b=mYtlomJ65weskb65sG7IGKoaWIFT9RALhsLdsqHA9FqLLr/DOPPNHbqRGZ8VUxI3Y4crWUW61jjmzK7skl8p/8ZGoRWJNHfYl0b1LoPmzkVsqBXmiQJPk+/7oFqrO6jvoLcO7vM2eVtcxvpoFTS9EGHPQe4yCVceN5/K0W7z3PQ= Received: by 10.90.88.13 with SMTP id l13mr4745095agb.1173717381426; Mon, 12 Mar 2007 09:36:21 -0700 (PDT) Received: by 10.100.191.11 with HTTP; Mon, 12 Mar 2007 09:36:21 -0700 (PDT) Message-ID: <3bbf2fe10703120936w1909c744uc36ca1c030e76b6@mail.gmail.com> Date: Mon, 12 Mar 2007 17:36:21 +0100 From: "Attilio Rao" Sender: asmrookie@gmail.com To: "John Baldwin" In-Reply-To: <200703121114.59859.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200703092241.l29Mf2Ds062856@repoman.freebsd.org> <20070310205236.GA9185@garage.freebsd.pl> <3bbf2fe10703101611r60d6e1c5o8ef5fcf309fbf0e3@mail.gmail.com> <200703121114.59859.jhb@freebsd.org> X-Google-Sender-Auth: 9667930bde4e6e78 Cc: cvs-src@freebsd.org, src-committers@freebsd.org, Pawel Jakub Dawidek , cvs-all@freebsd.org Subject: Re: cvs commit: src/share/man/man9 Makefile condvar.9 lock.9 mi_switch.9 mtx_pool.9 mutex.9 rwlock.9 sleep.9 sleepqueue.9 sx.9 thread_exit.9 src/sys/kern kern_synch.c src/sys/sys mutex.h rwlock.h sleepqueue.h sx.h systm.h X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Mar 2007 16:36:22 -0000 2007/3/12, John Baldwin : > On Saturday 10 March 2007 19:11, Attilio Rao wrote: > > 2007/3/10, Pawel Jakub Dawidek : > > > On Sat, Mar 10, 2007 at 12:44:26PM +0100, Attilio Rao wrote: > > > > 2007/3/9, John Baldwin : > > > > >I don't have a date set for removing msleep(), esp. given it's wide > use. > > > > >I would like to remove it and all the spl*() functions in 8.0 if we can > > > > >swing it. > > > > > > > > > >I also have patches to let condition variables work with rwlocks and sx > > > > >locks, but the current implementation results in an API "explosion" > > > > >since each of the cv_*wait*() functions grows a cv_*wait*_rw() version > for > > > > >rwlocks and a cv_*waut*_sx() version for use with sx locks. One > possibility > > > > >would be to just cast the lock argument to (struct lock_object *) since > all > > > > >of our locks have a lock_object as the first member, but then you use > having > > > > >the compiler do type checking, and I'm really not willing to give up on > > > > >that. Too easy to have evil bugs that way. I suppose we could use > some > > > > >evil macro that used typeof() but that would be very gcc specific? > > > > > > > > > >I guess one other possibility is to standardize on the field name for > > > > >the lock_object, calling it lo_object instead of mtx_object, rw_object, > > > > >sx_object, etc. Anyone else have any ideas? > > > > > > > > What about adding a new function like: > > > > > > > > static __inline struct lock_object * > > > > mtx_export_lc(struct mtx *m) > > > > { > > > > > > > > return (&m->mtx_object); > > > > } > > > > > > > > to be per-interface (so having sx_export_lc() and rw_export_lc() too) > > > > and than using in this way: > > > > > > > > static struct mtx foo_lock; > > > > static struct cv foo_cv; > > > > ... > > > > > > > > mtx_lock(&foo_lock); > > > > ... > > > > cv_wait(&foo_cv, mtx_export_lc(&foo_lock)); > > > > > > > > (obviously using new struct lock_object methods you added for > locking/unlocking) > > > > > > > > It sounds reasonable to you? > > > > > > This is ugly. If we really need to provide information about which type > > > of lock we are using, I'd probably prefer cv_wait_(). > > > > > > What about something like this: > > > > > > #define cv_wait(cv, lock) do { > > > switch (LO_CLASSINDEX((struct lock_object *)(lock))) { > > > case 1: > > > cv_wait_mtx(cv, lock); > > > break; > > > case 2: > > > cv_wait_sx(cv, lock); > > > break; > > > case 3: > > > cv_wait_rw(cv, lock); > > > break; > > > default: > > > panic("Invalid lock."); > > > } > > > } while (0) > > > > This is exactly what John is trying to avoid. > > You have however to export cv_wait_*() & friends in the public > > namespace and at this point you don't need such wrapper. > > > > I know it is not so elegant, but the other solutions are uglier. > > Having a function returning the lock object per-primitive is the most > > suitable, IMHO. > > No, that's more typing than _rw and _sx. Here is what I want to happen if > possible: Ok, but it doesn't lead to the API explosion and doesn't have the two problems you mentioned before with struct lock_object: 1) having a "name costrained" member for the lock object 2) having the lock object as first member > cv_wait(cv, mtx); > > cv_wait(cv, rw); > > cv_wait(cv, sx); > > and have the the compiler figure it out. Basically, trying to shoehorn some > C++ into C since mtx, rw, and sx are sub-classes of 'lock_object'. :) That > is, I'd like it to do something like this: > > #define cv_wait(cv, lock) do { \ > if (typeof(lock) == (struct mtx *)) \ > _cv_wait(cv, &lock->mtx_object); \ > else if (typeof(lock) == (struct rwlock *)) \ > _cv_wait(cv, &lock->rw_object); \ > else if (typeof(lock) == (struct sx *)) \ > _cv_wait(cv, &lock->sx_object); \ > else \ > compile_error; \ > } while (0) > > So you still get type checking, etc. I'm thinking maybe the simplest thing to > do is to rename 'mtx_object', 'rw_object', and 'sx_object' fields to all > be 'lock_object' and then do this: Even if this idea is not so bad (beacause we primitives are in a small and controlled number of them) we have a lot of places to update for that. A macro could be a temporary help, btw. Attilio -- Peace can only be achieved by understanding - A. Einstein