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
| ||
|
Date: Fri, 10 Jan 2014 15:03:01 +0800 From: Jason Wang <jasowang@...hat.com> To: Neil Horman <nhorman@...driver.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 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@...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. 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@...r.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 netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists