Re: [PATCH net 4/5] net/sched: sch_cake: annotate data-races in cake_dump_stats() (IV)

["Toke Høiland-Jørgensen" <toke@toke.dk>]

Eric Dumazet <edumazet@google.com> writes:

> cake_dump_stats() runs without qdisc spinlock being held.
>
> In this fourth patch, I add READ_ONCE()/WRITE_ONCE() annotations
> for the following fields:
>
> - avg_peak_bandwidth
> - buffer_limit
> - buffer_max_used
> - avg_netoff
> - max_netlen
> - max_adjlen
> - min_netlen
> - min_adjlen
> - active_queues
> - tin_rate_bps
> - bytes
> - tin_backlog
>
> Other annotations are added in following patch, to ease code review.
>
> Fixes: 046f6fd5daef ("sched: Add Common Applications Kept Enhanced (cake) qdisc")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "Toke Høiland-Jørgensen" <toke@toke.dk>
> ---
>  net/sched/sch_cake.c | 90 ++++++++++++++++++++++----------------------
>  1 file changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index c5aae31565e984e40937b55201b498174a37180e..c3b09f67f0fdbc51d23b3d22df9ab89a716c7e2b 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1379,9 +1379,9 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
>  		len -= off;
>  
>  	if (qd->max_netlen < len)
> -		qd->max_netlen = len;
> +		WRITE_ONCE(qd->max_netlen, len);
>  	if (qd->min_netlen > len)
> -		qd->min_netlen = len;
> +		WRITE_ONCE(qd->min_netlen, len);
>  
>  	len += q->rate_overhead;
>  
> @@ -1401,9 +1401,9 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
>  	}
>  
>  	if (qd->max_adjlen < len)
> -		qd->max_adjlen = len;
> +		WRITE_ONCE(qd->max_adjlen, len);
>  	if (qd->min_adjlen > len)
> -		qd->min_adjlen = len;
> +		WRITE_ONCE(qd->min_adjlen, len);
>  
>  	return len;
>  }
> @@ -1416,7 +1416,7 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  	u16 segs = qdisc_pkt_segs(skb);
>  	u32 len = qdisc_pkt_len(skb);
>  
> -	q->avg_netoff = cake_ewma(q->avg_netoff, off << 16, 8);
> +	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
>  
>  	if (segs == 1)
>  		return cake_calc_overhead(q, len, off);
> @@ -1596,7 +1596,7 @@ static unsigned int cake_drop(struct Qdisc *sch, struct sk_buff **to_free)
>  	len = qdisc_pkt_len(skb);
>  	q->buffer_used      -= skb->truesize;
>  	b->backlogs[idx]    -= len;
> -	b->tin_backlog      -= len;
> +	WRITE_ONCE(b->tin_backlog, b->tin_backlog - len);
>  	sch->qstats.backlog -= len;
>  
>  	flow->dropped++;
> @@ -1824,9 +1824,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  		}
>  
>  		/* stats */
> -		b->bytes	    += slen;
> +		WRITE_ONCE(b->bytes, b->bytes + slen);
>  		b->backlogs[idx]    += slen;
> -		b->tin_backlog      += slen;
> +		WRITE_ONCE(b->tin_backlog, b->tin_backlog + slen);
>  		sch->qstats.backlog += slen;
>  		q->avg_window_bytes += slen;

nit: these break up the aligned block, which hurts readability, IMO. Can
we keep the WRITE_ONCE() lines at the top or the bottom of the block?

> @@ -1847,7 +1847,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  			WRITE_ONCE(b->ack_drops, b->ack_drops + 1);
>  			sch->qstats.drops++;
>  			ack_pkt_len = qdisc_pkt_len(ack);
> -			b->bytes += ack_pkt_len;
> +			WRITE_ONCE(b->bytes, b->bytes + ack_pkt_len);
>  			q->buffer_used += skb->truesize - ack->truesize;
>  			if (q->config->rate_flags & CAKE_FLAG_INGRESS)
>  				cake_advance_shaper(q, b, ack, now, true);
> @@ -1861,9 +1861,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  
>  		/* stats */
>  		WRITE_ONCE(b->packets, b->packets + 1);
> -		b->bytes	    += len - ack_pkt_len;
> +		WRITE_ONCE(b->bytes, b->bytes + len - ack_pkt_len);
>  		b->backlogs[idx]    += len - ack_pkt_len;
> -		b->tin_backlog      += len - ack_pkt_len;
> +		WRITE_ONCE(b->tin_backlog, b->tin_backlog + len - ack_pkt_len);
>  		sch->qstats.backlog += len - ack_pkt_len;
>  		q->avg_window_bytes += len - ack_pkt_len;

same here
>  	}
> @@ -1895,9 +1895,9 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  			u64 b = q->avg_window_bytes * (u64)NSEC_PER_SEC;
>  
>  			b = div64_u64(b, window_interval);
> -			q->avg_peak_bandwidth =
> -				cake_ewma(q->avg_peak_bandwidth, b,
> -					  b > q->avg_peak_bandwidth ? 2 : 8);
> +			WRITE_ONCE(q->avg_peak_bandwidth,
> +				   cake_ewma(q->avg_peak_bandwidth, b,
> +					     b > q->avg_peak_bandwidth ? 2 : 8));
>  			q->avg_window_bytes = 0;
>  			q->avg_window_begin = now;
>  
> @@ -1938,7 +1938,7 @@ static s32 cake_enqueue(struct sk_buff *skb, struct Qdisc *sch,
>  	}
>  
>  	if (q->buffer_used > q->buffer_max_used)
> -		q->buffer_max_used = q->buffer_used;
> +		WRITE_ONCE(q->buffer_max_used, q->buffer_used);
>  
>  	if (q->buffer_used <= q->buffer_limit)
>  		return NET_XMIT_SUCCESS;
> @@ -1978,7 +1978,7 @@ static struct sk_buff *cake_dequeue_one(struct Qdisc *sch)
>  		skb = dequeue_head(flow);
>  		len = qdisc_pkt_len(skb);
>  		b->backlogs[q->cur_flow] -= len;
> -		b->tin_backlog		 -= len;
> +		WRITE_ONCE(b->tin_backlog, b->tin_backlog - len);
>  		sch->qstats.backlog      -= len;
>  		q->buffer_used		 -= skb->truesize;
>  		sch->q.qlen--;

and here


-Toke