Re: [PATCHv3 1/2] spinlock: move rwlock API and per-cpu rwlocks into their own files

["Jan Beulich" <JBeulich@suse.com>]

>>> On 01.02.16 at 12:31, <david.vrabel@citrix.com> wrote:
> From: Jennifer Herbert <jennifer.herbert@citrix.com>
> 
> 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 <jennifer.herbert@citrix.com>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

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 <xen/spinlock.h>

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