Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 26 Aug 2013 15:02:13 -0400
From:      John Baldwin <jhb@freebsd.org>
To:        Davide Italiano <davide@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r254703 - in head: share/man/man9 sys/sys
Message-ID:  <201308261502.13277.jhb@freebsd.org>
In-Reply-To: <CACYV=-ELQNPR3_Ok12J5UbUpROcr5oEVv6ndnnpnU781tyKgrw@mail.gmail.com>
References:  <201308231412.r7NECdG7081565@svn.freebsd.org> <201308231258.50969.jhb@freebsd.org> <CACYV=-ELQNPR3_Ok12J5UbUpROcr5oEVv6ndnnpnU781tyKgrw@mail.gmail.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On Monday, August 26, 2013 2:33:41 pm Davide Italiano wrote:
> On Fri, Aug 23, 2013 at 9:58 AM, John Baldwin <jhb@freebsd.org> wrote:
> > On Friday, August 23, 2013 11:29:45 am Davide Italiano wrote:
> >> On Fri, Aug 23, 2013 at 4:51 PM, John Baldwin <jhb@freebsd.org> wrote:
> >> > On Friday, August 23, 2013 10:12:39 am Davide Italiano wrote:
> >> >> Author: davide
> >> >> Date: Fri Aug 23 14:12:39 2013
> >> >> New Revision: 254703
> >> >> URL: http://svnweb.freebsd.org/changeset/base/254703
> >> >>
> >> >> Log:
> >> >>   Introduce callout_init_rm() so that callouts can be used in 
conjunction
> >> >>   with rmlocks. This works only with non-sleepable rm because handlers 
run
> >> >>   in SWI context. While here, document the new KPI in the timeout(9)
> >> >>   manpage.
> >> >
> >> > It also only works with exclusive locks.  (lc_unlock/lc_lock only 
handle
> >> > write locks for rmlocks).
> >> >
> >> > --
> >> > John Baldwin
> >>
> >> Thanks for pointing out this.
> >> I think it would be nice to have lc_lock/lc_unlock working both for
> >> shared and exclusive locks but I'm not 100% sure about all the
> >> implications/complications. From what I see for rwlocks asserting if a
> >> lock is held in read-mode is really cheap (check against a flag) while
> >> for rmlocks the assertion relies on traversing the tracker list for
> >> the rmlock so I'm worried this operation could be expensive. What's
> >> your opinion about?
> >
> > The much bigger problem is you need an rmtracker object to pass to the
> > lock/unlock routines.
> >
> > You could make this work hackishly in the callout case by special casing
> > rm locks that use read locking and using a tracker on softclock's stack,
> > but it is much harder to fix this for the rm_sleep() case where the
> > sequence is lc_unlock/lc_lock.
> >
> > --
> > John Baldwin
> 
> I see. I would really like to go for a clean solution if possible, and
> if the timeframe for 10 doesn't allow this just revert the commit
> until a better solution would be available. FWIW, I pondered a bit
> about this and the only way I was able to think is that of augmenting
> 'struct lock_object' with a 'void *arg' field  that in this case could
> be used to store a pointer to something, which in this case is a
> pointer to a rmtracker object, and this could allow easily to retrieve
> the needed information (as far as I see something similar is done to
> store WITNESS information). This, OTOH, could be overkill just to fix
> this case though.

Well, I've thought about changing lc_lock/unlock to return a uintptr_t or
void * instead of an int and then I could make rm_sleep work fine.  However, 
that still doesn't solve the callout case.  The callout case can't be fixed
easily without explicitly allocating storage in the softclock thread itself.

Also, I don't think you want a pointer in a lock_object.  Imagine if two 
threads both locked and then slept on the same rm lock in succession while
waiting for a wakeup.  You would have two trackers to keep track of, but only
one pointer in the lock_object.

I'm not sure you need to revert your commit.  It should pretty much panic
instantly if someone tries to use it with a read lock instead of a write
lock, even without INVARIANTS.

-- 
John Baldwin



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201308261502.13277.jhb>