Re: [Bridge] [PATCH 3/4] macvlan: implement bridge, VEPA and private mode

[Patrick McHardy <kaber@trash.net>]

Arnd Bergmann wrote:
> This allows each macvlan slave device to be in one
> of three modes, depending on the use case:
> 
> MACVLAN_PRIVATE:
>   The device never communicates with any other device
>   on the same upper_dev. This even includes frames
>   coming back from a reflective relay, where supported
>   by the adjacent bridge.
> 
> MACVLAN_VEPA:
>   The new Virtual Ethernet Port Aggregator (VEPA) mode,
>   we assume that the adjacent bridge returns all frames
>   where both source and destination are local to the
>   macvlan port, i.e. the bridge is set up as a reflective
>   relay.
>   Broadcast frames coming in from the upper_dev get
>   flooded to all macvlan interfaces in VEPA mode.
>   We never deliver any frames locally.
> 
> MACVLAN_BRIDGE:
>   We provide the behavior of a simple bridge between
>   different macvlan interfaces on the same port. Frames
>   from one interface to another one get delivered directly
>   and are not sent out externally. Broadcast frames get
>   flooded to all other bridge ports and to the external
>   interface, but when they come back from a reflective
>   relay, we don't deliver them again.
>   Since we know all the MAC addresses, the macvlan bridge
>   mode does not require learning or STP like the bridge
>   module does.

This looks pretty nice. Some stylistic nitpicking below :)

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index a0dea23..b840b3a 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -29,9 +29,16 @@
>  #include <linux/if_link.h>
>  #include <linux/if_macvlan.h>
>  #include <net/rtnetlink.h>
> +#include <net/xfrm.h>

Do we really need this?

> @@ -129,11 +137,14 @@ static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
>  }
>  
>  static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> -				 const struct ethhdr *eth)
> +				 const struct ethhdr *eth, int local)

bool local?

>  {
>  	if (!skb)
>  		return NET_RX_DROP;
>  
> +	if (local)
> +		return dev_forward_skb(dev, skb);
> +
>  	skb->dev = dev;
>  	if (!compare_ether_addr_64bits(eth->h_dest,
>  				       dev->broadcast))
> @@ -145,7 +156,9 @@ static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
>  }
>  
>  static void macvlan_broadcast(struct sk_buff *skb,
> -			      const struct macvlan_port *port)
> +			      const struct macvlan_port *port,
> +			      struct net_device *src,
> +			      enum macvlan_mode mode)
>  {
>  	const struct ethhdr *eth = eth_hdr(skb);
>  	const struct macvlan_dev *vlan;
> @@ -159,8 +172,12 @@ static void macvlan_broadcast(struct sk_buff *skb,
>  
>  	for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
>  		hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
> +			if ((vlan->dev == src) || !(vlan->mode & mode))

Please remove those unnecessary parentheses around the
device comparison.

> @@ -173,6 +190,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
>  	const struct ethhdr *eth = eth_hdr(skb);
>  	const struct macvlan_port *port;
>  	const struct macvlan_dev *vlan;
> +	const struct macvlan_dev *src;
>  	struct net_device *dev;
>  
>  	port = rcu_dereference(skb->dev->macvlan_port);
> @@ -180,7 +198,20 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
>  		return skb;
>  
>  	if (is_multicast_ether_addr(eth->h_dest)) {
> -		macvlan_broadcast(skb, port);
> +		src = macvlan_hash_lookup(port, eth->h_source);
> +		if (!src)
> +			/* frame comes from an external address */
> +			macvlan_broadcast(skb, port, NULL, MACVLAN_MODE_PRIVATE
> +				| MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);

The '|' should go on the first line.

> +		else if (src->mode == MACVLAN_MODE_VEPA)
> +			/* flood to everyone except source */
> +			macvlan_broadcast(skb, port, src->dev,
> +				MACVLAN_MODE_VEPA | MACVLAN_MODE_BRIDGE);
> +		else if (src->mode == MACVLAN_MODE_BRIDGE)
> +			/* flood only to VEPA ports, bridge ports
> +			   already saw the frame */

Multi-line comments should begin every line with '*'.

> +			macvlan_broadcast(skb, port, src->dev,
> +				MACVLAN_MODE_VEPA);

Please align the mode values (in all cases above) to the arguments
on the line above.

>  		return skb;
>  	}
>  
> @@ -203,18 +234,46 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
>  	return NULL;
>  }
>  
> +static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	const struct macvlan_dev *vlan = netdev_priv(dev);
> +	const struct macvlan_port *port = vlan->port;
> +	const struct macvlan_dev *dest;
> +
> +	if (vlan->mode == MACVLAN_MODE_BRIDGE) {
> +		const struct ethhdr *eth = (void *)skb->data;

eth_hdr()?

> +
> +		/* send to other bridge ports directly */
> +		if (is_multicast_ether_addr(eth->h_dest)) {
> +			macvlan_broadcast(skb, port, dev, MACVLAN_MODE_BRIDGE);
> +			goto xmit_world;
> +		}
> +
> +		dest = macvlan_hash_lookup(port, eth->h_dest);
> +		if (dest && dest->mode == MACVLAN_MODE_BRIDGE) {
> +			int length = skb->len + ETH_HLEN;

unsigned int for length values please.

> +			int ret = dev_forward_skb(dest->dev, skb);
> +			macvlan_count_rx(dest, length,
> +					 likely(ret == NET_RX_SUCCESS), 0);
> +
> +			return NET_XMIT_SUCCESS;
> +		}
> +	}
> +
> +xmit_world:
> +	skb->dev = vlan->lowerdev;
> +	return dev_queue_xmit(skb);
> +}