From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> To: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>, netdev@vger.kernel.org Cc: roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, mrv@mojatatu.com, dsa@cumulusnetworks.com, davem@davemloft.net Subject: Re: [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add Date: Fri, 27 Oct 2017 11:48:47 +0300 [thread overview] Message-ID: <1f6df7e3-cd05-9b96-9f45-49c433011ff2@cumulusnetworks.com> (raw) In-Reply-To: <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> On 27/10/17 11:24, Toshiaki Makita wrote: > On 2017/10/26 22:41, Nikolay Aleksandrov wrote: >> Before this patch there was no way to tell if the vlan add operation >> actually changed anything, thus we would always generate a notification >> on adds. Let's make the notifications more precise and generate them >> only if anything changed, so use the new bool parameter to signal that the >> vlan was updated. We cannot return an error because there are valid use >> cases that will be broken (e.g. overlapping range add) and also we can't >> risk masking errors due to calls into drivers for vlan add which can >> potentially return anything. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > Thanks for the explanation in the previous mail. > >> --- > ... >> @@ -576,8 +592,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> refcount_inc(&vlan->refcnt); >> vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY; >> vg->num_vlans++; >> + *changed = true; >> } >> - __vlan_add_flags(vlan, flags); >> + if (__vlan_add_flags(vlan, flags)) >> + *changed = true; >> + >> return 0; >> } >> >> @@ -601,6 +620,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> free_percpu(vlan->stats); >> kfree(vlan); > > We should not set changed=true in this failure case? > Right, I don't know what I was thinking, clearly I missed the fact that we directly clean up and return "ret" with an error. Very good catch, thanks! That should go in the else branch, will send a v6 later with that fixed up. Thanks, Nik >> } >> + *changed = true; >> >> return ret; >> } > ... >> @@ -1050,6 +1083,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) >> ret = __vlan_add(vlan, flags); >> if (ret) >> kfree(vlan); > > Likewise. > >> + *changed = true; >> >> return ret; >> } >> >
WARNING: multiple messages have this Message-ID (diff)
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> To: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>, netdev@vger.kernel.org Cc: roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, mrv@mojatatu.com, dsa@cumulusnetworks.com, davem@davemloft.net Subject: Re: [Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add Date: Fri, 27 Oct 2017 11:48:47 +0300 [thread overview] Message-ID: <1f6df7e3-cd05-9b96-9f45-49c433011ff2@cumulusnetworks.com> (raw) In-Reply-To: <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> On 27/10/17 11:24, Toshiaki Makita wrote: > On 2017/10/26 22:41, Nikolay Aleksandrov wrote: >> Before this patch there was no way to tell if the vlan add operation >> actually changed anything, thus we would always generate a notification >> on adds. Let's make the notifications more precise and generate them >> only if anything changed, so use the new bool parameter to signal that the >> vlan was updated. We cannot return an error because there are valid use >> cases that will be broken (e.g. overlapping range add) and also we can't >> risk masking errors due to calls into drivers for vlan add which can >> potentially return anything. >> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> > > Thanks for the explanation in the previous mail. > >> --- > ... >> @@ -576,8 +592,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> refcount_inc(&vlan->refcnt); >> vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY; >> vg->num_vlans++; >> + *changed = true; >> } >> - __vlan_add_flags(vlan, flags); >> + if (__vlan_add_flags(vlan, flags)) >> + *changed = true; >> + >> return 0; >> } >> >> @@ -601,6 +620,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags) >> free_percpu(vlan->stats); >> kfree(vlan); > > We should not set changed=true in this failure case? > Right, I don't know what I was thinking, clearly I missed the fact that we directly clean up and return "ret" with an error. Very good catch, thanks! That should go in the else branch, will send a v6 later with that fixed up. Thanks, Nik >> } >> + *changed = true; >> >> return ret; >> } > ... >> @@ -1050,6 +1083,7 @@ int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) >> ret = __vlan_add(vlan, flags); >> if (ret) >> kfree(vlan); > > Likewise. > >> + *changed = true; >> >> return ret; >> } >> >
next prev parent reply other threads:[~2017-10-27 8:48 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-10-26 13:41 [PATCH net-next v5 0/2] bridge: make setlink/dellink notifications more accurate Nikolay Aleksandrov 2017-10-26 13:41 ` [Bridge] " Nikolay Aleksandrov 2017-10-26 13:41 ` [PATCH net-next v5 1/2] bridge: netlink: " Nikolay Aleksandrov 2017-10-26 13:41 ` [Bridge] " Nikolay Aleksandrov 2017-10-26 13:41 ` [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add Nikolay Aleksandrov 2017-10-26 13:41 ` [Bridge] " Nikolay Aleksandrov 2017-10-27 1:55 ` Toshiaki Makita 2017-10-27 1:55 ` [Bridge] " Toshiaki Makita 2017-10-27 7:49 ` Nikolay Aleksandrov 2017-10-27 7:49 ` [Bridge] " Nikolay Aleksandrov 2017-10-27 8:24 ` Toshiaki Makita 2017-10-27 8:24 ` [Bridge] " Toshiaki Makita 2017-10-27 8:48 ` Nikolay Aleksandrov [this message] 2017-10-27 8:48 ` Nikolay Aleksandrov 2017-10-27 10:19 ` [PATCH net-next v6 0/2] bridge: make setlink/dellink notifications more accurate Nikolay Aleksandrov 2017-10-27 10:19 ` [Bridge] " Nikolay Aleksandrov 2017-10-27 10:19 ` [PATCH net-next v6 1/2] bridge: netlink: " Nikolay Aleksandrov 2017-10-27 10:19 ` [Bridge] " Nikolay Aleksandrov 2017-10-27 10:19 ` [PATCH net-next v6 2/2] bridge: vlan: signal if anything changed on vlan add Nikolay Aleksandrov 2017-10-27 10:19 ` [Bridge] " Nikolay Aleksandrov 2017-10-27 23:34 ` Toshiaki Makita 2017-10-27 23:34 ` [Bridge] " Toshiaki Makita 2017-10-29 2:04 ` [PATCH net-next v6 0/2] bridge: make setlink/dellink notifications more accurate David Miller 2017-10-29 2:04 ` [Bridge] " David Miller
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 * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1f6df7e3-cd05-9b96-9f45-49c433011ff2@cumulusnetworks.com \ --to=nikolay@cumulusnetworks.com \ --cc=bridge@lists.linux-foundation.org \ --cc=davem@davemloft.net \ --cc=dsa@cumulusnetworks.com \ --cc=makita.toshiaki@lab.ntt.co.jp \ --cc=mrv@mojatatu.com \ --cc=netdev@vger.kernel.org \ --cc=roopa@cumulusnetworks.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.