From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nikolay Aleksandrov Subject: Re: [PATCH net-next] net: bridge: add support for sticky fdb entries Date: Tue, 11 Sep 2018 02:55:10 +0300 Message-ID: <38bfcada-abea-be8d-c794-154113ca17f8@cumulusnetworks.com> References: <20180910101601.28073-1-nikolay@cumulusnetworks.com> <20180910141802.268e85c5@shemminger-XPS-13-9360> <886aea12-29f7-8722-42c9-5db2f3f34421@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com, davem@davemloft.net, bridge@lists.linux-foundation.org To: Grygorii Strashko , Stephen Hemminger Return-path: Received: from mail-wr1-f65.google.com ([209.85.221.65]:44255 "EHLO mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725945AbeIKEwA (ORCPT ); Tue, 11 Sep 2018 00:52:00 -0400 Received: by mail-wr1-f65.google.com with SMTP id v16-v6so23689259wro.11 for ; Mon, 10 Sep 2018 16:55:31 -0700 (PDT) In-Reply-To: <886aea12-29f7-8722-42c9-5db2f3f34421@ti.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 11/09/18 02:22, Grygorii Strashko wrote: > > > On 09/10/2018 04:18 PM, Stephen Hemminger wrote: >> On Mon, 10 Sep 2018 13:16:01 +0300 >> Nikolay Aleksandrov wrote: >> >>> Add support for entries which are "sticky", i.e. will not change their port >>> if they show up from a different one. A new ndm flag is introduced for that >>> purpose - NTF_STICKY. We allow to set it only to non-local entries. >> >> Is there a name for this in other network switch API's? > > In TI CPSW hardware we have following definition for similar FDB/ALE entries > (AM437x TRM - 15.3.2.7.1.4 Unicast Address Table Entry): > > Block (BLOCK) – The block bit indicates that a packet with a matching source or > destination address should be dropped (block the address). > 0 – Address is not blocked. > 1 – Drop a packet with a matching source or destination address (secure must be zero) > If block and secure are both set, then they no longer mean block and secure. > When both are set, the block and secure bits indicate that the packet is > a unicast supervisory (super) packet and they determine the unicast forward state test criteria. > If both bits are set then the packet is forwarded if the receive port is in > the Forwarding/Blocking/Learning state. > If both bits are not set then the packet is forwarded if the receive port is in > the Forwarding state. > > Secure (SECURE) - This bit indicates that a packet with a matching source address > should be dropped if the received port number is not equal to the table entry port_number. > 0 – Received port number is a don’t care. > 1 – Drop the packet if the received port is not the secure port for the source > address and do not update the address (block must be zero) > > Updating Process: > if ((source address found) and (receive port number != port_number) and (secure or block)) > then do not update address > > "sticky" would mean SECURE+BLOCK = supervisory (super) packet > That could work. but we haven't really made any arrangements w.r.t. to current implementations. So if IUC secure+block should be: - block: 1 – Drop a packet with a matching source or destination address (secure must be zero) If block and secure are both set, then they no longer mean block and secure. - secure: 1 – Drop the packet if the received port is not the secure port for the source address and do not update the address (block must be zero) These could mean a very different config based on their description. Feel free to add. >> >>> >>> Signed-off-by: Nikolay Aleksandrov >>> --- >>> I'll send the selftest for sticky with the iproute2 patch if this one is >>> accepted. We've had multiple requests to support such flag and now it's >>> also needed for some eVPN and clag setups. >>> >>> include/uapi/linux/neighbour.h | 1 + >>> net/bridge/br_fdb.c | 19 ++++++++++++++++--- >>> net/bridge/br_private.h | 1 + >>> 3 files changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h >>> index 904db6148476..998155444e0d 100644 >>> --- a/include/uapi/linux/neighbour.h >>> +++ b/include/uapi/linux/neighbour.h >>> @@ -43,6 +43,7 @@ enum { >>> #define NTF_PROXY 0x08 /* == ATF_PUBL */ >>> #define NTF_EXT_LEARNED 0x10 >>> #define NTF_OFFLOADED 0x20 >>> +#define NTF_STICKY 0x40 >>> #define NTF_ROUTER 0x80 >>> >>> /* >>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >>> index 502f66349530..26569ed06a4d 100644 >>> --- a/net/bridge/br_fdb.c >>> +++ b/net/bridge/br_fdb.c >>> @@ -584,7 +584,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, >>> unsigned long now = jiffies; >>> >>> /* fastpath: update of existing entry */ >>> - if (unlikely(source != fdb->dst)) { >>> + if (unlikely(source != fdb->dst && !fdb->is_sticky)) { >>> fdb->dst = source; >>> fdb_modified = true; >>> /* Take over HW learned entry */ >>> @@ -656,6 +656,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br, >>> ndm->ndm_flags |= NTF_OFFLOADED; >>> if (fdb->added_by_external_learn) >>> ndm->ndm_flags |= NTF_EXT_LEARNED; >>> + if (fdb->is_sticky) >>> + ndm->ndm_flags |= NTF_STICKY; >>> >>> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr)) >>> goto nla_put_failure; >>> @@ -772,7 +774,8 @@ int br_fdb_dump(struct sk_buff *skb, >>> >>> /* Update (create or replace) forwarding database entry */ >>> static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, >>> - const __u8 *addr, __u16 state, __u16 flags, __u16 vid) >>> + const u8 *addr, u16 state, u16 flags, u16 vid, >>> + u8 is_sticky) >> >> Why not change the API to take a full ndm flags, someone is sure to add more later. Sure, I've added a similar "fix" on my bridge todo list which is getting very long by the way, but it is not the point of this patch. 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=D8B+44QVXDSYWZS4z/0TPOu2xDpiPgmFdA4yBIQUN50=; b=bTJ319AtEkmILByp6crBG7i+6w5S5DvhozmFcDEjfu9VJwdGEog/3OozNgzxe5Qz+u TbnbqqRHyUXYqngGBQJf7XUnbsU5j3uxKEIF4ulrW1qKGQXZkWdZNvKpKZzA/WOxgHeg EwrxmOL30bXVXoqF6UvgWXgWGVYxLPGNcsdLU= References: <20180910101601.28073-1-nikolay@cumulusnetworks.com> <20180910141802.268e85c5@shemminger-XPS-13-9360> <886aea12-29f7-8722-42c9-5db2f3f34421@ti.com> From: Nikolay Aleksandrov Message-ID: <38bfcada-abea-be8d-c794-154113ca17f8@cumulusnetworks.com> Date: Tue, 11 Sep 2018 02:55:10 +0300 MIME-Version: 1.0 In-Reply-To: <886aea12-29f7-8722-42c9-5db2f3f34421@ti.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Bridge] [PATCH net-next] net: bridge: add support for sticky fdb entries List-Id: Linux Ethernet Bridging List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Grygorii Strashko , Stephen Hemminger Cc: netdev@vger.kernel.org, roopa@cumulusnetworks.com, bridge@lists.linux-foundation.org, davem@davemloft.net On 11/09/18 02:22, Grygorii Strashko wrote: > > > On 09/10/2018 04:18 PM, Stephen Hemminger wrote: >> On Mon, 10 Sep 2018 13:16:01 +0300 >> Nikolay Aleksandrov wrote: >> >>> Add support for entries which are "sticky", i.e. will not change their port >>> if they show up from a different one. A new ndm flag is introduced for that >>> purpose - NTF_STICKY. We allow to set it only to non-local entries. >> >> Is there a name for this in other network switch API's? > > In TI CPSW hardware we have following definition for similar FDB/ALE entries > (AM437x TRM - 15.3.2.7.1.4 Unicast Address Table Entry): > > Block (BLOCK) – The block bit indicates that a packet with a matching source or > destination address should be dropped (block the address). > 0 – Address is not blocked. > 1 – Drop a packet with a matching source or destination address (secure must be zero) > If block and secure are both set, then they no longer mean block and secure. > When both are set, the block and secure bits indicate that the packet is > a unicast supervisory (super) packet and they determine the unicast forward state test criteria. > If both bits are set then the packet is forwarded if the receive port is in > the Forwarding/Blocking/Learning state. > If both bits are not set then the packet is forwarded if the receive port is in > the Forwarding state. > > Secure (SECURE) - This bit indicates that a packet with a matching source address > should be dropped if the received port number is not equal to the table entry port_number. > 0 – Received port number is a don’t care. > 1 – Drop the packet if the received port is not the secure port for the source > address and do not update the address (block must be zero) > > Updating Process: > if ((source address found) and (receive port number != port_number) and (secure or block)) > then do not update address > > "sticky" would mean SECURE+BLOCK = supervisory (super) packet > That could work. but we haven't really made any arrangements w.r.t. to current implementations. So if IUC secure+block should be: - block: 1 – Drop a packet with a matching source or destination address (secure must be zero) If block and secure are both set, then they no longer mean block and secure. - secure: 1 – Drop the packet if the received port is not the secure port for the source address and do not update the address (block must be zero) These could mean a very different config based on their description. Feel free to add. >> >>> >>> Signed-off-by: Nikolay Aleksandrov >>> --- >>> I'll send the selftest for sticky with the iproute2 patch if this one is >>> accepted. We've had multiple requests to support such flag and now it's >>> also needed for some eVPN and clag setups. >>> >>> include/uapi/linux/neighbour.h | 1 + >>> net/bridge/br_fdb.c | 19 ++++++++++++++++--- >>> net/bridge/br_private.h | 1 + >>> 3 files changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h >>> index 904db6148476..998155444e0d 100644 >>> --- a/include/uapi/linux/neighbour.h >>> +++ b/include/uapi/linux/neighbour.h >>> @@ -43,6 +43,7 @@ enum { >>> #define NTF_PROXY 0x08 /* == ATF_PUBL */ >>> #define NTF_EXT_LEARNED 0x10 >>> #define NTF_OFFLOADED 0x20 >>> +#define NTF_STICKY 0x40 >>> #define NTF_ROUTER 0x80 >>> >>> /* >>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c >>> index 502f66349530..26569ed06a4d 100644 >>> --- a/net/bridge/br_fdb.c >>> +++ b/net/bridge/br_fdb.c >>> @@ -584,7 +584,7 @@ void br_fdb_update(struct net_bridge *br, struct net_bridge_port *source, >>> unsigned long now = jiffies; >>> >>> /* fastpath: update of existing entry */ >>> - if (unlikely(source != fdb->dst)) { >>> + if (unlikely(source != fdb->dst && !fdb->is_sticky)) { >>> fdb->dst = source; >>> fdb_modified = true; >>> /* Take over HW learned entry */ >>> @@ -656,6 +656,8 @@ static int fdb_fill_info(struct sk_buff *skb, const struct net_bridge *br, >>> ndm->ndm_flags |= NTF_OFFLOADED; >>> if (fdb->added_by_external_learn) >>> ndm->ndm_flags |= NTF_EXT_LEARNED; >>> + if (fdb->is_sticky) >>> + ndm->ndm_flags |= NTF_STICKY; >>> >>> if (nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->key.addr)) >>> goto nla_put_failure; >>> @@ -772,7 +774,8 @@ int br_fdb_dump(struct sk_buff *skb, >>> >>> /* Update (create or replace) forwarding database entry */ >>> static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source, >>> - const __u8 *addr, __u16 state, __u16 flags, __u16 vid) >>> + const u8 *addr, u16 state, u16 flags, u16 vid, >>> + u8 is_sticky) >> >> Why not change the API to take a full ndm flags, someone is sure to add more later. Sure, I've added a similar "fix" on my bridge todo list which is getting very long by the way, but it is not the point of this patch.