Re: [PATCH net-next 07/14] net_sched: sch_ets: implement lockless ets_dump()

[Simon Horman <horms@kernel.org>]

On Mon, Apr 15, 2024 at 01:20:47PM +0000, Eric Dumazet wrote:
> Instead of relying on RTNL, ets_dump() can use READ_ONCE()
> annotations, paired with WRITE_ONCE() ones in ets_change().
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/sched/sch_ets.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
> index 835b4460b44854a803d3054e744702022b7551f4..f80bc05d4c5a5050226e6cfd30fa951c0b61029f 100644
> --- a/net/sched/sch_ets.c
> +++ b/net/sched/sch_ets.c

...

> @@ -658,11 +658,11 @@ static int ets_qdisc_change(struct Qdisc *sch, struct nlattr *opt,
>  			list_del(&q->classes[i].alist);
>  		qdisc_tree_flush_backlog(q->classes[i].qdisc);
>  	}
> -	q->nstrict = nstrict;
> +	WRITE_ONCE(q->nstrict, nstrict);
>  	memcpy(q->prio2band, priomap, sizeof(priomap));

Hi Eric,

I think that writing elements of q->prio2band needs WRITE_ONCE() treatment too.

>  	for (i = 0; i < q->nbands; i++)
> -		q->classes[i].quantum = quanta[i];
> +		WRITE_ONCE(q->classes[i].quantum, quanta[i]);
>  
>  	for (i = oldbands; i < q->nbands; i++) {
>  		q->classes[i].qdisc = queues[i];

...

> @@ -733,6 +733,7 @@ static int ets_qdisc_dump(struct Qdisc *sch, struct sk_buff *skb)
>  	struct ets_sched *q = qdisc_priv(sch);
>  	struct nlattr *opts;
>  	struct nlattr *nest;
> +	u8 nbands, nstrict;
>  	int band;
>  	int prio;
>  	int err;

The next few lines of this function are:

	err = ets_offload_dump(sch);
	if (err)
		return err;

Where ets_offload_dump may indirectly call ndo_setup_tc().
And I am concerned that ndo_setup_tc() expects RTNL to be held,
although perhaps that assumption is out of date.

...

> @@ -771,7 +773,8 @@ static int ets_qdisc_dump(struct Qdisc *sch, struct sk_buff *skb)
>  		goto nla_err;
>  
>  	for (prio = 0; prio <= TC_PRIO_MAX; prio++) {
> -		if (nla_put_u8(skb, TCA_ETS_PRIOMAP_BAND, q->prio2band[prio]))
> +		if (nla_put_u8(skb, TCA_ETS_PRIOMAP_BAND,
> +			       READ_ONCE(q->prio2band[prio])))
>  			goto nla_err;
>  	}

...