[PATCH net-next 0/4] net: bridge: simplify receive path and consolidate forwarding paths

[PATCH net-next 0/4] net: bridge: simplify receive path and consolidate forwarding paths

[Nikolay Aleksandrov]

Hi all,
This set tries to simplify the receive and forwarding paths. Patch 01 is
a trivial style adjustment, patch 02 removes one conditional from the
unicast fast path, patch 03 removes another conditional and more imporantly
removes the skb0/skb2 ambiguity about locally receiving the skb and
switches to a boolean called "local_rcv".
Patch 04 is the most important change which consolidates the forwarding
paths for locally originated and forwarded packets into __br_forward. This
allows us to remove the function pointers giving a minor performance boost,
more importantly it makes it much easier to reason about the forwarding
path and reduces the code duplication that was needed when making changes.
Also it allows the receive path to fully setup the environment prior to
calling any forwarding functions (i.e. to properly set unicast, local_rcv
and search for unicast/mcast dst).
Functionally everything should stay the same after this set.

I've done basic tests with unicast/multicast/broadcast Tx/Rx. Please
review carefully.

Thank you,
 Nik

Nikolay Aleksandrov (4):
  net: bridge: minor style adjustments in br_handle_frame_finish
  net: bridge: rearrange flood vs unicast receive paths
  net: bridge: drop skb2/skb0 variables and use a local_rcv boolean
  net: bridge: remove _deliver functions and consolidate forward code

 net/bridge/br_device.c                   |  22 ++--
 net/bridge/br_forward.c                  | 195 +++++++++++--------------------
 net/bridge/br_input.c                    |  62 +++++-----
 net/bridge/br_private.h                  |  29 ++---
 net/bridge/netfilter/nft_reject_bridge.c |   8 +-
 5 files changed, 125 insertions(+), 191 deletions(-)

-- 
2.4.3

[PATCH net-next 1/4] net: bridge: minor style adjustments in br_handle_frame_finish

[Nikolay Aleksandrov]

Trivial style changes in br_handle_frame_finish.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_input.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index a7817e6f306f..0b6d32619468 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -131,11 +131,11 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct net_bridge *br,
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
-	const unsigned char *dest = eth_hdr(skb)->h_dest;
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
-	struct net_bridge *br;
-	struct net_bridge_fdb_entry *dst;
+	const unsigned char *dest = eth_hdr(skb)->h_dest;
+	struct net_bridge_fdb_entry *dst = NULL;
 	struct net_bridge_mdb_entry *mdst;
+	struct net_bridge *br;
 	struct sk_buff *skb2;
 	bool unicast = true;
 	u16 vid = 0;
@@ -166,8 +166,6 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	if (br->dev->flags & IFF_PROMISC)
 		skb2 = skb;
 
-	dst = NULL;
-
 	if (IS_ENABLED(CONFIG_INET) && skb->protocol == htons(ETH_P_ARP))
 		br_do_proxy_arp(skb, br, vid, p);
 
@@ -185,13 +183,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 			skb = NULL;
 			if (!skb2)
 				goto out;
-		} else
+		} else {
 			skb2 = skb;
-
+		}
 		unicast = false;
 		br->dev->stats.multicast++;
-	} else if ((dst = __br_fdb_get(br, dest, vid)) &&
-			dst->is_local) {
+	} else if ((dst = __br_fdb_get(br, dest, vid)) && dst->is_local) {
 		skb2 = skb;
 		/* Do not forward the packet since it's local. */
 		skb = NULL;
@@ -201,8 +198,9 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		if (dst) {
 			dst->used = jiffies;
 			br_forward(dst->dst, skb, skb2);
-		} else
+		} else {
 			br_flood_forward(br, skb, skb2, unicast);
+		}
 	}
 
 	if (skb2)
-- 
2.4.3

[PATCH net-next 2/4] net: bridge: rearrange flood vs unicast receive paths

[Nikolay Aleksandrov]

This patch removes one conditional from the unicast path by using the fact
that skb is NULL only when the packet is multicast or is local.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_input.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 0b6d32619468..c20c5be6fc22 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -134,10 +134,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
 	const unsigned char *dest = eth_hdr(skb)->h_dest;
 	struct net_bridge_fdb_entry *dst = NULL;
+	bool mcast_hit = false, unicast = true;
 	struct net_bridge_mdb_entry *mdst;
 	struct net_bridge *br;
 	struct sk_buff *skb2;
-	bool unicast = true;
 	u16 vid = 0;
 
 	if (!p || p->state == BR_STATE_DISABLED)
