xfs
[Top] [All Lists]

Re: [PATCH 2/2] xfs: another memory barrier before wake_up_bit()

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/2] xfs: another memory barrier before wake_up_bit()
From: Alex Elder <elder@xxxxxxxxxxx>
Date: Mon, 04 Feb 2013 19:38:40 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130204232617.GO2667@dastard>
References: <510FDDE5.4050103@xxxxxxxxxxx> <510FDE23.9050801@xxxxxxxxxxx> <20130204232617.GO2667@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130106 Thunderbird/17.0.2
On 02/04/2013 05:26 PM, Dave Chinner wrote:
> On Mon, Feb 04, 2013 at 10:13:23AM -0600, Alex Elder wrote:
>> In xfs_inode_item_unpin() there is a call to wake_up_bit() following
>> an independent test for whether waiters should be awakened.  This
>> requires a memory barrier in order to guarantee correct operation
>> (see the comment above wake_up_bit()).
>>
>> Signed-off-by: Alex Elder <elder@xxxxxxxxxxx>
>> ---
>>  fs/xfs/xfs_inode_item.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
>> index d041d47..a7cacf7 100644
>> --- a/fs/xfs/xfs_inode_item.c
>> +++ b/fs/xfs/xfs_inode_item.c
>> @@ -474,8 +474,10 @@ xfs_inode_item_unpin(
>>
>>      trace_xfs_inode_unpin(ip, _RET_IP_);
>>      ASSERT(atomic_read(&ip->i_pincount) > 0);
>> -    if (atomic_dec_and_test(&ip->i_pincount))
>> -            wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
>> +    if (!atomic_dec_and_test(&ip->i_pincount))
>> +            return;
>> +    smp_mb();
>> +    wake_up_bit(&ip->i_flags, __XFS_IPINNED_BIT);
> 
> I'm not sure this a barrier is actually needed here.  The "wake up"
> bit is never stored or cleared anywhere in this case, it is used
> only to define a wait channel and directed wake up. Hence the "need
> a barrier so all CPUs see the cleared bit" case doesn't arise here.
> We use an atomic variable instead, and that makes it safe.
> 
> If you read Documentation/atomic_ops.txt, you'll find that atomic
> modification operations are required to have explicit barrier
> semantics. i.e. that atomic_dec_and_test() must behave like it has
> both a smp_mb() before and after the atomic operation. i.e:
> 
>       Unlike the above routines, it is required that explicit memory
>       barriers are performed before and after the operation.  It must be
>       done such that all memory operations before and after the atomic
>       operation calls are strongly ordered with respect to the atomic
>       operation itself.
> 
> So, the smp_mb() that is added here is redundant - the
> atomic_dec_and_test() call already has the necesary memory barriers
> that wake_up_bit() requires.

I hadn't looked at that in as much detail, but now that you point it
out I concur.

I retract this patch.

Thanks.

                                        -Alex

> 
> Cheers,
> 
> Dave.
> 

<Prev in Thread] Current Thread [Next in Thread>