Re: [PATCH v4] lib/dlock-list: Scale dlock_lists_empty()

[Boqun Feng]

On Tue, Nov 07, 2017 at 10:59:29AM -0700, Andreas Dilger wrote:
> On Nov 7, 2017, at 4:59 AM, Jan Kara <jack@xxxxxxx> wrote:
> > On Mon 06-11-17 10:47:08, Davidlohr Bueso wrote:
> >> +	/*
> >> +	 * Serialize dlist->used_lists such that a 0->1 transition is not
> >> +	 * missed by another thread checking if any of the dlock lists are
> >> +	 * used.
> >> +	 *
> >> +	 * CPU0				    CPU1
> >> +	 * dlock_list_add()                 dlock_lists_empty()
> >> +	 *   [S] atomic_inc(used_lists);
> >> +	 *       smp_mb__after_atomic();
> >> +	 *					  smp_mb__before_atomic();
> >> +	 *				      [L] atomic_read(used_lists)
> >> +	 *       list_add()
> >> +	 */
> >> +	smp_mb__before_atomic();
> >> +	return !atomic_read(&dlist->used_lists);
> 
> Just a general kernel programming question here - I thought the whole point
> of atomics is that they are, well, atomic across all CPUs so there is no
> need for a memory barrier?  If there is a need for a memory barrier for
> each atomic access (assuming it isn't accessed under another lock, which would
> make the use of atomic types pointless, IMHO) then I'd think there is a lot
> of code in the kernel that isn't doing this properly.
> 
> What am I missing here?
> 
> I don't see how this helps if the operations are executed like:
> 
> 	 * CPU0				    CPU1
> 	 * dlock_list_add()                 dlock_lists_empty()
> 	 *   [S] atomic_inc(used_lists);
> 	 *					  smp_mb__before_atomic();
> 	 *       smp_mb__after_atomic();
> 	 *				      [L] atomic_read(used_lists)
> 
> or alternately like:
> 
> 	 * CPU0				    CPU1
> 	 * dlock_list_add()                 dlock_lists_empty()
> 	 *					  smp_mb__before_atomic();
> 	 *   [S] atomic_inc(used_lists);
> 	 *       smp_mb__after_atomic();
> 	 *				      [L] atomic_read(used_lists)
> 

Or worse:

 	 * CPU0				    CPU1
 	 * dlock_list_add()                 dlock_lists_empty()
 	 *					  smp_mb__before_atomic();
 	 *				      [L] atomic_read(used_lists)
 	 *   [S] atomic_inc(used_lists);
 	 *       smp_mb__after_atomic();

, in which case dlock_lists_empty() misses a increment of used_lists.

That said, this may be fine for the usage of dlock_list. But I think the
comments are confusing or wrong. The reason is you can not use barriers
to order two memory ops on different CPUs, and usually we add comments
like:

	[S] ...			[S] ...
	<barrier>		<barrier>
	[L] ...			[L] ...

Davidlohr, could you try to improve the comments by finding both memory
ops preceding and following the barriers? Maybe you will find those
barriers are not necessary in the end.

Regards,
Boqun

> then the same problem would exist, unless those functions/macros are somehow
> bound to the atomic operations themselves?  In that case, what makes the use
> of atomic_{inc,dec,read}() in other parts of the code safe without them?
> 
> Cheers, Andreas
> 
> 
> 
> 
>

Attachment: signature.asc
Description: PGP signature