@@ -177,30 +177,29 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
 		    br_multicast_querier_exists(br, eth_hdr(skb))) {
 			if ((mdst && mdst->mglist) ||
-			    br_multicast_is_router(br))
+			    br_multicast_is_router(br)) {
 				skb2 = skb;
-			br_multicast_forward(mdst, skb, skb2);
-			skb = NULL;
-			if (!skb2)
-				goto out;
+				br->dev->stats.multicast++;
+			}
+			mcast_hit = true;
 		} else {
 			skb2 = skb;
+			br->dev->stats.multicast++;
 		}
 		unicast = false;
-		br->dev->stats.multicast++;
 	} else if ((dst = __br_fdb_get(br, dest, vid)) && dst->is_local) {
-		skb2 = skb;
 		/* Do not forward the packet since it's local. */
-		skb = NULL;
+		return br_pass_frame_up(skb);
 	}
 
-	if (skb) {
-		if (dst) {
-			dst->used = jiffies;
-			br_forward(dst->dst, skb, skb2);
-		} else {
+	if (dst) {
+		dst->used = jiffies;
+		br_forward(dst->dst, skb, skb2);
+	} else {
+		if (!mcast_hit)
 			br_flood_forward(br, skb, skb2, unicast);
-		}
+		else
+			br_multicast_forward(mdst, skb, skb2);
 	}
 
 	if (skb2)
-- 
2.4.3

[PATCH net-next 3/4] net: bridge: drop skb2/skb0 variables and use a local_rcv boolean

[Nikolay Aleksandrov]

Currently if the packet is going to be received locally we set skb0 or
sometimes called skb2 variables to the original skb. This can get
confusing and also we can avoid one conditional on the fast path by
simply using a boolean and passing it around. Thanks to Roopa for the
name suggestion.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_forward.c | 35 ++++++++++++++++++-----------------
 net/bridge/br_input.c   | 25 ++++++++++---------------
 net/bridge/br_private.h | 10 +++++-----
 3 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index d610644368b9..204f99304a8a 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -138,17 +138,18 @@ void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
 EXPORT_SYMBOL_GPL(br_deliver);
 
 /* called with rcu_read_lock */
-void br_forward(const struct net_bridge_port *to, struct sk_buff *skb, struct sk_buff *skb0)
+void br_forward(const struct net_bridge_port *to, struct sk_buff *skb,
+		bool local_rcv)
 {
 	if (to && should_deliver(to, skb)) {
-		if (skb0)
+		if (local_rcv)
 			deliver_clone(to, skb, __br_forward);
 		else
 			__br_forward(to, skb);
 		return;
 	}
 
-	if (!skb0)
+	if (!local_rcv)
 		kfree_skb(skb);
 }
 
@@ -193,10 +194,9 @@ out:
 
 /* called under bridge lock */
 static void br_flood(struct net_bridge *br, struct sk_buff *skb,
-		     struct sk_buff *skb0,
 		     void (*__packet_hook)(const struct net_bridge_port *p,
 					   struct sk_buff *skb),
-		     bool unicast)
+		     bool local_rcv, bool unicast)
 {
 	u8 igmp_type = br_multicast_igmp_type(skb);
 	struct net_bridge_port *prev;
@@ -227,14 +227,14 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
 	if (!prev)
 		goto out;
 
-	if (skb0)
+	if (local_rcv)
 		deliver_clone(prev, skb, __packet_hook);
 	else
 		__packet_hook(prev, skb);
 	return;
 
 out:
-	if (!skb0)
+	if (!local_rcv)
 		kfree_skb(skb);
 }
 
@@ -242,23 +242,24 @@ out:
 /* called with rcu_read_lock */
 void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb, bool unicast)
 {
-	br_flood(br, skb, NULL, __br_deliver, unicast);
+	br_flood(br, skb, __br_deliver, false, unicast);
 }
 
 /* called under bridge lock */
 void br_flood_forward(struct net_bridge *br, struct sk_buff *skb,
-		      struct sk_buff *skb2, bool unicast)
+		      bool local_rcv, bool unicast)
 {
-	br_flood(br, skb, skb2, __br_forward, unicast);
+	br_flood(br, skb, __br_forward, local_rcv, unicast);
 }
 
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 /* called with rcu_read_lock */
 static void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
