[Differential] D8637: buf_ring.h: fix memory order issues.

[alc (Alan Cox)]

alc added a comment.


  Have you looked at https://reviews.freebsd.org/D1945, in particular, the most recent postings by sbahra_repnop.org?  It's not clear to me that these changes will address the problem described in sbahra_repnop.org's postings.  That said, your proposed changes do correct the most obvious remaining issues with the use of acquires and releases in this code.

INLINE COMMENTS

> buf_ring.h:78
>  	do {
> +		cons_tail = atomic_load_acq_32(&br->br_cons_tail);
>  		prod_head = br->br_prod_head;

Was there a reason for moving this load?

> buf_ring.h:98
>  	 */   
>  	while (br->br_prod_tail != prod_head)
>  		cpu_spinwait();

You may need to use a load acquire on br_prod_tail here to establish an unbroken synchronizes-with chain between the thread that enqueues an item X and the thread that later dequeues it if there are other concurrent enqueues.

> buf_ring.h:117
>  	do {
> +		prod_tail = atomic_load_acq_32(&br->br_prod_tail);
>  		cons_head = br->br_cons_head;

Was there a reason for moving this load?

> buf_ring.h:159
>  	prod_tail = atomic_load_acq_32(&br->br_prod_tail);
> -	
> -	cons_next = (cons_head + 1) & br->br_cons_mask;
> -#ifdef PREFETCH_DEFINED
> -	cons_next_next = (cons_head + 2) & br->br_cons_mask;
> -#endif
> +	cons_head = br->br_cons_head;
>  	

Was there a reason for swapping the order of the preceding loads?

> buf_ring.h:174
>  	buf = br->br_ring[cons_head];
> +	br->br_cons_head = cons_next;
>  

Was there a reason for swapping the order of the preceding loads?

REVISION DETAIL
  https://reviews.freebsd.org/D8637

EMAIL PREFERENCES
  https://reviews.freebsd.org/settings/panel/emailpreferences/

To: oleg, kmacy, kib, alc
Cc: emaste, freebsd-net-list