From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.bufferbloat.net (Postfix) with ESMTPS id C9CE43CB37 for ; Wed, 17 Apr 2024 04:54:42 -0400 (EDT) Received: by mail-wm1-x333.google.com with SMTP id 5b1f17b1804b1-41699bbfb91so46265e9.0 for ; Wed, 17 Apr 2024 01:54:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1713344082; x=1713948882; darn=lists.bufferbloat.net; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=D2Q0qgouMY0HxajVxeo2A2P9aG8P/wFEHGo3jktH7h0=; b=qxzeZFVYafcDdGERqMiVxX/IEGqJNa0P9nrU42iAlPUUaJUmyBiRFp5HzBHs/BG8fD 5BlzrOHqOOCr/KYmisyTv+uq6XqvCZbbMaQPgIAafwjkdW1pyrQLY8mG7HCKBVfP2/5l uaQZtf3KCrrjxBfUd3TlTbI0b3OppJdC4POs1rP0DapricqcIVrqDvk5MFZB3MaHwTgu 1GdjGQ8UWQxMkGvKiNOqDp3PzGvcTo2p1BgWLaD/X7blwS10FH6LmT6em98Wu1LKMii4 dVaoE6OzRzZd1ViQaboc/qTtBryDXzu8sm4eXFV2460QhgjxLO+MgdFcb5L/Bj+FWmCp 6gtQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713344082; x=1713948882; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=D2Q0qgouMY0HxajVxeo2A2P9aG8P/wFEHGo3jktH7h0=; b=M0wf1k88RLLCJ/MUZK/u+9TvqDsPnHSN9sEAw8mo3y1MoeP8z6+m+KDsOi96YAWFu/ buOtFGx6/EFAcMuW3xEPaFPDgC/fPxNkB5/AP2Ezv04g7hkel48qSTj9Wkmqv7Z04t5k ak9jSdMUPht56VVi6FK6+U2fKFvx3uGn7XNGCvdEL7YZtW69Bqy92FarvH26Q9+FEgv7 Eo+iCNzeXfmE5rf5Q4/PKle4OrKSjrhxKl1DuC9wCEjTgcq6vdfIaU/vDPUeeURDLaTB KtmBarnFYsITqZ/mbt4CEi3cuoglgtUBG7k8D6Zz/GOIB0Ir4L5F2kbU8j1PumQNAJUp fGbQ== X-Forwarded-Encrypted: i=1; AJvYcCWrEC0F3mh5+pGeoZ/LsSCjP2NednHdDLrm51tKXh0NzQWLrbRnHEjq7KwyUi2HFRgjzm4B3c0NWpt9HY4/UJ/hluQncAR7O9jLMw== X-Gm-Message-State: AOJu0Yxq2080bX0upv2cx2hKZu4gmx3go6SefHcHrG6Mb3gy1M6IJMko eSF4SA9+cBoB5toWX0h+fubYJYktXnn5A96pB1b0z6/2xse7EIvmPNvgI+mMg4WRNm3AE4WvniM 6vniIhifDalzrmpUK3ApV+31myi8mqkjS4iVl X-Google-Smtp-Source: AGHT+IGmZMmrMNfHNZNI0CE9mmS9XBy4pTP277sxXwhsYJGqIlMVTOzbwRfRxIrQ0gVvHZ+nTB9CIRDpfvo42LwGJjE= X-Received: by 2002:a05:600c:1d28:b0:418:138e:f27c with SMTP id l40-20020a05600c1d2800b00418138ef27cmr150776wms.6.1713344081402; Wed, 17 Apr 2024 01:54:41 -0700 (PDT) MIME-Version: 1.0 References: <20240415132054.3822230-1-edumazet@google.com> <20240415132054.3822230-3-edumazet@google.com> <20240417083549.GA3846178@kernel.org> In-Reply-To: <20240417083549.GA3846178@kernel.org> From: Eric Dumazet Date: Wed, 17 Apr 2024 10:54:30 +0200 Message-ID: To: Simon Horman Cc: "David S. Miller" , Jakub Kicinski , Paolo Abeni , Jamal Hadi Salim , Cong Wang , Jiri Pirko , netdev@vger.kernel.org, eric.dumazet@gmail.com, =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= , cake@lists.bufferbloat.net Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [Cake] [PATCH net-next 02/14] net_sched: cake: implement lockless cake_dump() X-BeenThere: cake@lists.bufferbloat.net X-Mailman-Version: 2.1.20 Precedence: list List-Id: Cake - FQ_codel the next generation List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Apr 2024 08:54:43 -0000 On Wed, Apr 17, 2024 at 10:35=E2=80=AFAM Simon Horman wr= ote: > > + Toke H=C3=B8iland-J=C3=B8rgensen > 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 > > ... > > > @@ -2774,68 +2783,71 @@ static int cake_dump(struct Qdisc *sch, struct = sk_buff *skb) > > { > > struct cake_sched_data *q =3D qdisc_priv(sch); > > struct nlattr *opts; > > + u16 rate_flags; > > > > opts =3D 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 =3D 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..9602dafe32e61d38dc00b0a35e1= ee3f530989610 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 =3D qdisc_priv(sch); struct nlattr *tb[TCA_CAKE_MAX + 1]; u16 rate_flags; + u8 flow_mode; int err; err =3D nla_parse_nested_deprecated(tb, TCA_CAKE_MAX, opt, cake_pol= icy, @@ -2580,10 +2581,11 @@ static int cake_change(struct Qdisc *sch, struct nlattr *opt, if (err < 0) return err; + flow_mode =3D q->flow_mode; if (tb[TCA_CAKE_NAT]) { #if IS_ENABLED(CONFIG_NF_CONNTRACK) - q->flow_mode &=3D ~CAKE_FLOW_NAT_FLAG; - q->flow_mode |=3D CAKE_FLOW_NAT_FLAG * + flow_mode &=3D ~CAKE_FLOW_NAT_FLAG; + flow_mode |=3D 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 =3D ((q->flow_mode & CAKE_FLOW_NAT_FLAG) | + flow_mode =3D ((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 =3D qdisc_priv(sch); struct nlattr *opts; u16 rate_flags; + u8 flow_mode; opts =3D 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 =3D 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)= ))