-			       struct sk_buff *skb, struct sk_buff *skb0,
+			       struct sk_buff *skb,
 			       void (*__packet_hook)(
 					const struct net_bridge_port *p,
-					struct sk_buff *skb))
+					struct sk_buff *skb),
+			       bool local_rcv)
 {
 	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
 	u8 igmp_type = br_multicast_igmp_type(skb);
@@ -295,14 +296,14 @@ static void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 	if (!prev)
 		goto out;
 
-	if (skb0)
+	if (local_rcv)
 		deliver_clone(prev, skb, __packet_hook);
 	else
 		__packet_hook(prev, skb);
 	return;
 
 out:
-	if (!skb0)
+	if (!local_rcv)
 		kfree_skb(skb);
 }
 
@@ -310,13 +311,13 @@ out:
 void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
 			  struct sk_buff *skb)
 {
-	br_multicast_flood(mdst, skb, NULL, __br_deliver);
+	br_multicast_flood(mdst, skb, __br_deliver, false);
 }
 
 /* called with rcu_read_lock */
 void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb, struct sk_buff *skb2)
+			  struct sk_buff *skb, bool local_rcv)
 {
-	br_multicast_flood(mdst, skb, skb2, __br_forward);
+	br_multicast_flood(mdst, skb, __br_forward, local_rcv);
 }
 #endif
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index c20c5be6fc22..dd8885def11b 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -131,13 +131,12 @@ static void br_do_proxy_arp(struct sk_buff *skb, struct net_bridge *br,
 /* note: already called with rcu_read_lock */
 int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
+	bool local_rcv = false, mcast_hit = false, unicast = true;
 	struct net_bridge_port *p = br_port_get_rcu(skb->dev);
 	const unsigned char *dest = eth_hdr(skb)->h_dest;
 	struct net_bridge_fdb_entry *dst = NULL;
-	bool mcast_hit = false, unicast = true;
 	struct net_bridge_mdb_entry *mdst;
 	struct net_bridge *br;
-	struct sk_buff *skb2;
 	u16 vid = 0;
 
 	if (!p || p->state == BR_STATE_DISABLED)
@@ -160,17 +159,13 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 	BR_INPUT_SKB_CB(skb)->brdev = br->dev;
 
-	/* The packet skb2 goes to the local host (NULL to skip). */
-	skb2 = NULL;
-
-	if (br->dev->flags & IFF_PROMISC)
-		skb2 = skb;
+	local_rcv = !!(br->dev->flags & IFF_PROMISC);
 
 	if (IS_ENABLED(CONFIG_INET) && skb->protocol == htons(ETH_P_ARP))
 		br_do_proxy_arp(skb, br, vid, p);
 
 	if (is_broadcast_ether_addr(dest)) {
-		skb2 = skb;
+		local_rcv = true;
 		unicast = false;
 	} else if (is_multicast_ether_addr(dest)) {
 		mdst = br_mdb_get(br, skb, vid);
@@ -178,12 +173,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 		    br_multicast_querier_exists(br, eth_hdr(skb))) {
 			if ((mdst && mdst->mglist) ||
 			    br_multicast_is_router(br)) {
-				skb2 = skb;
+				local_rcv = true;
 				br->dev->stats.multicast++;
 			}
 			mcast_hit = true;
 		} else {
-			skb2 = skb;
+			local_rcv = true;
 			br->dev->stats.multicast++;
 		}
 		unicast = false;
@@ -194,16 +189,16 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 	if (dst) {
 		dst->used = jiffies;
-		br_forward(dst->dst, skb, skb2);
+		br_forward(dst->dst, skb, local_rcv);
 	} else {
 		if (!mcast_hit)
-			br_flood_forward(br, skb, skb2, unicast);
+			br_flood_forward(br, skb, local_rcv, unicast);
 		else
-			br_multicast_forward(mdst, skb, skb2);
+			br_multicast_forward(mdst, skb, local_rcv);
 	}
 
-	if (skb2)
-		return br_pass_frame_up(skb2);
+	if (local_rcv)
+		return br_pass_frame_up(skb);
 
 out:
 	return 0;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 40f200947ddc..4d6cdf459e57 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -507,12 +507,12 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 /* br_forward.c */
 void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
 int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb);
-void br_forward(const struct net_bridge_port *to,
-		struct sk_buff *skb, struct sk_buff *skb0);
+void br_forward(const struct net_bridge_port *to, struct sk_buff *skb,
+		bool local_rcv);
 int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
 void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb, bool unicast);
 void br_flood_forward(struct net_bridge *br, struct sk_buff *skb,
-		      struct sk_buff *skb2, bool unicast);
+		      bool local_rcv, bool unicast);
 
 /* br_if.c */
 void br_port_carrier_check(struct net_bridge_port *p);
