Re: [RFC PATCH 1/2] net: Add layer 2 hardware acceleration operations for macvlan devices

[John Fastabend <john.fastabend@gmail.com>]

On 09/25/2013 01:16 PM, Neil Horman wrote:
> Add a operations structure that allows a network interface to export the fact
> that it supports package forwarding in hardware between physical interfaces and
> other mac layer devices assigned to it (such as macvlans).  this operaions
> structure can be used by virtual mac devices to bypass software switching so
> that forwarding can be done in hardware more efficiently.

Some additional nits below which maybe you have already thought of.

>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: john.fastabend@intel.com
> CC: "David S. Miller" <davem@davemloft.net>
> ---
>   drivers/net/macvlan.c      | 37 +++++++++++++++++++++++++++++++++++++
>   include/linux/if_macvlan.h |  1 +
>   include/linux/netdevice.h  | 10 ++++++++++
>   net/core/dev.c             |  3 +++
>   4 files changed, 51 insertions(+)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 9bf46bd..0c37b30 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -296,8 +296,16 @@ netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
>   	unsigned int len = skb->len;
>   	int ret;
>   	const struct macvlan_dev *vlan = netdev_priv(dev);
> +	const struct l2_forwarding_accel_ops *l2a_ops = vlan->lowerdev->l2a_ops;
> +
> +	if (l2a_ops->l2_accel_xmit) {
> +		ret = l2a_ops->l2_accel_xmit(skb, vlan->l2a_priv);
> +		if (likely(ret == NETDEV_TX_OK))

maybe dev_xmit_complete() would be more appropriate?

> +			goto update_stats;
> +	}
>
>   	ret = macvlan_queue_xmit(skb, dev);
> +update_stats:
>   	if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) {
>   		struct macvlan_pcpu_stats *pcpu_stats;
>
> @@ -336,6 +344,7 @@ static int macvlan_open(struct net_device *dev)
>   {
>   	struct macvlan_dev *vlan = netdev_priv(dev);
>   	struct net_device *lowerdev = vlan->lowerdev;
> +	const struct l2_forwarding_accel_ops *l2a_ops = lowerdev->l2a_ops;
>   	int err;
>
>   	if (vlan->port->passthru) {
> @@ -347,6 +356,19 @@ static int macvlan_open(struct net_device *dev)
>   		goto hash_add;

Looks like this might break in the passthru case? If you don't call
l2_accel_add_dev here but still use the l2_accel_xmit.

>   	}
>
> +	if (l2a_ops->l2_accel_add_dev) {

In the error cases it might be preferred to fallback to the
non-offloaded software path. For example hardware may have a limit
to the number of VSIs that can be created but we wouldn't want to
push that up the stack.

> +		/* The lowerdev supports l2 switching
> +		 * try to add this macvlan to it
> +		 */
> +		vlan->l2a_priv = kzalloc(l2a_ops->priv_size, GFP_KERNEL);
> +		if (!vlan->l2a_priv)
> +			return -ENOMEM;
> +		err = l2a_ops->l2_accel_add_dev(vlan->lowerdev,
> +						dev, vlan->l2a_priv);
> +		if (err < 0)
> +			return err;
> +	}
> +
>   	err = -EBUSY;
>   	if (macvlan_addr_busy(vlan->port, dev->dev_addr))
>   		goto out;
> @@ -367,6 +389,13 @@ hash_add:
>   del_unicast:
>   	dev_uc_del(lowerdev, dev->dev_addr);
>   out:
> +	if (vlan->l2a_priv) {

Add a feature flag here so it can be disabled.

> +		if (l2a_ops->l2_accel_del_dev)
> +			l2a_ops->l2_accel_del_dev(vlan->lowerdev,
> +						  vlan->l2a_priv);
> +		kfree(vlan->l2a_priv);
> +		vlan->l2a_priv = NULL;
> +	}
>   	return err;
>   }

[...]

Thanks,
John


-- 
John Fastabend         Intel Corporation