[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] [FOR 4.2] tmem: add matching unlock for an about-to-be-destroyed object



> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Wednesday, September 05, 2012 7:25 AM
> To: Dan Magenheimer
> Cc: xen-devel@xxxxxxxxxxxxx; Konrad Wilk; Zhenzhong Duan; keir@xxxxxxx
> Subject: Re: [Xen-devel] [PATCH] [FOR 4.2] tmem: add matching unlock for an 
> about-to-be-destroyed
> object
> 
> >>> On 31.08.12 at 21:58, Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote:
> > A 4.2 changeset forces a preempt_disable/enable with
> > every lock/unlock.
> >
> > Tmem has dynamically allocated "objects" that contain a
> > lock.  The lock is held when the object is destroyed.
> > No reason to unlock something that's about to be destroyed!
> > But with the preempt_enable/disable in the generic locking code,
> > and the fact that do_softirq ASSERTs that preempt_count
> > must be zero, a crash occurs soon after any object is
> > destroyed.
> >
> > So force lock to be released before destroying objects.
> 
> We had noticed this oddity during XSA-15 processing too.
> What's the purpose of holding a lock on a to be destroyed
> object anyway? One would expect a lock of a containing
> entity to be held for that purpose (or really just while
> taking the object off whatever data structure it can be
> looked up through), which would eliminate the need for
> locking the object itself. That lock generally appears to be
> pool_rwlock, but in several places that one gets acquired
> _after_ checking pgp_count to be zero - how is it being
> guaranteed that this count doesn't increase between the
> check and the lock being acquired?

The flush_object code needs to obj_find the object before
it can destroy it and a successful obj_find takes the
lock.

Pushing the lock down to the object level was a misguided
attempt to maximize concurrency, especially for early
versions of persistent pools (frontswap) where the number of
objects was small (one per swap device).

I do agree that the tmem locking model deserves a complete rewrite.
The locking model of the similar code in upstream Linux
(which model was originally suggested by Jeremy Fitzhardinge)
is almost certainly superior and less buggy, though probably
slightly less concurrent.  The linux version probably should
be "back ported" into Xen now.

Dan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.