@@ -563,7 +563,7 @@ void br_multicast_dev_del(struct net_bridge *br);
 void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
 			  struct sk_buff *skb);
 void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb, struct sk_buff *skb2);
+			  struct sk_buff *skb, bool local_rcv);
 int br_multicast_set_router(struct net_bridge *br, unsigned long val);
 int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val);
 int br_multicast_toggle(struct net_bridge *br, unsigned long val);
@@ -698,7 +698,7 @@ static inline void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
 
 static inline void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
 					struct sk_buff *skb,
-					struct sk_buff *skb2)
+					bool local_rcv)
 {
 }
 static inline bool br_multicast_is_router(struct net_bridge *br)
-- 
2.4.3

[PATCH net-next 4/4] net: bridge: remove _deliver functions and consolidate forward code

[Nikolay Aleksandrov]

Before this patch we had two flavors of most forwarding functions -
_forward and _deliver, the difference being that the latter are used
when the packets are locally originated. Instead of all this function
pointer passing and code duplication, we can just pass a boolean noting
that the packet was locally originated and use that to perform the
necessary checks in __br_forward. This gives a minor performance
improvement but more importantly consolidates the forwarding paths.
Also add a kernel doc comment to explain the exported br_forward()'s
arguments.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_device.c                   |  22 ++--
 net/bridge/br_forward.c                  | 184 +++++++++++--------------------
 net/bridge/br_input.c                    |   6 +-
 net/bridge/br_private.h                  |  27 ++---
 net/bridge/netfilter/nft_reject_bridge.c |   8 +-
 5 files changed, 94 insertions(+), 153 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 8eecd0ec22f2..09f26940aba5 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -61,11 +61,11 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid))
 		goto out;
 
-	if (is_broadcast_ether_addr(dest))
-		br_flood_deliver(br, skb, false);
-	else if (is_multicast_ether_addr(dest)) {
+	if (is_broadcast_ether_addr(dest)) {
+		br_flood(br, skb, false, false, true);
+	} else if (is_multicast_ether_addr(dest)) {
 		if (unlikely(netpoll_tx_running(dev))) {
-			br_flood_deliver(br, skb, false);
+			br_flood(br, skb, false, false, true);
 			goto out;
 		}
 		if (br_multicast_rcv(br, NULL, skb, vid)) {
@@ -76,14 +76,14 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
 		mdst = br_mdb_get(br, skb, vid);
 		if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
 		    br_multicast_querier_exists(br, eth_hdr(skb)))
-			br_multicast_deliver(mdst, skb);
+			br_multicast_flood(mdst, skb, false, true);
 		else
-			br_flood_deliver(br, skb, false);
-	} else if ((dst = __br_fdb_get(br, dest, vid)) != NULL)
-		br_deliver(dst->dst, skb);
-	else
-		br_flood_deliver(br, skb, true);
-
+			br_flood(br, skb, false, false, true);
+	} else if ((dst = __br_fdb_get(br, dest, vid)) != NULL) {
+		br_forward(dst->dst, skb, false, true);
+	} else {
+		br_flood(br, skb, true, false, true);
+	}
 out:
 	rcu_read_unlock();
 	return NETDEV_TX_OK;
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 204f99304a8a..63a83d8d7da3 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -21,11 +21,6 @@
 #include <linux/netfilter_bridge.h>
 #include "br_private.h"
 
-static int deliver_clone(const struct net_bridge_port *prev,
-			 struct sk_buff *skb,
-			 void (*__packet_hook)(const struct net_bridge_port *p,
-					       struct sk_buff *skb));
-
 /* Don't forward packets to originating port or forwarding disabled */
 static inline int should_deliver(const struct net_bridge_port *p,
 				 const struct sk_buff *skb)
@@ -75,106 +70,92 @@ int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(br_forward_finish);
 
