From: Eric Dumazet <edumazet@google.com>
To: Simon Horman <horms@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>,
"Jakub Kicinski" <kuba@kernel.org>,
"Paolo Abeni" <pabeni@redhat.com>,
"Jamal Hadi Salim" <jhs@mojatatu.com>,
"Cong Wang" <xiyou.wangcong@gmail.com>,
"Jiri Pirko" <jiri@resnulli.us>,
netdev@vger.kernel.org, eric.dumazet@gmail.com,
"Toke Høiland-Jørgensen" <toke@toke.dk>,
cake@lists.bufferbloat.net
Subject: Re: [Cake] [PATCH net-next 02/14] net_sched: cake: implement lockless cake_dump()
Date: Wed, 17 Apr 2024 10:54:30 +0200 [thread overview]
Message-ID: <CANn89i+MF3F8Q063USmLYvyrffBKQRvb3ZM2c1MhAbFOwk9B-A@mail.gmail.com> (raw)
In-Reply-To: <20240417083549.GA3846178@kernel.org>
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:
diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index bb37a0dedcc1e4b3418f6681d87108aad7ea066f..9602dafe32e61d38dc00b0a35e1ee3f530989610
100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -2573,6 +2573,7 @@ static int cake_change(struct Qdisc *sch, struct
nlattr *opt,
struct cake_sched_data *q = qdisc_priv(sch);
struct nlattr *tb[TCA_CAKE_MAX + 1];
u16 rate_flags;
+ u8 flow_mode;
int err;
err = nla_parse_nested_deprecated(tb, TCA_CAKE_MAX, opt, cake_policy,
@@ -2580,10 +2581,11 @@ static int cake_change(struct Qdisc *sch,
struct nlattr *opt,
if (err < 0)
return err;
+ flow_mode = q->flow_mode;
if (tb[TCA_CAKE_NAT]) {
#if IS_ENABLED(CONFIG_NF_CONNTRACK)
- q->flow_mode &= ~CAKE_FLOW_NAT_FLAG;
- q->flow_mode |= CAKE_FLOW_NAT_FLAG *
+ flow_mode &= ~CAKE_FLOW_NAT_FLAG;
+ flow_mode |= CAKE_FLOW_NAT_FLAG *
!!nla_get_u32(tb[TCA_CAKE_NAT]);
#else
NL_SET_ERR_MSG_ATTR(extack, tb[TCA_CAKE_NAT],
@@ -2609,7 +2611,7 @@ static int cake_change(struct Qdisc *sch, struct
nlattr *opt,
}
if (tb[TCA_CAKE_FLOW_MODE])
- q->flow_mode = ((q->flow_mode & CAKE_FLOW_NAT_FLAG) |
+ flow_mode = ((flow_mode & CAKE_FLOW_NAT_FLAG) |
(nla_get_u32(tb[TCA_CAKE_FLOW_MODE]) &
CAKE_FLOW_MASK));
@@ -2689,6 +2691,7 @@ static int cake_change(struct Qdisc *sch, struct
nlattr *opt,
}
WRITE_ONCE(q->rate_flags, rate_flags);
+ WRITE_ONCE(q->flow_mode, flow_mode);
if (q->tins) {
sch_tree_lock(sch);
cake_reconfigure(sch);
@@ -2784,6 +2787,7 @@ 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;
+ u8 flow_mode;
opts = nla_nest_start_noflag(skb, TCA_OPTIONS);
if (!opts)
@@ -2793,8 +2797,8 @@ static int cake_dump(struct Qdisc *sch, struct
sk_buff *skb)
READ_ONCE(q->rate_bps), TCA_CAKE_PAD))
goto nla_put_failure;
- if (nla_put_u32(skb, TCA_CAKE_FLOW_MODE,
- READ_ONCE(q->flow_mode) & CAKE_FLOW_MASK))
+ flow_mode = READ_ONCE(q->flow_mode);
+ if (nla_put_u32(skb, TCA_CAKE_FLOW_MODE, flow_mode & CAKE_FLOW_MASK))
goto nla_put_failure;
if (nla_put_u32(skb, TCA_CAKE_RTT, READ_ONCE(q->interval)))
@@ -2820,7 +2824,7 @@ static int cake_dump(struct Qdisc *sch, struct
sk_buff *skb)
goto nla_put_failure;
if (nla_put_u32(skb, TCA_CAKE_NAT,
- !!(READ_ONCE(q->flow_mode) & CAKE_FLOW_NAT_FLAG)))
+ !!(flow_mode & CAKE_FLOW_NAT_FLAG)))
goto nla_put_failure;
if (nla_put_u32(skb, TCA_CAKE_DIFFSERV_MODE, READ_ONCE(q->tin_mode)))
next parent reply other threads:[~2024-04-17 8:54 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240415132054.3822230-1-edumazet@google.com>
[not found] ` <20240415132054.3822230-3-edumazet@google.com>
[not found] ` <20240417083549.GA3846178@kernel.org>
2024-04-17 8:54 ` Eric Dumazet [this message]
2024-04-17 12:25 ` Toke Høiland-Jørgensen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://lists.bufferbloat.net/postorius/lists/cake.lists.bufferbloat.net/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANn89i+MF3F8Q063USmLYvyrffBKQRvb3ZM2c1MhAbFOwk9B-A@mail.gmail.com \
--to=edumazet@google.com \
--cc=cake@lists.bufferbloat.net \
--cc=davem@davemloft.net \
--cc=eric.dumazet@gmail.com \
--cc=horms@kernel.org \
--cc=jhs@mojatatu.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=toke@toke.dk \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox