Message ID | 20190404115638.28592-1-pablo@netfilter.org |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: bridge: update multicast stats from maybe_deliver() | expand |
On 04/04/2019 14:56, Pablo Neira Ayuso wrote: > Simplify this code by updating bridge multicast stats from > maybe_deliver(). > > Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case > the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous > port pointer, therefore it is always going to be different from the > existing port in this deduplicated list iteration. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > net/bridge/br_forward.c | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > This was intentional, the reason I didn't add that call to maybe_deliver() is to avoid these checks for the standard unicast fast-path. We need to avoid touching the mcast cache lines (and checks) when using unicast.
On 04/04/2019 15:02, Nikolay Aleksandrov wrote: > On 04/04/2019 14:56, Pablo Neira Ayuso wrote: >> Simplify this code by updating bridge multicast stats from >> maybe_deliver(). >> >> Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case >> the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous >> port pointer, therefore it is always going to be different from the >> existing port in this deduplicated list iteration. >> >> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> >> --- >> net/bridge/br_forward.c | 15 ++++----------- >> 1 file changed, 4 insertions(+), 11 deletions(-) >> > > This was intentional, the reason I didn't add that call to maybe_deliver() is to avoid > these checks for the standard unicast fast-path. We need to avoid touching the mcast > cache lines (and checks) when using unicast. > To follow-up after my bridge boolean conversion to a flags field in the first cache line the hit is smaller but it still adds at least 2 new tests in the unicast fast-path which can easily be avoided.
On Thu, Apr 04, 2019 at 03:02:19PM +0300, Nikolay Aleksandrov wrote: > On 04/04/2019 14:56, Pablo Neira Ayuso wrote: > > Simplify this code by updating bridge multicast stats from > > maybe_deliver(). > > > > Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case > > the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous > > port pointer, therefore it is always going to be different from the > > existing port in this deduplicated list iteration. > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > net/bridge/br_forward.c | 15 ++++----------- > > 1 file changed, 4 insertions(+), 11 deletions(-) This is removing two branches, it is less code to maintain and easier to read than: if (prev == p) > This was intentional, the reason I didn't add that call to maybe_deliver() is to avoid > these checks for the standard unicast fast-path. We need to avoid touching the mcast > cache lines (and checks) when using unicast. maybe_deliver() is called from br_flood() and br_multicast_flood(), not the standard unicast fast-path. Thanks.
On 04/04/2019 15:21, Pablo Neira Ayuso wrote: > On Thu, Apr 04, 2019 at 03:02:19PM +0300, Nikolay Aleksandrov wrote: >> On 04/04/2019 14:56, Pablo Neira Ayuso wrote: >>> Simplify this code by updating bridge multicast stats from >>> maybe_deliver(). >>> >>> Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case >>> the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous >>> port pointer, therefore it is always going to be different from the >>> existing port in this deduplicated list iteration. >>> >>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> >>> --- >>> net/bridge/br_forward.c | 15 ++++----------- >>> 1 file changed, 4 insertions(+), 11 deletions(-) > > This is removing two branches, it is less code to maintain and easier > to read than: > > if (prev == p) > ack >> This was intentional, the reason I didn't add that call to maybe_deliver() is to avoid >> these checks for the standard unicast fast-path. We need to avoid touching the mcast >> cache lines (and checks) when using unicast. > > maybe_deliver() is called from br_flood() and br_multicast_flood(), > not the standard unicast fast-path. > Oh right, I was thinking of should_deliver(), my bad. > Thanks. > Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: Thu, 4 Apr 2019 13:56:38 +0200 > Simplify this code by updating bridge multicast stats from > maybe_deliver(). > > Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case > the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous > port pointer, therefore it is always going to be different from the > existing port in this deduplicated list iteration. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Applied.
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c index 48ddc60b4fbd..82225b8b54f5 100644 --- a/net/bridge/br_forward.c +++ b/net/bridge/br_forward.c @@ -173,6 +173,7 @@ static struct net_bridge_port *maybe_deliver( struct net_bridge_port *prev, struct net_bridge_port *p, struct sk_buff *skb, bool local_orig) { + u8 igmp_type = br_multicast_igmp_type(skb); int err; if (!should_deliver(p, skb)) @@ -184,8 +185,9 @@ static struct net_bridge_port *maybe_deliver( err = deliver_clone(prev, skb, local_orig); if (err) return ERR_PTR(err); - out: + br_multicast_count(p->br, p, skb, igmp_type, BR_MCAST_DIR_TX); + return p; } @@ -193,7 +195,6 @@ static struct net_bridge_port *maybe_deliver( void br_flood(struct net_bridge *br, struct sk_buff *skb, enum br_pkt_type pkt_type, bool local_rcv, bool local_orig) { - u8 igmp_type = br_multicast_igmp_type(skb); struct net_bridge_port *prev = NULL; struct net_bridge_port *p; @@ -226,9 +227,6 @@ void br_flood(struct net_bridge *br, struct sk_buff *skb, prev = maybe_deliver(prev, p, skb, local_orig); if (IS_ERR(prev)) goto out; - if (prev == p) - br_multicast_count(p->br, p, skb, igmp_type, - BR_MCAST_DIR_TX); } if (!prev) @@ -277,7 +275,6 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, bool local_rcv, bool local_orig) { struct net_device *dev = BR_INPUT_SKB_CB(skb)->brdev; - u8 igmp_type = br_multicast_igmp_type(skb); struct net_bridge *br = netdev_priv(dev); struct net_bridge_port *prev = NULL; struct net_bridge_port_group *p; @@ -304,13 +301,9 @@ void br_multicast_flood(struct net_bridge_mdb_entry *mdst, } prev = maybe_deliver(prev, port, skb, local_orig); -delivered: if (IS_ERR(prev)) goto out; - if (prev == port) - br_multicast_count(port->br, port, skb, igmp_type, - BR_MCAST_DIR_TX); - +delivered: if ((unsigned long)lport >= (unsigned long)port) p = rcu_dereference(p->next); if ((unsigned long)rport >= (unsigned long)port)
Simplify this code by updating bridge multicast stats from maybe_deliver(). Note that commit 6db6f0eae605 ("bridge: multicast to unicast"), in case the port flag BR_MULTICAST_TO_UNICAST is set, never updates the previous port pointer, therefore it is always going to be different from the existing port in this deduplicated list iteration. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/bridge/br_forward.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)