From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCHv3 1/2] spinlock: move rwlock API and per-cpu rwlocks into their own files Date: Wed, 03 Feb 2016 04:22:30 -0700 Message-ID: <56B1F10602000078000CDEFF@prv-mh.provo.novell.com> References: <1454326273-23588-1-git-send-email-david.vrabel@citrix.com> <1454326273-23588-2-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aQvW7-00010E-SC for xen-devel@lists.xenproject.org; Wed, 03 Feb 2016 11:22:35 +0000 In-Reply-To: <1454326273-23588-2-git-send-email-david.vrabel@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel , Jennifer Herbert Cc: xen-devel@lists.xenproject.org, Ian Campbell List-Id: xen-devel@lists.xenproject.org >>> On 01.02.16 at 12:31, wrote: > From: Jennifer Herbert > > In preparation for a replacement read-write lock implementation, move > the API and the per-cpu read-write locks into their own files. > > Signed-off-by: Jennifer Herbert > Signed-off-by: David Vrabel I suppose this was meant to also state "no functional change" or "no change to generated code" or some such? > --- /dev/null > +++ b/xen/include/xen/rwlock.h > @@ -0,0 +1,150 @@ > +#ifndef __RWLOCK_H__ > +#define __RWLOCK_H__ > + > +#include I suppose you need this because ... > +#define read_lock(l) _read_lock(l) > +#define read_lock_irq(l) _read_lock_irq(l) > +#define read_lock_irqsave(l, f) \ > + ({ \ > + BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \ > + ((f) = _read_lock_irqsave(l)); \ > + }) > + > +#define read_unlock(l) _read_unlock(l) > +#define read_unlock_irq(l) _read_unlock_irq(l) > +#define read_unlock_irqrestore(l, f) _read_unlock_irqrestore(l, f) > +#define read_trylock(l) _read_trylock(l) > + > +#define write_lock(l) _write_lock(l) > +#define write_lock_irq(l) _write_lock_irq(l) > +#define write_lock_irqsave(l, f) \ > + ({ \ > + BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \ > + ((f) = _write_lock_irqsave(l)); \ > + }) > +#define write_trylock(l) _write_trylock(l) > + > +#define write_unlock(l) _write_unlock(l) > +#define write_unlock_irq(l) _write_unlock_irq(l) > +#define write_unlock_irqrestore(l, f) _write_unlock_irqrestore(l, f) > + > +#define rw_is_locked(l) _rw_is_locked(l) > +#define rw_is_write_locked(l) _rw_is_write_locked(l) ... you move all the macro wrappers, but not the underlying function declarations? Why is that? Apart from appearing inconsistent, getting rid of the seeming stray include above would be nice. Or, looking at the next patch, was this instead done with the goal of making that other patch more easily reviewable? In which case this intention should have been stated at least after the first --- separator above, and the second patch should then remove the then unnecessary include again (and likely put some other includes there which tight now get pulled in via xen/spinlock.h). Jan