lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@intel.com>
    > Cc: Neil Horman <nhorman@tuxdriver.com>
    > Cc: e1000-devel@lists.sourceforge.net
    > Signed-off-by: Jason Wang <jasowang@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.

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



    \
     
     \ /
      Last update: 2014-01-09 14:21    [W:3.027 / U:0.440 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site