Re: [PATCH net-next 02/14] net_sched: cake: implement lockless cake_dump()

[Simon Horman <horms@kernel.org>]

On Wed, Apr 17, 2024 at 10:54:30AM +0200, Eric Dumazet wrote:
> On Wed, Apr 17, 2024 at 10:35 AM Simon Horman <horms@kernel.org> wrote:
> >
> > + Toke Høiland-Jørgensen <toke@toke.dk>
> >   cake@lists.bufferbloat.net
> >
> > On Mon, Apr 15, 2024 at 01:20:42PM +0000, Eric Dumazet wrote:
> > > Instead of relying on RTNL, cake_dump() can use READ_ONCE()
> > > annotations, paired with WRITE_ONCE() ones in cake_change().
> > >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> >
> > ...
> >
> > > @@ -2774,68 +2783,71 @@ static int cake_dump(struct Qdisc *sch, struct sk_buff *skb)
> > >  {
> > >       struct cake_sched_data *q = qdisc_priv(sch);
> > >       struct nlattr *opts;
> > > +     u16 rate_flags;
> > >
> > >       opts = nla_nest_start_noflag(skb, TCA_OPTIONS);
> > >       if (!opts)
> > >               goto nla_put_failure;
> > >
> > > -     if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64, q->rate_bps,
> > > -                           TCA_CAKE_PAD))
> > > +     if (nla_put_u64_64bit(skb, TCA_CAKE_BASE_RATE64,
> > > +                           READ_ONCE(q->rate_bps), TCA_CAKE_PAD))
> > >               goto nla_put_failure;
> > >
> > >       if (nla_put_u32(skb, TCA_CAKE_FLOW_MODE,
> > > -                     q->flow_mode & CAKE_FLOW_MASK))
> > > +                     READ_ONCE(q->flow_mode) & CAKE_FLOW_MASK))
> > >               goto nla_put_failure;
> >
> > Hi Eric,
> >
> > q->flow_mode is read twice in this function. Once here...
> >
> > >
> > > -     if (nla_put_u32(skb, TCA_CAKE_RTT, q->interval))
> > > +     if (nla_put_u32(skb, TCA_CAKE_RTT, READ_ONCE(q->interval)))
> > >               goto nla_put_failure;
> > >
> > > -     if (nla_put_u32(skb, TCA_CAKE_TARGET, q->target))
> > > +     if (nla_put_u32(skb, TCA_CAKE_TARGET, READ_ONCE(q->target)))
> > >               goto nla_put_failure;
> > >
> > > -     if (nla_put_u32(skb, TCA_CAKE_MEMORY, q->buffer_config_limit))
> > > +     if (nla_put_u32(skb, TCA_CAKE_MEMORY,
> > > +                     READ_ONCE(q->buffer_config_limit)))
> > >               goto nla_put_failure;
> > >
> > > +     rate_flags = READ_ONCE(q->rate_flags);
> > >       if (nla_put_u32(skb, TCA_CAKE_AUTORATE,
> > > -                     !!(q->rate_flags & CAKE_FLAG_AUTORATE_INGRESS)))
> > > +                     !!(rate_flags & CAKE_FLAG_AUTORATE_INGRESS)))
> > >               goto nla_put_failure;
> > >
> > >       if (nla_put_u32(skb, TCA_CAKE_INGRESS,
> > > -                     !!(q->rate_flags & CAKE_FLAG_INGRESS)))
> > > +                     !!(rate_flags & CAKE_FLAG_INGRESS)))
> > >               goto nla_put_failure;
> > >
> > > -     if (nla_put_u32(skb, TCA_CAKE_ACK_FILTER, q->ack_filter))
> > > +     if (nla_put_u32(skb, TCA_CAKE_ACK_FILTER, READ_ONCE(q->ack_filter)))
> > >               goto nla_put_failure;
> > >
> > >       if (nla_put_u32(skb, TCA_CAKE_NAT,
> > > -                     !!(q->flow_mode & CAKE_FLOW_NAT_FLAG)))
> > > +                     !!(READ_ONCE(q->flow_mode) & CAKE_FLOW_NAT_FLAG)))
> > >               goto nla_put_failure;
> >
> > ... and once here.
> >
> > I am assuming that it isn't a big deal, but perhaps it is better to save
> > q->flow_mode into a local variable.
> >
> > Also, more importantly, q->flow_mode does not seem to be handled
> > using WRITE_ONCE() in cake_change(). It's a non-trivial case,
> > which I guess is well served by a mechanism built around a local variable.
> 
> Thanks !
> 
> I will squash in v2:

Likewise, thanks.
This looks good to me.

...