-static void __br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
+static void __br_forward(const struct net_bridge_port *to,
+			 struct sk_buff *skb, bool local_orig)
 {
 	struct net_bridge_vlan_group *vg;
+	struct net_device *indev;
+	struct net *net;
+	int br_hook;
 
 	vg = nbp_vlan_group_rcu(to);
 	skb = br_handle_vlan(to->br, vg, skb);
 	if (!skb)
 		return;
 
+	indev = skb->dev;
 	skb->dev = to->dev;
-
-	if (unlikely(netpoll_tx_running(to->br->dev))) {
-		if (!is_skb_forwardable(skb->dev, skb))
+	if (!local_orig) {
+		if (skb_warn_if_lro(skb)) {
 			kfree_skb(skb);
-		else {
-			skb_push(skb, ETH_HLEN);
-			br_netpoll_send_skb(to, skb);
+			return;
 		}
-		return;
+		br_hook = NF_BR_FORWARD;
+		skb_forward_csum(skb);
+		net = dev_net(indev);
+	} else {
+		if (unlikely(netpoll_tx_running(to->br->dev))) {
+			if (!is_skb_forwardable(skb->dev, skb)) {
+				kfree_skb(skb);
+			} else {
+				skb_push(skb, ETH_HLEN);
+				br_netpoll_send_skb(to, skb);
+			}
+			return;
+		}
+		br_hook = NF_BR_LOCAL_OUT;
+		net = dev_net(skb->dev);
+		indev = NULL;
 	}
 
-	NF_HOOK(NFPROTO_BRIDGE, NF_BR_LOCAL_OUT,
-		dev_net(skb->dev), NULL, skb,NULL, skb->dev,
+	NF_HOOK(NFPROTO_BRIDGE, br_hook,
+		net, NULL, skb, indev, skb->dev,
 		br_forward_finish);
 }
 
-static void __br_forward(const struct net_bridge_port *to, struct sk_buff *skb)
+static int deliver_clone(const struct net_bridge_port *prev,
+			 struct sk_buff *skb, bool local_orig)
 {
-	struct net_bridge_vlan_group *vg;
-	struct net_device *indev;
-
-	if (skb_warn_if_lro(skb)) {
-		kfree_skb(skb);
-		return;
-	}
-
-	vg = nbp_vlan_group_rcu(to);
-	skb = br_handle_vlan(to->br, vg, skb);
-	if (!skb)
-		return;
-
-	indev = skb->dev;
-	skb->dev = to->dev;
-	skb_forward_csum(skb);
-
-	NF_HOOK(NFPROTO_BRIDGE, NF_BR_FORWARD,
-		dev_net(indev), NULL, skb, indev, skb->dev,
-		br_forward_finish);
-}
+	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
 
-/* called with rcu_read_lock */
-void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb)
-{
-	if (to && should_deliver(to, skb)) {
-		__br_deliver(to, skb);
-		return;
+	skb = skb_clone(skb, GFP_ATOMIC);
+	if (!skb) {
+		dev->stats.tx_dropped++;
+		return -ENOMEM;
 	}
 
-	kfree_skb(skb);
+	__br_forward(prev, skb, local_orig);
+	return 0;
 }
-EXPORT_SYMBOL_GPL(br_deliver);
 
-/* called with rcu_read_lock */
-void br_forward(const struct net_bridge_port *to, struct sk_buff *skb,
-		bool local_rcv)
+/**
+ * br_forward - forward a packet to a specific port
+ * @to: destination port
+ * @skb: packet being forwarded
+ * @local_rcv: packet will be received locally after forwarding
+ * @local_orig: packet is locally originated
+ *
+ * Should be called with rcu_read_lock.
+ */
+void br_forward(const struct net_bridge_port *to,
+		struct sk_buff *skb, bool local_rcv, bool local_orig)
 {
 	if (to && should_deliver(to, skb)) {
 		if (local_rcv)
-			deliver_clone(to, skb, __br_forward);
+			deliver_clone(to, skb, local_orig);
 		else
-			__br_forward(to, skb);
+			__br_forward(to, skb, local_orig);
 		return;
 	}
 
 	if (!local_rcv)
 		kfree_skb(skb);
 }
-
-static int deliver_clone(const struct net_bridge_port *prev,
-			 struct sk_buff *skb,
-			 void (*__packet_hook)(const struct net_bridge_port *p,
-					       struct sk_buff *skb))
-{
-	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
-
-	skb = skb_clone(skb, GFP_ATOMIC);
-	if (!skb) {
-		dev->stats.tx_dropped++;
-		return -ENOMEM;
-	}
-
-	__packet_hook(prev, skb);
-	return 0;
-}
+EXPORT_SYMBOL_GPL(br_forward);
 
 static struct net_bridge_port *maybe_deliver(
 	struct net_bridge_port *prev, struct net_bridge_port *p,
-	struct sk_buff *skb,
-	void (*__packet_hook)(const struct net_bridge_port *p,
-			      struct sk_buff *skb))
+	struct sk_buff *skb, bool local_orig)
 {
 	int err;
 
@@ -184,7 +165,7 @@ static struct net_bridge_port *maybe_deliver(
 	if (!prev)
 		goto out;
 
-	err = deliver_clone(prev, skb, __packet_hook);
+	err = deliver_clone(prev, skb, local_orig);
 	if (err)
 		return ERR_PTR(err);
 
@@ -192,18 +173,14 @@ out:
 	return p;
 }
 
-/* called under bridge lock */
-static void br_flood(struct net_bridge *br, struct sk_buff *skb,
-		     void (*__packet_hook)(const struct net_bridge_port *p,
-					   struct sk_buff *skb),
-		     bool local_rcv, bool unicast)
+/* called under rcu_read_lock */
+void br_flood(struct net_bridge *br, struct sk_buff *skb,
+	      bool unicast, bool local_rcv, bool local_orig)
 {
 	u8 igmp_type = br_multicast_igmp_type(skb);
-	struct net_bridge_port *prev;
+	struct net_bridge_port *prev = NULL;
 	struct net_bridge_port *p;
 
-	prev = NULL;
-
 	list_for_each_entry_rcu(p, &br->port_list, list) {
 		/* Do not flood unicast traffic to ports that turn it off */
 		if (unicast && !(p->flags & BR_FLOOD))
@@ -216,7 +193,7 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
 		    BR_INPUT_SKB_CB(skb)->proxyarp_replied)
 			continue;
 
-		prev = maybe_deliver(prev, p, skb, __packet_hook);
+		prev = maybe_deliver(prev, p, skb, local_orig);
 		if (IS_ERR(prev))
 			goto out;
 		if (prev == p)
@@ -228,9 +205,9 @@ static void br_flood(struct net_bridge *br, struct sk_buff *skb,
 		goto out;
 
 	if (local_rcv)
-		deliver_clone(prev, skb, __packet_hook);
+		deliver_clone(prev, skb, local_orig);
 	else
-		__packet_hook(prev, skb);
+		__br_forward(prev, skb, local_orig);
 	return;
 
 out:
@@ -238,28 +215,11 @@ out:
 		kfree_skb(skb);
 }
 
-
-/* called with rcu_read_lock */
-void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb, bool unicast)
-{
-	br_flood(br, skb, __br_deliver, false, unicast);
-}
-
-/* called under bridge lock */
-void br_flood_forward(struct net_bridge *br, struct sk_buff *skb,
-		      bool local_rcv, bool unicast)
-{
-	br_flood(br, skb, __br_forward, local_rcv, unicast);
-}
-
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
 /* called with rcu_read_lock */
-static void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
-			       struct sk_buff *skb,
-			       void (*__packet_hook)(
-					const struct net_bridge_port *p,
-					struct sk_buff *skb),
-			       bool local_rcv)
+void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
+			struct sk_buff *skb,
+			bool local_rcv, bool local_orig)
 {
 	struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev;
 	u8 igmp_type = br_multicast_igmp_type(skb);
@@ -280,7 +240,7 @@ static void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 		port = (unsigned long)lport > (unsigned long)rport ?
 		       lport : rport;
 
-		prev = maybe_deliver(prev, port, skb, __packet_hook);
+		prev = maybe_deliver(prev, port, skb, local_orig);
 		if (IS_ERR(prev))
 			goto out;
 		if (prev == port)
@@ -297,27 +257,13 @@ static void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
 		goto out;
 
 	if (local_rcv)
-		deliver_clone(prev, skb, __packet_hook);
+		deliver_clone(prev, skb, local_orig);
 	else
-		__packet_hook(prev, skb);
+		__br_forward(prev, skb, local_orig);
 	return;
 
 out:
 	if (!local_rcv)
 		kfree_skb(skb);
 }
-
-/* called with rcu_read_lock */
-void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb)
-{
-	br_multicast_flood(mdst, skb, __br_deliver, false);
-}
-
-/* called with rcu_read_lock */
-void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb, bool local_rcv)
-{
-	br_multicast_flood(mdst, skb, __br_forward, local_rcv);
-}
 #endif
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index dd8885def11b..8b08eec763a5 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -189,12 +189,12 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
 
 	if (dst) {
 		dst->used = jiffies;
-		br_forward(dst->dst, skb, local_rcv);
+		br_forward(dst->dst, skb, local_rcv, false);
 	} else {
 		if (!mcast_hit)
-			br_flood_forward(br, skb, local_rcv, unicast);
+			br_flood(br, skb, unicast, local_rcv, false);
 		else
-			br_multicast_forward(mdst, skb, local_rcv);
+			br_multicast_flood(mdst, skb, local_rcv, false);
 	}
 
 	if (local_rcv)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 4d6cdf459e57..b3088264f844 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -505,14 +505,12 @@ int br_fdb_external_learn_del(struct net_bridge *br, struct net_bridge_port *p,
 			      const unsigned char *addr, u16 vid);
 
 /* br_forward.c */
