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

On 01/09/2014 08:31 PM, Neil Horman wrote:

On Thu, Jan 09, 2014 at 05:37:32PM +0800, Jason Wang wrote:

quoted

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 <redacted>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: e1000-devel@lists.sourceforge.net
Signed-off-by: Jason Wang <redacted>

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

quoted

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

quoted

<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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today. 
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired