From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov 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 Message-ID: <1f6df7e3-cd05-9b96-9f45-49c433011ff2@cumulusnetworks.com> References: <1509025309-10165-1-git-send-email-nikolay@cumulusnetworks.com> <1509025309-10165-3-git-send-email-nikolay@cumulusnetworks.com> <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, mrv@mojatatu.com, dsa@cumulusnetworks.com, davem@davemloft.net To: Toshiaki Makita , netdev@vger.kernel.org Return-path: In-Reply-To: <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: bridge-bounces@lists.linux-foundation.org Errors-To: bridge-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org 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 > > 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; >> } >> > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cumulusnetworks.com; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=KV4Z3wON+dx6vGMNCHnoWxJlVoWz5lRMtiC+8r3QaOQ=; b=UvGdRQeWiQpv30w9UhYzXwsY4pCiWTjKbsfzmtNvJqTwUjIWTiti4PfJZFdTzg+Ey6 oJSfKQnnIKv6j1XgTkQaN/MoAowyVVEqJ7mumB2e1mZjahSBLWDLyJpkrxz8DcmRXGwM w3kzymJ0RIXqkkivY+kTpfcPJAeUDZZu/hoWk= References: <1509025309-10165-1-git-send-email-nikolay@cumulusnetworks.com> <1509025309-10165-3-git-send-email-nikolay@cumulusnetworks.com> <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> From: Nikolay Aleksandrov Message-ID: <1f6df7e3-cd05-9b96-9f45-49c433011ff2@cumulusnetworks.com> Date: Fri, 27 Oct 2017 11:48:47 +0300 MIME-Version: 1.0 In-Reply-To: <9dd7d80a-1afb-0d22-eb35-73023e65a881@lab.ntt.co.jp> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Toshiaki Makita , netdev@vger.kernel.org Cc: roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, mrv@mojatatu.com, dsa@cumulusnetworks.com, davem@davemloft.net 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 > > 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; >> } >> >