linux-kernel - Re: [PATCH net V2 2/2] net: core: explicitly select a txq before doing l2 forwarding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140109123144.GC16701@hmsreliant.think-freely.org>
Date:	Thu, 9 Jan 2014 07:31:44 -0500
From:	Neil Horman <nhorman@...driver.com>
To:	Jason Wang <jasowang@...hat.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, mst@...hat.com,
	John Fastabend <john.r.fastabend@...el.com>,
	e1000-devel@...ts.sourceforge.net
Subject: Re: [PATCH net V2 2/2] net: core: explicitly select a txq before
 doing l2 forwarding

On Thu, Jan 09, 2014 at 05:37:32PM +0800, Jason Wang wrote:
> Currently, the tx queue were selected implicitly in ndo_dfwd_start_xmit(). The
> will cause several issues:
> 
> - NETIF_F_LLTX were removed for macvlan, so txq lock were done for macvlan
>   instead of lower device which misses the necessary txq synchronization for
>   lower device such as txq stopping or frozen required by dev watchdog or
>   control path.
> - dev_hard_start_xmit() was called with NULL txq which bypasses the net device
>   watchdog.
> - dev_hard_start_xmit() does not check txq everywhere which will lead a crash
>   when tso is disabled for lower device.
> 
> Fix this by explicitly introducing a new param for .ndo_select_queue() for just
> selecting queues in the case of l2 forwarding offload. And also introducing
> dfwd_direct_xmit() to do the queue selecting, txq holding and transmitting for
> l2 forwarding.
> 
> With this fixes, NETIF_F_LLTX could be preserved for macvlan and there's no need
> to check txq against NULL in dev_hard_start_xmit(). Also there's no need to keep
> a dedicated ndo_dfwd_start_xmit().
> 
> In the future, it was also required for macvtap l2 forwarding support since it
> provides a necessary synchronization method.
> 
> Cc: John Fastabend <john.r.fastabend@...el.com>
> Cc: Neil Horman <nhorman@...driver.com>
> Cc: e1000-devel@...ts.sourceforge.net
> Signed-off-by: Jason Wang <jasowang@...hat.com>
> 
> ---
> Changes from V1:
> - Adding a new parameter to ndo_select_queue instead of a new method to select
>   queue for l2 forwarding.
> - Remove the unnecessary ndo_dfwd_start_xmit() since txq was selected
>   explicitly.
> - Keep NETIF_F_LLTX when netdev feature is changed.
> - Shape the commit log

A few minor nits inline.
><snip>

>  }
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 5360f73..7eb4c82 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -299,7 +299,7 @@ netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
>  
>  	if (vlan->fwd_priv) {
>  		skb->dev = vlan->lowerdev;
> -		ret = dev_hard_start_xmit(skb, skb->dev, NULL, vlan->fwd_priv);
> +		ret = dfwd_direct_xmit(skb, skb->dev, vlan->fwd_priv);
>  	} else {
>  		ret = macvlan_queue_xmit(skb, dev);
>  	}
> @@ -366,7 +366,6 @@ static int macvlan_open(struct net_device *dev)
>  		if (IS_ERR_OR_NULL(vlan->fwd_priv)) {
>  			vlan->fwd_priv = NULL;
>  		} else {
> -			dev->features &= ~NETIF_F_LLTX;
>  			return 0;
>  		}
After removing the features flag operation here, you don't need the braces
around the else statement either.

><snip>
> +int dfwd_direct_xmit(struct sk_buff *skb, struct net_device *dev,
> +		     void *accel_priv)
> +{
> +	struct netdev_queue *txq;
> +	int ret = NETDEV_TX_BUSY;
> +	int index;
> +
> +	BUG_ON(!dev->netdev_ops->ndo_select_queue);
> +	index =	dev->netdev_ops->ndo_select_queue(dev, skb, accel_priv);
> +
> +	local_bh_disable();
> +
> +	skb_set_queue_mapping(skb, index);
> +	txq = netdev_get_tx_queue(dev, index);
> +
> +	HARD_TX_LOCK(dev, txq, smp_processor_id());
> +	if (!netif_xmit_frozen_or_stopped(txq))
> +		ret = dev_hard_start_xmit(skb, dev, txq);
> +	HARD_TX_UNLOCK(dev, txq);
> +
> +	local_bh_enable();
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfwd_direct_xmit);
> +
Now that we're using the common path to select a queue, can we just use
dev_queue_xmit here instead of creating our own transmit function?  The txq we
select from the ixgbe card will just have a pfifo_fast queue on it (if not a
noop queue), so dev_queue_xmit should just fall into the dev_hard_start_xmit
path, and save us this extra coding.

Neil

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/