-void br_deliver(const struct net_bridge_port *to, struct sk_buff *skb);
 int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb);
 void br_forward(const struct net_bridge_port *to, struct sk_buff *skb,
-		bool local_rcv);
+		bool local_rcv, bool local_orig);
 int br_forward_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
-void br_flood_deliver(struct net_bridge *br, struct sk_buff *skb, bool unicast);
-void br_flood_forward(struct net_bridge *br, struct sk_buff *skb,
-		      bool local_rcv, bool unicast);
+void br_flood(struct net_bridge *br, struct sk_buff *skb,
+	      bool unicast, bool local_rcv, bool local_orig);
 
 /* br_if.c */
 void br_port_carrier_check(struct net_bridge_port *p);
@@ -560,10 +558,8 @@ void br_multicast_init(struct net_bridge *br);
 void br_multicast_open(struct net_bridge *br);
 void br_multicast_stop(struct net_bridge *br);
 void br_multicast_dev_del(struct net_bridge *br);
-void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb);
-void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
-			  struct sk_buff *skb, bool local_rcv);
+void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
+			struct sk_buff *skb, bool local_rcv, bool local_orig);
 int br_multicast_set_router(struct net_bridge *br, unsigned long val);
 int br_multicast_set_port_router(struct net_bridge_port *p, unsigned long val);
 int br_multicast_toggle(struct net_bridge *br, unsigned long val);
