diff mbox

[2/2] macvlan: Avoid unnecessary multicast cloning

Message ID 20160530082828.GB5106@gondor.apana.org.au
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Herbert Xu May 30, 2016, 8:28 a.m. UTC 
Currently we always queue a multicast packet for further processing,
even if none of the macvlan devices are subscribed to the address.

This patch optimises this by adding a global multicast filter for
a macvlan_port.

Note that this patch doesn't handle the broadcast addresses of the
individual macvlan devices correctly, if they are not all identical.
However, this is already broken because there is no mechanism in
place to update the individual multicast filters when you change
the broadcast address.

If someone cares enough they should fix this by collecting all
broadcast addresses for a macvlan as we do for multicast and unicast.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

David Miller May 31, 2016, 9:07 p.m. UTC | #1 
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 30 May 2016 16:28:28 +0800

> @@ -725,6 +730,8 @@ static void macvlan_set_mac_lists(struct net_device *dev)
>  {
>  	struct macvlan_dev *vlan = netdev_priv(dev);
>  
> +	dev_uc_sync(vlan->lowerdev, dev);
> +	dev_mc_sync(vlan->lowerdev, dev);
>  	if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
>  		bitmap_fill(vlan->mc_filter, MACVLAN_MC_FILTER_SZ);
>  	} else {

I think you need to set the vlan->port->mc_filter to all 1's in the
PROMISC/ALLMUTI branch here.

Otherwise packets won't properly pass your new hash test.
Herbert Xu June 1, 2016, 3:42 a.m. UTC | #2 
On Tue, May 31, 2016 at 02:07:13PM -0700, David Miller wrote:
> 
> I think you need to set the vlan->port->mc_filter to all 1's in the
> PROMISC/ALLMUTI branch here.
> 
> Otherwise packets won't properly pass your new hash test.

Good point.  Here's v2.

This patch tries to improve macvlan multicast performance by
maintaining a filter hash at the macvlan_port level so that we
can quickly determine whether a given packet is needed or not.

It is preceded by a patch that fixes a potential use-after-free
bug that I discovered while looking over this.

v2 fixed a bug where promiscuous/allmulti settings weren't handled
correctly.

Thanks,
David Miller June 2, 2016, 12:49 a.m. UTC | #3 
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Wed, 1 Jun 2016 11:42:18 +0800

> This patch tries to improve macvlan multicast performance by
> maintaining a filter hash at the macvlan_port level so that we
> can quickly determine whether a given packet is needed or not.
> 
> It is preceded by a patch that fixes a potential use-after-free
> bug that I discovered while looking over this.
> 
> v2 fixed a bug where promiscuous/allmulti settings weren't handled
> correctly.

Series applied to net-next, thanks Herbert.
diff mbox

Patch

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index f55fe21..9fa4532 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -49,6 +49,7 @@  struct macvlan_port {
 	bool 			passthru;
 	int			count;
 	struct hlist_head	vlan_source_hash[MACVLAN_HASH_SIZE];
+	DECLARE_BITMAP(mc_filter, MACVLAN_MC_FILTER_SZ);
 };
 
 struct macvlan_source_entry {
@@ -418,6 +419,8 @@  static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 
 	port = macvlan_port_get_rcu(skb->dev);
 	if (is_multicast_ether_addr(eth->h_dest)) {
+		unsigned int hash;
+
 		skb = ip_check_defrag(dev_net(skb->dev), skb, IP_DEFRAG_MACVLAN);
 		if (!skb)
 			return RX_HANDLER_CONSUMED;
@@ -435,7 +438,9 @@  static rx_handler_result_t macvlan_handle_frame(struct sk_buff **pskb)
 			goto out;
 		}
 
-		macvlan_broadcast_enqueue(port, src, skb);
+		hash = mc_hash(NULL, eth->h_dest);
+		if (test_bit(hash, port->mc_filter))
+			macvlan_broadcast_enqueue(port, src, skb);
 
 		return RX_HANDLER_PASS;
 	}
@@ -725,6 +730,8 @@  static void macvlan_set_mac_lists(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
 
+	dev_uc_sync(vlan->lowerdev, dev);
+	dev_mc_sync(vlan->lowerdev, dev);
 	if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
 		bitmap_fill(vlan->mc_filter, MACVLAN_MC_FILTER_SZ);
 	} else {
@@ -739,9 +746,31 @@  static void macvlan_set_mac_lists(struct net_device *dev)
 		__set_bit(mc_hash(vlan, dev->broadcast), filter);
 
 		bitmap_copy(vlan->mc_filter, filter, MACVLAN_MC_FILTER_SZ);
+
+		/* This is slightly inaccurate as we're including
+		 * the subscription list of vlan->lowerdev too.
+		 */
+		bitmap_zero(filter, MACVLAN_MC_FILTER_SZ);
+		netdev_for_each_mc_addr(ha, vlan->lowerdev) {
+			__set_bit(mc_hash(NULL, ha->addr), filter);
+		}
+
+		/* Bug alert: This only works if everyone has the
+		 * same broadcast address.  As soon as someone
+		 * changes theirs this will break.
+		 *
+		 * However, this is already broken as when you
+		 * change your broadcast address we don't get
+		 * called.
+		 *
+		 * The solution is to maintain a list of broadcast
+		 * addresses like we do for uc/mc, if you care.
+		 */
+		__set_bit(mc_hash(NULL, dev->broadcast), filter);
+
+		bitmap_copy(vlan->port->mc_filter, filter,
+			    MACVLAN_MC_FILTER_SZ);
 	}
-	dev_uc_sync(vlan->lowerdev, dev);
-	dev_mc_sync(vlan->lowerdev, dev);
 }
 
 static int macvlan_change_mtu(struct net_device *dev, int new_mtu)