Re: [PATCH net V2 2/2] net: core: explicitly select a txq beforedoing l2 forwarding

[Jason Wang]

On 01/09/2014 08:31 PM, Neil Horman wrote:
> 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@xxxxxxxxx>
>> Cc: Neil Horman <nhorman@xxxxxxxxxxxxx>
>> Cc: e1000-devel@xxxxxxxxxxxxxxxxxxxxx
>> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
>>
>> ---
>> 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.

Ok.
>> <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

Ture, and this will make no difference with the case without l2
forwarding. To not trouble other parts too much, I will keep the current
dev_queue_xmit() API and rename the current dev_queue_xmit() to
__dev_queue_xmit() can make it can accept a accel_priv parameter. So
dev_queue_xmit() will call this will NULL accel_priv and introduce a
dev_queue_xmit_accel() that can accept a accel_priv parameter.

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

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