@@ -691,28 +687,27 @@ static inline void br_multicast_dev_del(struct net_bridge *br)
 {
 }
 
-static inline void br_multicast_deliver(struct net_bridge_mdb_entry *mdst,
-					struct sk_buff *skb)
+static inline void br_multicast_flood(struct net_bridge_mdb_entry *mdst,
+				      struct sk_buff *skb,
+				      bool local_rcv, bool local_orig)
 {
 }
 
-static inline void br_multicast_forward(struct net_bridge_mdb_entry *mdst,
-					struct sk_buff *skb,
-					bool local_rcv)
-{
-}
 static inline bool br_multicast_is_router(struct net_bridge *br)
 {
 	return 0;
 }
+
 static inline bool br_multicast_querier_exists(struct net_bridge *br,
 					       struct ethhdr *eth)
 {
 	return false;
 }
+
 static inline void br_mdb_init(void)
 {
 }
+
 static inline void br_mdb_uninit(void)
 {
 }
diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
index 77f7e7a9ebe1..0b77ffbc27d6 100644
--- a/net/bridge/netfilter/nft_reject_bridge.c
+++ b/net/bridge/netfilter/nft_reject_bridge.c
@@ -72,7 +72,7 @@ static void nft_reject_br_send_v4_tcp_reset(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(dev), nskb);
+	br_forward(br_port_get_rcu(dev), nskb, false, true);
 }
 
 static void nft_reject_br_send_v4_unreach(struct net *net,
@@ -140,7 +140,7 @@ static void nft_reject_br_send_v4_unreach(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(dev), nskb);
+	br_forward(br_port_get_rcu(dev), nskb, false, true);
 }
 
 static void nft_reject_br_send_v6_tcp_reset(struct net *net,
@@ -174,7 +174,7 @@ static void nft_reject_br_send_v6_tcp_reset(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(dev), nskb);
+	br_forward(br_port_get_rcu(dev), nskb, false, true);
 }
 
 static bool reject6_br_csum_ok(struct sk_buff *skb, int hook)
