On Fri, Jan 10, 2014 at 03:03:01PM +0800, Jason Wang wrote:
> 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.fastab...@intel.com>
> >> Cc: Neil Horman <nhor...@tuxdriver.com>
> >> Cc: e1000-de...@lists.sourceforge.net
> >> Signed-off-by: Jason Wang <jasow...@redhat.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.
>
> 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.
>
Sounds perfect, thanks!
Neil
> Thanks
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majord...@vger.kernel.org
> > 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
