On Tue, Oct 24, 2006 at 10:38:22PM +0100, Christoph Hellwig wrote:
> > +static inline int
> > +__xfs_iflags_test(xfs_inode_t *ip, unsigned short flags)
> > +{
> > + return (ip->i_flags & flags);
> > +}
> > +
> > +static inline int
> > +xfs_iflags_test(xfs_inode_t *ip, unsigned short flags)
> > +{
> > + int ret;
> > + spin_lock(&ip->i_flags_lock);
> > + ret = __xfs_iflags_test(ip, flags);
> > + spin_unlock(&ip->i_flags_lock);
> > + return ret;
>
> This is not actually guaranteed to work on machiens with very weak
> memory ordering. Please use the *_bit routines from bitops.h instead.
Hmm - don't you have that the wrong way around?
Documentation/memory-barriers.txt:
...
Therefore, from (1), (2) and (4) an UNLOCK followed by an unconditional LOCK is
equivalent to a full barrier, but a LOCK followed by an UNLOCK is not.
....
But the bitops don't guarantee ordering or barriers e.g. from
include/asm-i386/bitops.h:
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
* @addr: the address to start counting from
*
* This function is atomic and may not be reordered. See __set_bit()
* if you do not require the atomic guarantees.
*
* Note: there are no guarantees that this function will not be reordered
* on non x86 architectures, so if you are writting portable code,
* make sure not to rely on its reordering guarantees.
*
* Note that @nr may be almost arbitrarily large; this function is not
* restricted to acting on a single-word quantity.
*/
So I think the code is fine as it stands.
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
|