Re: [Xen-devel] [PATCH v3 05/13] x86/altp2m: basic data structures and support routines.

[Tim Deegan <tim@xxxxxxx>]

At 16:04 +0100 on 07 Jul (1436285059), George Dunlap wrote:
> On 07/01/2015 07:09 PM, Ed White wrote:
> > diff --git a/xen/arch/x86/mm/mm-locks.h b/xen/arch/x86/mm/mm-locks.h
> > index b4f035e..301ca59 100644
> > --- a/xen/arch/x86/mm/mm-locks.h
> > +++ b/xen/arch/x86/mm/mm-locks.h
> > @@ -217,7 +217,7 @@ declare_mm_lock(nestedp2m)
> >  #define nestedp2m_lock(d)   mm_lock(nestedp2m, &(d)->arch.nested_p2m_lock)
> >  #define nestedp2m_unlock(d) mm_unlock(&(d)->arch.nested_p2m_lock)
> >  
> > -/* P2M lock (per-p2m-table)
> > +/* P2M lock (per-non-alt-p2m-table)
> >   *
> >   * This protects all queries and updates to the p2m table.
> >   * Queries may be made under the read lock but all modifications
> > @@ -225,10 +225,44 @@ declare_mm_lock(nestedp2m)
> >   *
> >   * The write lock is recursive as it is common for a code path to look
> >   * up a gfn and later mutate it.
> > + *
> > + * Note that this lock shares its implementation with the altp2m
> > + * lock (not the altp2m list lock), so the implementation
> > + * is found there.
> >   */
> >  
> >  declare_mm_rwlock(p2m);
> > -#define p2m_lock(p)           mm_write_lock(p2m, &(p)->lock);
> > +
> > +/* Alternate P2M list lock (per-domain)
> > + *
> > + * A per-domain lock that protects the list of alternate p2m's.
> > + * Any operation that walks the list needs to acquire this lock.
> > + * Additionally, before destroying an alternate p2m all VCPU's
> > + * in the target domain must be paused.
> > + */
> > +
> > +declare_mm_lock(altp2mlist)
> > +#define altp2m_lock(d)   mm_lock(altp2mlist, &(d)->arch.altp2m_lock)
> > +#define altp2m_unlock(d) mm_unlock(&(d)->arch.altp2m_lock)
> > +
> > +/* P2M lock (per-altp2m-table)
> > + *
> > + * This protects all queries and updates to the p2m table.
> > + * Queries may be made under the read lock but all modifications
> > + * need the main (write) lock.
> > + *
> > + * The write lock is recursive as it is common for a code path to look
> > + * up a gfn and later mutate it.
> > + */
> > +
> > +declare_mm_rwlock(altp2m);
> > +#define p2m_lock(p)                         \
> > +{                                           \
> > +    if ( p2m_is_altp2m(p) )                 \
> > +        mm_write_lock(altp2m, &(p)->lock);  \
> > +    else                                    \
> > +        mm_write_lock(p2m, &(p)->lock);     \
> > +}
> >  #define p2m_unlock(p)         mm_write_unlock(&(p)->lock);
> >  #define gfn_lock(p,g,o)       p2m_lock(p)
> >  #define gfn_unlock(p,g,o)     p2m_unlock(p)
> 
> I've just taken on the role of mm maintainer, and so this late in a
> series, having Tim's approval and also having Andy's reviewed-by, I'd
> normally just skim through and Ack it as a matter of course.  But I just
> wouldn't feel right giving this my maintainer's ack without
> understanding the locking a bit better.  So please bear with me here.
> 
> I see in the cover letter that you "sandwiched" the altp2mlist lock
> between p2m and altp2m at Tim's suggestion.  But I can't find the
> discussion where that was suggested (it doesn't seem to be in response
> to v1 patch 5),

I suggested changing the locking order here:
http://lists.xenproject.org/archives/html/xen-devel/2015-01/msg01824.html

Cheers,

Tim.

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