@@ -255,7 +255,7 @@ static void nft_reject_br_send_v6_unreach(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(dev), nskb);
+	br_forward(br_port_get_rcu(dev), nskb, false, true);
 }
 
 static void nft_reject_bridge_eval(const struct nft_expr *expr,
-- 
2.4.3

Re: [PATCH net-next 2/4] net: bridge: rearrange flood vs unicast receive paths

[Cong Wang]

On Wed, Jul 13, 2016 at 8:10 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> This patch removes one conditional from the unicast path by using the fact
> that skb is NULL only when the packet is multicast or is local.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  net/bridge/br_input.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
> index 0b6d32619468..c20c5be6fc22 100644
> --- a/net/bridge/br_input.c
> +++ b/net/bridge/br_input.c
> @@ -134,10 +134,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>         struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>         const unsigned char *dest = eth_hdr(skb)->h_dest;
>         struct net_bridge_fdb_entry *dst = NULL;
> +       bool mcast_hit = false, unicast = true;
>         struct net_bridge_mdb_entry *mdst;
>         struct net_bridge *br;
>         struct sk_buff *skb2;
> -       bool unicast = true;
>         u16 vid = 0;
>
>         if (!p || p->state == BR_STATE_DISABLED)
> @@ -177,30 +177,29 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>                 if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>                     br_multicast_querier_exists(br, eth_hdr(skb))) {
>                         if ((mdst && mdst->mglist) ||
> -                           br_multicast_is_router(br))
> +                           br_multicast_is_router(br)) {
>                                 skb2 = skb;
> -                       br_multicast_forward(mdst, skb, skb2);
> -                       skb = NULL;
> -                       if (!skb2)
> -                               goto out;
> +                               br->dev->stats.multicast++;
> +                       }
> +                       mcast_hit = true;
>                 } else {
>                         skb2 = skb;
> +                       br->dev->stats.multicast++;
>                 }
>                 unicast = false;
> -               br->dev->stats.multicast++;


Looks like you change the unconditional increment of this counter,
is this intended?

Re: [PATCH net-next 2/4] net: bridge: rearrange flood vs unicast receive paths

[Nikolay Aleksandrov]

> On Jul 15, 2016, at 10:35 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> 
> On Wed, Jul 13, 2016 at 8:10 PM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> This patch removes one conditional from the unicast path by using the fact
>> that skb is NULL only when the packet is multicast or is local.
>> 
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>> net/bridge/br_input.c | 29 ++++++++++++++---------------
>> 1 file changed, 14 insertions(+), 15 deletions(-)
>> 
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 0b6d32619468..c20c5be6fc22 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -134,10 +134,10 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>        struct net_bridge_port *p = br_port_get_rcu(skb->dev);
>>        const unsigned char *dest = eth_hdr(skb)->h_dest;
>>        struct net_bridge_fdb_entry *dst = NULL;
>> +       bool mcast_hit = false, unicast = true;
>>        struct net_bridge_mdb_entry *mdst;
>>        struct net_bridge *br;
>>        struct sk_buff *skb2;
>> -       bool unicast = true;
>>        u16 vid = 0;
>> 
>>        if (!p || p->state == BR_STATE_DISABLED)
>> @@ -177,30 +177,29 @@ int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb
>>                if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
>>                    br_multicast_querier_exists(br, eth_hdr(skb))) {
>>                        if ((mdst && mdst->mglist) ||
>> -                           br_multicast_is_router(br))
>> +                           br_multicast_is_router(br)) {
>>                                skb2 = skb;
>> -                       br_multicast_forward(mdst, skb, skb2);
>> -                       skb = NULL;
>> -                       if (!skb2)
>> -                               goto out;
>> +                               br->dev->stats.multicast++;
>> +                       }
>> +                       mcast_hit = true;
>>                } else {
>>                        skb2 = skb;
>> +                       br->dev->stats.multicast++;
>>                }
>>                unicast = false;
>> -               br->dev->stats.multicast++;
> 
> 
> Looks like you change the unconditional increment of this counter,
> is this intended?

Oops +CC all, mixed the reply list.

Yes, this counter must increment only when the packet is going to be locally received as you can see before
there was an unconditional jump if the packet was not going to be locally received (if !skb2, goto).
This is exactly one of the confusing cases trying to avoid with these skb0/skb2 variables..

Cheers,
Nik

Re: [PATCH net-next 0/4] net: bridge: simplify receive path and consolidate forwarding paths

[David Miller]

From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Thu, 14 Jul 2016 06:09:58 +0300

> This set tries to simplify the receive and forwarding paths. Patch 01 is
> a trivial style adjustment, patch 02 removes one conditional from the
> unicast fast path, patch 03 removes another conditional and more imporantly
> removes the skb0/skb2 ambiguity about locally receiving the skb and
> switches to a boolean called "local_rcv".
> Patch 04 is the most important change which consolidates the forwarding
> paths for locally originated and forwarded packets into __br_forward. This
> allows us to remove the function pointers giving a minor performance boost,
> more importantly it makes it much easier to reason about the forwarding
> path and reduces the code duplication that was needed when making changes.
> Also it allows the receive path to fully setup the environment prior to
> calling any forwarding functions (i.e. to properly set unicast, local_rcv
> and search for unicast/mcast dst).
> Functionally everything should stay the same after this set.
> 
> I've done basic tests with unicast/multicast/broadcast Tx/Rx. Please
> review carefully.

I've reviewed this twice and can't find any problems, so applied to
net-next, thanks.