* Re: Possible bug: SO_TIMESTAMPING 2.6.30+
From: Eric Dumazet @ 2009-11-10 8:44 UTC (permalink / raw)
To: Christopher Zimmermann; +Cc: Marcus D. Leech, netdev
In-Reply-To: <20091110091252.1667d27d@pundit>
Christopher Zimmermann a écrit :
> On Mon, 09 Nov 2009 19:40:30 -0500
> "Marcus D. Leech" <mleech@ripnet.com> wrote:
>
>
>> I know that Patrick Ohly has essentially moved on from doing the
>> SO_TIMESTAMPING stuff, so
>> who's maintaining it now?
>
> I worked on it a month ago or so and have a patchset from Patick Ohly
> and some changes by me which fix software timestamping and make the
> ioctl interface to the hardware more flexible (keeping backwards
> compatibility). Patches are attached.
> Still I never tried IPv6 and don't think the patches do anything about
> it.
> It would be nice to know weather software tx timestamps work
> with/without the patches.
>
>
> Christopher
>
I see some sock_put()/sock_hold() stuff in your second patch.
We removed these things on transmit path, please dont add them back...
If skb->sk is set, it probably also has a destructor and a reference already taken.
(This reference being on sk_wmem_alloc by the way, not on sk_refcnt)
You probably can try to change skb destructor ?
^ permalink raw reply
* Re: [PATCH] act_mirred: don't go back.
From: Changli Gao @ 2009-11-10 8:50 UTC (permalink / raw)
To: hadi; +Cc: Stephen Hemminger, David S. Miller, netdev
In-Reply-To: <1257838834.3647.4.camel@bigi>
On Tue, Nov 10, 2009 at 3:40 PM, jamal <hadi@cyberus.ca> wrote:
>
> I apologize - I am still not convinced this is a cleanup and i can
> already see holes you are introducing (example not freeing skb etc).
Where? After skb2 is allocated, there won't be any failure any more.
> You are putting me in a dilemma of not wanting to discourage you
> but at the same time not seeing this as a useful change to be made.
> Can we let this one slide?
>
It's just OK. When using tc, I also found act_mirred doesn't support
ingress, then I realized that there isn't any difference between
ingress and egress, as it depends on its parent. However I do think it
is confused, when it prints:
filter parent ffff: protocol ip pref 49152 basic handle 0x1
action order 1: mirred (Egress Redirect to device ifb0) stolen
index 5 ref 1 bind 1.
And the TODO note still is in the source code of act_mirred, it do
make me wonder for a while!
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 9:07 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF924A5.1050303@gmail.com>
Changli Gao a écrit :
> ifb: add multi-queue support
>
> Add multi-queue support, and one kernel thread is created for per queue.
> It can used to emulate multi-queue NIC in software, and distribute work
> among CPUs.
> gentux linux # modprobe ifb numtxqs=2
> gentux linux # ifconfig ifb0 up
> gentux linux # pgrep ifb0
> 18508
> 18509
> gentux linux # taskset -p 1 18508
> pid 18508's current affinity mask: 3
> pid 18508's new affinity mask: 1
> gentux linux # taskset -p 2 18509
> pid 18509's current affinity mask: 3
> pid 18509's new affinity mask: 2
> gentux linux # tc qdisc add dev br0 ingress
> gentux linux # tc filter add dev br0 parent ffff: protocol ip basic
> action mirred egress redirect dev ifb0
Seems pretty cool !
I have some comments
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ----
> drivers/net/ifb.c | 309
> ++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 186 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 030913f..6e04188 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> @@ -33,139 +33,101 @@
> #include <linux/etherdevice.h>
> #include <linux/init.h>
> #include <linux/moduleparam.h>
> +#include <linux/wait.h>
> +#include <linux/sched.h>
> +#include <linux/kthread.h>
> +#include <linux/ip.h>
> +#include <linux/ipv6.h>
> +#include <net/ip.h>
> #include <net/pkt_sched.h>
> #include <net/net_namespace.h>
>
> -#define TX_TIMEOUT (2*HZ)
> -
> #define TX_Q_LIMIT 32
> +
> struct ifb_private {
> - struct tasklet_struct ifb_tasklet;
> - int tasklet_pending;
> - /* mostly debug stats leave in for now */
> - unsigned long st_task_enter; /* tasklet entered */
> - unsigned long st_txq_refl_try; /* transmit queue refill attempt */
> - unsigned long st_rxq_enter; /* receive queue entered */
> - unsigned long st_rx2tx_tran; /* receive to trasmit transfers */
> - unsigned long st_rxq_notenter; /*receiveQ not entered, resched */
> - unsigned long st_rx_frm_egr; /* received from egress path */
> - unsigned long st_rx_frm_ing; /* received from ingress path */
> - unsigned long st_rxq_check;
> - unsigned long st_rxq_rsch;
> - struct sk_buff_head rq;
> - struct sk_buff_head tq;
> + struct net_device *dev;
> + struct sk_buff_head rq;
> + struct sk_buff_head tq;
> + wait_queue_head_t wq;
> + struct task_struct *task;
> };
Maybe you should allocate true per_cpu structure, to avoid cache line sharing
and get appropriate NUMA properties.
At least use a __cacheline_aligned_in_smp ...
>
> +/* Number of ifb devices to be set up by this module. */
> static int numifbs = 2;
> +module_param(numifbs, int, 0444);
> +MODULE_PARM_DESC(numifbs, "Number of ifb devices");
>
> -static void ri_tasklet(unsigned long dev);
> -static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
> -static int ifb_open(struct net_device *dev);
> -static int ifb_close(struct net_device *dev);
> +/* Number of TX queues per ifb */
> +static int numtxqs = 1;
> +module_param(numtxqs, int, 0444);
> +MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>
> -static void ri_tasklet(unsigned long dev)
> +static int ifb_thread(void *priv)
> {
> -
> - struct net_device *_dev = (struct net_device *)dev;
> - struct ifb_private *dp = netdev_priv(_dev);
> - struct net_device_stats *stats = &_dev->stats;
> - struct netdev_queue *txq;
> + struct ifb_private *dp = (struct ifb_private*)priv;
A space is required before * : (struct ifb_private *)priv;
(in many places in your patch)
> + struct net_device *dev = dp->dev;
> + struct net_device_stats *stats = &dev->stats;
Here you use a net_device_stats that is shared by all your queues,
your updates wont be protected and some will be lost.
You should use txq->tx_ counters.
> + unsigned int num = dp - (struct ifb_private*)netdev_priv(dev);
> + struct netdev_queue *txq = netdev_get_tx_queue(dev, num);
> struct sk_buff *skb;
> -
> - txq = netdev_get_tx_queue(_dev, 0);
> - dp->st_task_enter++;
> - if ((skb = skb_peek(&dp->tq)) == NULL) {
> - dp->st_txq_refl_try++;
> - if (__netif_tx_trylock(txq)) {
> - dp->st_rxq_enter++;
> - while ((skb = skb_dequeue(&dp->rq)) != NULL) {
> + DEFINE_WAIT(wait);
> +
> + while (1) {
> + /* move skb from rq to tq */
> + while (1) {
> + prepare_to_wait(&dp->wq, &wait, TASK_UNINTERRUPTIBLE);
> + while (!__netif_tx_trylock(txq))
> + yield();
> + while ((skb = skb_dequeue(&dp->rq)) != NULL)
> skb_queue_tail(&dp->tq, skb);
> - dp->st_rx2tx_tran++;
> - }
> + if (netif_queue_stopped(dev))
> + netif_wake_queue(dev);
> __netif_tx_unlock(txq);
> - } else {
> - /* reschedule */
> - dp->st_rxq_notenter++;
> - goto resched;
> + if (kthread_should_stop() || !skb_queue_empty(&dp->tq))
> + break;
> + schedule();
> }
> - }
> -
> - while ((skb = skb_dequeue(&dp->tq)) != NULL) {
> - u32 from = G_TC_FROM(skb->tc_verd);
> -
> - skb->tc_verd = 0;
> - skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
> - stats->tx_packets++;
> - stats->tx_bytes +=skb->len;
> -
> - skb->dev = dev_get_by_index(&init_net, skb->iif);
> - if (!skb->dev) {
> - dev_kfree_skb(skb);
> - stats->tx_dropped++;
> + finish_wait(&dp->wq, &wait);
> + if (kthread_should_stop())
> break;
> - }
> - dev_put(skb->dev);
> - skb->iif = _dev->ifindex;
> -
> - if (from & AT_EGRESS) {
> - dp->st_rx_frm_egr++;
> - dev_queue_xmit(skb);
> - } else if (from & AT_INGRESS) {
> - dp->st_rx_frm_ing++;
> - skb_pull(skb, skb->dev->hard_header_len);
> - netif_rx(skb);
> - } else
> - BUG();
> - }
>
> - if (__netif_tx_trylock(txq)) {
> - dp->st_rxq_check++;
> - if ((skb = skb_peek(&dp->rq)) == NULL) {
> - dp->tasklet_pending = 0;
> - if (netif_queue_stopped(_dev))
> - netif_wake_queue(_dev);
> - } else {
> - dp->st_rxq_rsch++;
> - __netif_tx_unlock(txq);
> - goto resched;
> + /* transfer packets */
> + while ((skb = skb_dequeue(&dp->tq)) != NULL) {
> + u32 from = G_TC_FROM(skb->tc_verd);
> +
> + skb->tc_verd = 0;
> + skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
> + stats->tx_packets++;
> + stats->tx_bytes +=skb->len;
Use : txq->tx_packets++
txq->tx_bytes += skb->len;
> +
> + skb->dev = dev_get_by_index(&init_net, skb->iif);
> + if (!skb->dev) {
> + dev_kfree_skb(skb);
> + stats->tx_dropped++;
txq->tx_dropped ?
> + break;
> + }
> + dev_put(skb->dev);
> + skb->iif = dev->ifindex;
> +
> + if (from & AT_EGRESS) {
> + dev_queue_xmit(skb);
> + } else if (from & AT_INGRESS) {
> + skb_pull(skb, skb->dev->hard_header_len);
> + netif_rx_ni(skb);
> + } else
> + BUG();
> }
> - __netif_tx_unlock(txq);
> - } else {
> -resched:
> - dp->tasklet_pending = 1;
> - tasklet_schedule(&dp->ifb_tasklet);
> }
>
> -}
> -
> -static const struct net_device_ops ifb_netdev_ops = {
> - .ndo_open = ifb_open,
> - .ndo_stop = ifb_close,
> - .ndo_start_xmit = ifb_xmit,
> - .ndo_validate_addr = eth_validate_addr,
> -};
> -
> -static void ifb_setup(struct net_device *dev)
> -{
> - /* Initialize the device structure. */
> - dev->destructor = free_netdev;
> - dev->netdev_ops = &ifb_netdev_ops;
> -
> - /* Fill in device structure with ethernet-generic values. */
> - ether_setup(dev);
> - dev->tx_queue_len = TX_Q_LIMIT;
> -
> - dev->flags |= IFF_NOARP;
> - dev->flags &= ~IFF_MULTICAST;
> - dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> - random_ether_addr(dev->dev_addr);
> + return 0;
> }
>
> static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> - struct ifb_private *dp = netdev_priv(dev);
> struct net_device_stats *stats = &dev->stats;
> u32 from = G_TC_FROM(skb->tc_verd);
> + int num = skb_get_queue_mapping(skb);
> + struct ifb_private *dp = ((struct ifb_private*)netdev_priv(dev)) + num;
>
> stats->rx_packets++;
> stats->rx_bytes+=skb->len;
Not sure how to solve this problem (several cpus can updates counter in //)
Thanks
^ permalink raw reply
* Re: [PATCH 2/2] au1000-eth: convert to platform_driver model
From: Manuel Lauss @ 2009-11-10 9:20 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Ralf Baechle, linux-mips, netdev, David Miller
In-Reply-To: <200911100113.38685.florian@openwrt.org>
Hi Florian,
On Tue, Nov 10, 2009 at 1:13 AM, Florian Fainelli <florian@openwrt.org> wrote:
> This patch converts the au1000-eth driver to become a full
> platform-driver as it ought to be. We now pass PHY-speficic
> configurations through platform_data but for compatibility
> the driver still assumes the default settings (search for PHY1 on
> MAC0) when no platform_data is passed. Tested on my MTX-1 board.
>
> Acked-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Florian Fainelli <florian@openwrt.org>
> ---
> diff --git a/drivers/net/au1000_eth.c b/drivers/net/au1000_eth.c
> index ce6f1ac..6d5a2cb 100644
> --- a/drivers/net/au1000_eth.c
> +++ b/drivers/net/au1000_eth.c
> -# if defined(AU1XXX_PHY1_SEARCH_ON_MAC0)
> - /* try harder to find a PHY */
> - if (!phydev && (aup->mac_id == 1)) {
> - /* no PHY found, maybe we have a dual PHY? */
> - printk (KERN_INFO DRV_NAME ": no PHY found on MAC1, "
> - "let's see if it's attached to MAC0...\n");
> -
> - BUG_ON(!au_macs[0]);
> - /* find the first (lowest address) non-attached PHY on
> - * the MAC0 MII bus */
> - for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
> - struct phy_device *const tmp_phydev =
> - au_macs[0]->mii_bus->phy_map[phy_addr];
> -
> - if (!tmp_phydev)
> - continue; /* no PHY here... */
> -
> - if (tmp_phydev->attached_dev)
> - continue; /* already claimed by MAC0 */
> + } else {
> + int phy_addr;
> +
> + /* find the first (lowest address) PHY on the current MAC's MII bus */
> + for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++)
> + if (aup->mii_bus->phy_map[phy_addr]) {
> + phydev = aup->mii_bus->phy_map[phy_addr];
> + if (!aup->phy_search_highest_addr)
> + break; /* break out with first one found */
> + }
>
> - phydev = tmp_phydev;
> - break; /* found it */
> + if (aup->phy1_search_mac0) {
> + /* try harder to find a PHY */
> + if (!phydev && (aup->mac_id == 1)) {
> + /* no PHY found, maybe we have a dual PHY? */
> + printk (KERN_INFO DRV_NAME ": no PHY found on MAC1, "
> + "let's see if it's attached to MAC0...\n");
> +
> + /* find the first (lowest address) non-attached PHY on
> + * the MAC0 MII bus */
> + for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
> + if (aup->mac_id == 1)
> + break;
aup->mac_id needs to be 1 for this loop to be executed in the first
place, and here
you immediately bail out if it is.
Also, how do you access the phy map of the other controller without use of the
au_macs[] structure? (which is unused after this patch and could be
removed, along
with the NUM_ETH_INTERFACES constant)
> + struct phy_device *const tmp_phydev =
> + aup->mii_bus->phy_map[phy_addr];
My compiler complains about mixed code/declarations.
Thanks!
Manuel Lauss
^ permalink raw reply
* Re: [PATCH] e1000e: Fix usage under kexec
From: Andi Kleen @ 2009-11-10 9:35 UTC (permalink / raw)
To: Sven Anders; +Cc: netdev
In-Reply-To: <4AF87AB1.1070603@anduras.de>
Sven Anders <sven@anduras.de> writes:
>
> We get the following error:
> e1000e: probe of 0000:02:00.0 failed with error -2
>
> We cannot unload the driver of the first kernel, because it's
> compiled in statically.
>
> It's caused by the PHY, because it's already initialized by the
> first driver and therefore does not respond correctly. The driver
> assumes the PHY to be in 'reset' state, which is done by the BIOS.
>
> We fixed it by adding a
>
> + /* Reset PHY before initializing it. Allows re-init after kexec. */
> + if (!e1000_check_reset_block(hw))
> + e1000_phy_hw_reset(hw);
The problem of resetting PHYs in the driver init path is that it tends
to lose link-state, and if the link takes some time to re-negotiate
you can significantly slow down the "total time to network access"
at boot.
Perhaps you could only reset when the message above would trigger
and try again?
-Andi
^ permalink raw reply
* Re: [RFC,PATCH] mutex: mutex_is_owner() helper
From: Peter Zijlstra @ 2009-11-10 9:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ingo Molnar, Linus Torvalds, David S. Miller, Linux Netdev List,
linux kernel, Thomas Gleixner
In-Reply-To: <4AF8A40B.10708@gmail.com>
On Tue, 2009-11-10 at 00:21 +0100, Eric Dumazet wrote:
> Peter Zijlstra a écrit :
> > On Wed, 2009-11-04 at 18:19 +0100, Eric Dumazet wrote:
> >> BTW, I was thinking of a mutex_yield() implementation, but could not
> >> cook it without hard thinking, maybe you already have some nice
> >> implementation ?
> >
> > Why? Yield sets off alarm bells, since 99.9%, and possibly more, of its
> > uses are wrong.
>
> If I remember well, I had problems doing "modprobe dummy numdummies=30000",
> because it creates 30000 netdevices, and thanks to hotplug starts 30000 udev
> that all wait that my modprobe is finished... Nice to see load average going
> so big by the way :)
lol :-) With a bit of luck udev will spawn a python interpreter for each
of those things too..
> I tried following patch without success, because rtnl_unlock()/rtnl_lock()
> is too fast (awaken process(es) ha(s/ve) no chance to get the lock, as we
> take it immediately after releasing it)
Right, due to lock-stealing.
> diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
> index 37dcfdc..108c4fa 100644
> --- a/drivers/net/dummy.c
> +++ b/drivers/net/dummy.c
> @@ -138,8 +138,12 @@ static int __init dummy_init_module(void)
> rtnl_lock();
> err = __rtnl_link_register(&dummy_link_ops);
>
> - for (i = 0; i < numdummies && !err; i++)
> + for (i = 0; i < numdummies && !err; i++) {
> err = dummy_init_one();
> + rtnl_unlock();
> + msleep(1);
> + rtnl_lock();
> + }
> if (err < 0)
> __rtnl_link_unregister(&dummy_link_ops);
> rtnl_unlock();
>
> But if hotplug is disabled, this force a useless msleep(1) * 30000 -> this is bit slow
>
> Yes, this code is stupid, but I use it to stress network stack
> with insane number of devices, to spot scalability problems.
Right...
> mutex_yield() could help in this situation.
Agreed, except I don't like the name, but I could be tained from
sched_yield().
> mutex is said to be FIFO, but its not exactly true : A new comer can take the mutex
> even if 10000 threads are waiting on mutex...
Yep, lock-stealing, you don't want to see the regression reports if you
'fix' that :-)
> I wont mention other problems, because mutex_{try}lock() has no timedwait variant
Nobody needed it I guess.. also I never quite understood the need for
timedwait, either you need to get the work done or you don't, not maybe.
Use mutex_lock_interruptible() and set a timer or something.
> , and funny code doing :
>
> if (!rtnl_trylock())
> return restart_syscall();
>
> Making 30000 processes running/fighting to get the mutex :(
Funny definition of funny ;-) That's some seriously fugly code there.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 9:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF92D6D.8060300@gmail.com>
2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>
> Not sure how to solve this problem (several cpus can updates counter in //)
>
Thanks, and follow your suggestions. I can maintain the counter per TX
queue, and update it in a timer handler like ixgbe or implement our
own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
and update the counters when it gets called.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 10:29 UTC (permalink / raw)
To: xiaosuo; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF924A5.1050303@gmail.com>
Changli Gao wrote:
> ifb: add multi-queue support
>
> Add multi-queue support, and one kernel thread is created for per queue.
> It can used to emulate multi-queue NIC in software, and distribute work
> among CPUs.
> gentux linux # modprobe ifb numtxqs=2
> gentux linux # ifconfig ifb0 up
> gentux linux # pgrep ifb0
> 18508
> 18509
> gentux linux # taskset -p 1 18508
> pid 18508's current affinity mask: 3
> pid 18508's new affinity mask: 1
> gentux linux # taskset -p 2 18509
> pid 18509's current affinity mask: 3
> pid 18509's new affinity mask: 2
> gentux linux # tc qdisc add dev br0 ingress
> gentux linux # tc filter add dev br0 parent ffff: protocol ip basic
> action mirred egress redirect dev ifb0
I'm not sure how this will help, ifb device transmission is
still serialized by the ingress queue lock and the mirred lock.
^ permalink raw reply
* netfilter -stable 00/02: netfilter -stable fixes
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
The following two patches fix two bug in netfilter:
- a bug in TCP conntrack sequence tracking when used with NAT helpers that
enlarge packets
- a regression in the xt_connlimit match introduced in 2.6.29, causing
false negatives
Please apply, thanks.
include/net/netfilter/nf_conntrack.h | 8 +--
include/net/netfilter/nf_nat_helper.h | 4 ++
net/ipv4/netfilter/nf_nat_core.c | 3 +
net/ipv4/netfilter/nf_nat_helper.c | 34 +++++++++++-----
net/netfilter/nf_conntrack_core.c | 8 ++++
net/netfilter/nf_conntrack_proto_tcp.c | 64 +++++++++++++-------------------
net/netfilter/xt_connlimit.c | 10 ++---
7 files changed, 71 insertions(+), 60 deletions(-)
Jan Engelhardt (1):
netfilter: xt_connlimit: fix regression caused by zero family value
Jozsef Kadlecsik (1):
netfilter: nf_nat: fix NAT issue in 2.6.30.4+
^ permalink raw reply
* netfilter -stable 01/02: nf_nat: fix NAT issue in 2.6.30.4+
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
In-Reply-To: <20091110104014.8250.89589.sendpatchset@x2.localnet>
commit eb3336f5b440ae5c15d09abe20d7bdd45a88d157
Author: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Tue Nov 10 10:53:08 2009 +0100
netfilter: nf_nat: fix NAT issue in 2.6.30.4+
Upstream commit f9dd09c7:
Vitezslav Samel discovered that since 2.6.30.4+ active FTP can not work
over NAT. The "cause" of the problem was a fix of unacknowledged data
detection with NAT (commit a3a9f79e361e864f0e9d75ebe2a0cb43d17c4272).
However, actually, that fix uncovered a long standing bug in TCP conntrack:
when NAT was enabled, we simply updated the max of the right edge of
the segments we have seen (td_end), by the offset NAT produced with
changing IP/port in the data. However, we did not update the other parameter
(td_maxend) which is affected by the NAT offset. Thus that could drift
away from the correct value and thus resulted breaking active FTP.
The patch below fixes the issue by *not* updating the conntrack parameters
from NAT, but instead taking into account the NAT offsets in conntrack in a
consistent way. (Updating from NAT would be more harder and expensive because
it'd need to re-calculate parameters we already calculated in conntrack.)
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 5d9a848..a96b835 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -252,11 +252,9 @@ static inline bool nf_ct_kill(struct nf_conn *ct)
}
/* These are for NAT. Icky. */
-/* Update TCP window tracking data when NAT mangles the packet */
-extern void nf_conntrack_tcp_update(const struct sk_buff *skb,
- unsigned int dataoff,
- struct nf_conn *ct, int dir,
- s16 offset);
+extern s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
/* Fake conntrack entry for untracked connections */
extern struct nf_conn nf_conntrack_untracked;
diff --git a/include/net/netfilter/nf_nat_helper.h b/include/net/netfilter/nf_nat_helper.h
index 237a961..4222220 100644
--- a/include/net/netfilter/nf_nat_helper.h
+++ b/include/net/netfilter/nf_nat_helper.h
@@ -32,4 +32,8 @@ extern int (*nf_nat_seq_adjust_hook)(struct sk_buff *skb,
* to port ct->master->saved_proto. */
extern void nf_nat_follow_master(struct nf_conn *ct,
struct nf_conntrack_expect *this);
+
+extern s16 nf_nat_get_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
#endif
diff --git a/net/ipv4/netfilter/nf_nat_core.c b/net/ipv4/netfilter/nf_nat_core.c
index b6ddd56..d396abf 100644
--- a/net/ipv4/netfilter/nf_nat_core.c
+++ b/net/ipv4/netfilter/nf_nat_core.c
@@ -750,6 +750,8 @@ static int __init nf_nat_init(void)
BUG_ON(nfnetlink_parse_nat_setup_hook != NULL);
rcu_assign_pointer(nfnetlink_parse_nat_setup_hook,
nfnetlink_parse_nat_setup);
+ BUG_ON(nf_ct_nat_offset != NULL);
+ rcu_assign_pointer(nf_ct_nat_offset, nf_nat_get_offset);
return 0;
cleanup_extend:
@@ -764,6 +766,7 @@ static void __exit nf_nat_cleanup(void)
nf_ct_extend_unregister(&nat_extend);
rcu_assign_pointer(nf_nat_seq_adjust_hook, NULL);
rcu_assign_pointer(nfnetlink_parse_nat_setup_hook, NULL);
+ rcu_assign_pointer(nf_ct_nat_offset, NULL);
synchronize_net();
}
diff --git a/net/ipv4/netfilter/nf_nat_helper.c b/net/ipv4/netfilter/nf_nat_helper.c
index 05ede41..79e28ad 100644
--- a/net/ipv4/netfilter/nf_nat_helper.c
+++ b/net/ipv4/netfilter/nf_nat_helper.c
@@ -73,6 +73,28 @@ adjust_tcp_sequence(u32 seq,
DUMP_OFFSET(this_way);
}
+/* Get the offset value, for conntrack */
+s16 nf_nat_get_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq)
+{
+ struct nf_conn_nat *nat = nfct_nat(ct);
+ struct nf_nat_seq *this_way;
+ s16 offset;
+
+ if (!nat)
+ return 0;
+
+ this_way = &nat->seq[dir];
+ spin_lock_bh(&nf_nat_seqofs_lock);
+ offset = after(seq, this_way->correction_pos)
+ ? this_way->offset_after : this_way->offset_before;
+ spin_unlock_bh(&nf_nat_seqofs_lock);
+
+ return offset;
+}
+EXPORT_SYMBOL_GPL(nf_nat_get_offset);
+
/* Frobs data inside this packet, which is linear. */
static void mangle_contents(struct sk_buff *skb,
unsigned int dataoff,
@@ -189,11 +211,6 @@ nf_nat_mangle_tcp_packet(struct sk_buff *skb,
adjust_tcp_sequence(ntohl(tcph->seq),
(int)rep_len - (int)match_len,
ct, ctinfo);
- /* Tell TCP window tracking about seq change */
- nf_conntrack_tcp_update(skb, ip_hdrlen(skb),
- ct, CTINFO2DIR(ctinfo),
- (int)rep_len - (int)match_len);
-
nf_conntrack_event_cache(IPCT_NATSEQADJ, ct);
}
return 1;
@@ -415,12 +432,7 @@ nf_nat_seq_adjust(struct sk_buff *skb,
tcph->seq = newseq;
tcph->ack_seq = newack;
- if (!nf_nat_sack_adjust(skb, tcph, ct, ctinfo))
- return 0;
-
- nf_conntrack_tcp_update(skb, ip_hdrlen(skb), ct, dir, seqoff);
-
- return 1;
+ return nf_nat_sack_adjust(skb, tcph, ct, ctinfo);
}
/* Setup NAT on this expected conntrack so it follows master. */
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 0d961ee..56ed268 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1296,6 +1296,11 @@ err_stat:
return ret;
}
+s16 (*nf_ct_nat_offset)(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq);
+EXPORT_SYMBOL_GPL(nf_ct_nat_offset);
+
int nf_conntrack_init(struct net *net)
{
int ret;
@@ -1313,6 +1318,9 @@ int nf_conntrack_init(struct net *net)
/* For use by REJECT target */
rcu_assign_pointer(ip_ct_attach, nf_conntrack_attach);
rcu_assign_pointer(nf_ct_destroy, destroy_conntrack);
+
+ /* Howto get NAT offsets */
+ rcu_assign_pointer(nf_ct_nat_offset, NULL);
}
return 0;
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index a38bc22..718c334 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -482,6 +482,21 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
}
}
+#ifdef CONFIG_NF_NAT_NEEDED
+static inline s16 nat_offset(const struct nf_conn *ct,
+ enum ip_conntrack_dir dir,
+ u32 seq)
+{
+ typeof(nf_ct_nat_offset) get_offset = rcu_dereference(nf_ct_nat_offset);
+
+ return get_offset != NULL ? get_offset(ct, dir, seq) : 0;
+}
+#define NAT_OFFSET(pf, ct, dir, seq) \
+ (pf == NFPROTO_IPV4 ? nat_offset(ct, dir, seq) : 0)
+#else
+#define NAT_OFFSET(pf, ct, dir, seq) 0
+#endif
+
static bool tcp_in_window(const struct nf_conn *ct,
struct ip_ct_tcp *state,
enum ip_conntrack_dir dir,
@@ -496,6 +511,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
struct ip_ct_tcp_state *receiver = &state->seen[!dir];
const struct nf_conntrack_tuple *tuple = &ct->tuplehash[dir].tuple;
__u32 seq, ack, sack, end, win, swin;
+ s16 receiver_offset;
bool res;
/*
@@ -509,11 +525,16 @@ static bool tcp_in_window(const struct nf_conn *ct,
if (receiver->flags & IP_CT_TCP_FLAG_SACK_PERM)
tcp_sack(skb, dataoff, tcph, &sack);
+ /* Take into account NAT sequence number mangling */
+ receiver_offset = NAT_OFFSET(pf, ct, !dir, ack - 1);
+ ack -= receiver_offset;
+ sack -= receiver_offset;
+
pr_debug("tcp_in_window: START\n");
pr_debug("tcp_in_window: ");
nf_ct_dump_tuple(tuple);
- pr_debug("seq=%u ack=%u sack=%u win=%u end=%u\n",
- seq, ack, sack, win, end);
+ pr_debug("seq=%u ack=%u+(%d) sack=%u+(%d) win=%u end=%u\n",
+ seq, ack, receiver_offset, sack, receiver_offset, win, end);
pr_debug("tcp_in_window: sender end=%u maxend=%u maxwin=%u scale=%i "
"receiver end=%u maxend=%u maxwin=%u scale=%i\n",
sender->td_end, sender->td_maxend, sender->td_maxwin,
@@ -599,8 +620,8 @@ static bool tcp_in_window(const struct nf_conn *ct,
pr_debug("tcp_in_window: ");
nf_ct_dump_tuple(tuple);
- pr_debug("seq=%u ack=%u sack =%u win=%u end=%u\n",
- seq, ack, sack, win, end);
+ pr_debug("seq=%u ack=%u+(%d) sack=%u+(%d) win=%u end=%u\n",
+ seq, ack, receiver_offset, sack, receiver_offset, win, end);
pr_debug("tcp_in_window: sender end=%u maxend=%u maxwin=%u scale=%i "
"receiver end=%u maxend=%u maxwin=%u scale=%i\n",
sender->td_end, sender->td_maxend, sender->td_maxwin,
@@ -686,7 +707,7 @@ static bool tcp_in_window(const struct nf_conn *ct,
before(seq, sender->td_maxend + 1) ?
after(end, sender->td_end - receiver->td_maxwin - 1) ?
before(sack, receiver->td_end + 1) ?
- after(ack, receiver->td_end - MAXACKWINDOW(sender)) ? "BUG"
+ after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1) ? "BUG"
: "ACK is under the lower bound (possible overly delayed ACK)"
: "ACK is over the upper bound (ACKed data not seen yet)"
: "SEQ is under the lower bound (already ACKed data retransmitted)"
@@ -701,39 +722,6 @@ static bool tcp_in_window(const struct nf_conn *ct,
return res;
}
-#ifdef CONFIG_NF_NAT_NEEDED
-/* Update sender->td_end after NAT successfully mangled the packet */
-/* Caller must linearize skb at tcp header. */
-void nf_conntrack_tcp_update(const struct sk_buff *skb,
- unsigned int dataoff,
- struct nf_conn *ct, int dir,
- s16 offset)
-{
- const struct tcphdr *tcph = (const void *)skb->data + dataoff;
- const struct ip_ct_tcp_state *sender = &ct->proto.tcp.seen[dir];
- const struct ip_ct_tcp_state *receiver = &ct->proto.tcp.seen[!dir];
- __u32 end;
-
- end = segment_seq_plus_len(ntohl(tcph->seq), skb->len, dataoff, tcph);
-
- write_lock_bh(&tcp_lock);
- /*
- * We have to worry for the ack in the reply packet only...
- */
- if (ct->proto.tcp.seen[dir].td_end + offset == end)
- ct->proto.tcp.seen[dir].td_end = end;
- ct->proto.tcp.last_end = end;
- write_unlock_bh(&tcp_lock);
- pr_debug("tcp_update: sender end=%u maxend=%u maxwin=%u scale=%i "
- "receiver end=%u maxend=%u maxwin=%u scale=%i\n",
- sender->td_end, sender->td_maxend, sender->td_maxwin,
- sender->td_scale,
- receiver->td_end, receiver->td_maxend, receiver->td_maxwin,
- receiver->td_scale);
-}
-EXPORT_SYMBOL_GPL(nf_conntrack_tcp_update);
-#endif
-
#define TH_FIN 0x01
#define TH_SYN 0x02
#define TH_RST 0x04
^ permalink raw reply related
* netfilter -stable 02/02: xt_connlimit: fix regression caused by zero family value
From: Patrick McHardy @ 2009-11-10 10:40 UTC (permalink / raw)
To: stable; +Cc: netdev, Patrick McHardy, netfilter-devel, davem
In-Reply-To: <20091110104014.8250.89589.sendpatchset@x2.localnet>
commit 4dcaa8fa124a6d8ab0f64dc1c289b13a724f8fc1
Author: Jan Engelhardt <jengelh@medozas.de>
Date: Tue Nov 10 10:53:33 2009 +0100
netfilter: xt_connlimit: fix regression caused by zero family value
Upstream commit 539054a8:
Commit v2.6.28-rc1~717^2~109^2~2 was slightly incomplete; not all
instances of par->match->family were changed to par->family.
References: http://bugzilla.netfilter.org/show_bug.cgi?id=610
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 6809809..38f03f7 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -103,7 +103,7 @@ static int count_them(struct xt_connlimit_data *data,
const struct nf_conntrack_tuple *tuple,
const union nf_inet_addr *addr,
const union nf_inet_addr *mask,
- const struct xt_match *match)
+ u_int8_t family)
{
const struct nf_conntrack_tuple_hash *found;
struct xt_connlimit_conn *conn;
@@ -113,8 +113,7 @@ static int count_them(struct xt_connlimit_data *data,
bool addit = true;
int matches = 0;
-
- if (match->family == NFPROTO_IPV6)
+ if (family == NFPROTO_IPV6)
hash = &data->iphash[connlimit_iphash6(addr, mask)];
else
hash = &data->iphash[connlimit_iphash(addr->ip & mask->ip)];
@@ -157,8 +156,7 @@ static int count_them(struct xt_connlimit_data *data,
continue;
}
- if (same_source_net(addr, mask, &conn->tuple.src.u3,
- match->family))
+ if (same_source_net(addr, mask, &conn->tuple.src.u3, family))
/* same source network -> be counted! */
++matches;
nf_ct_put(found_ct);
@@ -207,7 +205,7 @@ connlimit_mt(const struct sk_buff *skb, const struct xt_match_param *par)
spin_lock_bh(&info->data->lock);
connections = count_them(info->data, tuple_ptr, &addr,
- &info->mask, par->match);
+ &info->mask, par->family);
spin_unlock_bh(&info->data->lock);
if (connections < 0) {
^ permalink raw reply related
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 10:48 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF9406C.8010603@trash.net>
On Tue, Nov 10, 2009 at 6:29 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> I'm not sure how this will help, ifb device transmission is
> still serialized by the ingress queue lock and the mirred lock.
>
But not in ifb_thread. For firewall the main routine is after
netif_rx_ni(). For server applications, the packets belong to the same
flow are always routed to the same CPU, and the applications can
benefit with better localization.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 10:55 UTC (permalink / raw)
To: Changli Gao; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100248mbc14287p26c43141d6ab202c@mail.gmail.com>
Changli Gao a écrit :
> On Tue, Nov 10, 2009 at 6:29 PM, Patrick McHardy <kaber@trash.net> wrote:
>> I'm not sure how this will help, ifb device transmission is
>> still serialized by the ingress queue lock and the mirred lock.
>>
>
> But not in ifb_thread. For firewall the main routine is after
> netif_rx_ni(). For server applications, the packets belong to the same
> flow are always routed to the same CPU, and the applications can
> benefit with better localization.
>
Hmm, you know many cache lines already were bringed into cpu receiving
the original softirq ? But yes, this is a possible way to go / try :)
Please submit your future patch on top of net-next-2.6, because
I see you still use dev_get_by_index() :-(
commit 05e8689c9a3a208bf75b60662778d81e23eac460
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun Nov 1 19:45:16 2009 +0000
ifb: RCU locking avoids touching dev refcount
Avoids touching dev refcount in hotpath
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
commit db519144243de6b17ff0c56c26f06059743110a7
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue Oct 20 02:35:50 2009 +0000
ifb: should not use __dev_get_by_index() without locks
At this point (ri_tasklet()), RTNL or dev_base_lock are not held,
we must use dev_get_by_index() instead of __dev_get_by_index()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 10:57 UTC (permalink / raw)
To: Changli Gao; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100143k68284faes92e39d8ca94aa9d4@mail.gmail.com>
Changli Gao a écrit :
> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>> Not sure how to solve this problem (several cpus can updates counter in //)
>>
> Thanks, and follow your suggestions. I can maintain the counter per TX
> queue, and update it in a timer handler like ixgbe or implement our
> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
> and update the counters when it gets called.
Please no timer stuff :)
^ permalink raw reply
* Re: RFC: net: allow to propagate errors through ->ndo_hard_start_xmit()
From: Patrick McHardy @ 2009-11-10 11:04 UTC (permalink / raw)
To: Herbert Xu
Cc: Linux Netdev List, Jarek Poplawski, David S. Miller,
Stephen Hemminger
In-Reply-To: <20091109195000.GA10325@gondor.apana.org.au>
Herbert Xu wrote:
> On Mon, Nov 09, 2009 at 08:41:36PM +0100, Patrick McHardy wrote:
>> - I'm not sure the error handling in dev_hard_start_xmit() for GSO
>> skbs is optimal. When the driver returns an error, it is assumed
>> the current segment has been freed. The patch then frees the
>> entire GSO skb, including all remaining segments. Alternatively
>> it could try to transmit the remaining segments later.
>
> Well driver errors (not queueing errors) should never happen.
Yeah, usually there will only be queueing errors. One case for
a non-queueing error might be to return EHOSTUNREACH from ipip
or gre when there's no route to the peer.
> And if they do then they're likely to persist. So freeing the
> rest should be sufficient, unless of course if doing it some
> other way is simpler :)
This way seems simpler. Thanks.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 11:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF9471C.5080606@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 643 bytes --]
2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
> Changli Gao a écrit :
>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>> Not sure how to solve this problem (several cpus can updates counter in //)
>>>
>> Thanks, and follow your suggestions. I can maintain the counter per TX
>> queue, and update it in a timer handler like ixgbe or implement our
>> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
>> and update the counters when it gets called.
>
> Please no timer stuff :)
The whole ifb.c file is attached, please review and test it. Thanks!
--
Regards，
Changli Gao(xiaosuo@gmail.com)
[-- Attachment #2: ifb.c --]
[-- Type: application/octet-stream, Size: 9063 bytes --]
/* drivers/net/ifb.c:
The purpose of this driver is to provide a device that allows
for sharing of resources:
1) qdiscs/policies that are per device as opposed to system wide.
ifb allows for a device which can be redirected to thus providing
an impression of sharing.
2) Allows for queueing incoming traffic for shaping instead of
dropping.
The original concept is based on what is known as the IMQ
driver initially written by Martin Devera, later rewritten
by Patrick McHardy and then maintained by Andre Correa.
You need the tc action mirror or redirect to feed this device
packets.
This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License
as published by the Free Software Foundation; either version
2 of the License, or (at your option) any later version.
Authors: Jamal Hadi Salim (2005)
*/
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/init.h>
#include <linux/moduleparam.h>
#include <linux/wait.h>
#include <linux/sched.h>
#include <linux/kthread.h>
#include <linux/ip.h>
#include <linux/ipv6.h>
#include <net/ip.h>
#include <net/pkt_sched.h>
#include <net/net_namespace.h>
#define TX_Q_LIMIT 32
struct ifb_private {
struct net_device *dev;
struct sk_buff_head rq;
struct sk_buff_head tq;
wait_queue_head_t wq;
struct task_struct *task;
unsigned long rx_packets;
unsigned long rx_bytes;
unsigned long rx_dropped;
} ____cacheline_aligned_in_smp;
/* Number of ifb devices to be set up by this module. */
static int numifbs = 2;
module_param(numifbs, int, 0444);
MODULE_PARM_DESC(numifbs, "Number of ifb devices");
/* Number of TX queues per ifb */
static int numtxqs = 1;
module_param(numtxqs, int, 0444);
MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
static int ifb_thread(void *priv)
{
struct ifb_private *dp = (struct ifb_private *)priv;
struct net_device *dev = dp->dev;
unsigned int num = dp - (struct ifb_private *)netdev_priv(dev);
struct netdev_queue *txq = netdev_get_tx_queue(dev, num);
struct sk_buff *skb;
DEFINE_WAIT(wait);
while (1) {
/* move skb from rq to tq */
while (1) {
prepare_to_wait(&dp->wq, &wait, TASK_UNINTERRUPTIBLE);
while (!__netif_tx_trylock(txq))
yield();
while ((skb = skb_dequeue(&dp->rq)) != NULL)
skb_queue_tail(&dp->tq, skb);
if (netif_queue_stopped(dev))
netif_wake_queue(dev);
__netif_tx_unlock(txq);
if (kthread_should_stop() || !skb_queue_empty(&dp->tq))
break;
schedule();
}
finish_wait(&dp->wq, &wait);
if (kthread_should_stop())
break;
/* transfer packets */
while ((skb = skb_dequeue(&dp->tq)) != NULL) {
u32 from = G_TC_FROM(skb->tc_verd);
skb->tc_verd = 0;
skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
txq->tx_packets++;
txq->tx_bytes +=skb->len;
rcu_read_lock();
skb->dev = dev_get_by_index_rcu(&init_net, skb->iif);
if (!skb->dev) {
rcu_read_unlock();
dev_kfree_skb(skb);
txq->tx_dropped++;
break;
}
rcu_read_unlock();
skb->iif = dev->ifindex;
if (from & AT_EGRESS) {
dev_queue_xmit(skb);
} else if (from & AT_INGRESS) {
skb_pull(skb, skb->dev->hard_header_len);
netif_rx_ni(skb);
} else
BUG();
}
}
return 0;
}
struct net_device_stats* ifb_get_stats(struct net_device *dev)
{
struct net_device_stats *stats = &dev->stats;
struct ifb_private *dp = netdev_priv(dev);
struct netdev_queue *txq;
int i;
unsigned long rx_packets = 0, rx_bytes = 0, rx_dropped = 0;
unsigned long tx_packets = 0, tx_bytes = 0, tx_dropped = 0;
for (i = 0; i < dev->real_num_tx_queues; i++) {
rx_packets += dp[i].rx_packets;
rx_bytes += dp[i].rx_bytes;
rx_dropped += dp[i].rx_dropped;
txq = netdev_get_tx_queue(dev, i);
tx_packets = txq->tx_packets;
tx_bytes = txq->tx_bytes;
tx_dropped += txq->tx_dropped;
}
stats->rx_packets = rx_packets;
stats->rx_bytes = rx_bytes;
stats->rx_dropped = rx_dropped;
stats->tx_packets = tx_packets;
stats->tx_bytes = tx_bytes;
stats->tx_dropped = tx_dropped;
return stats;
}
static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev)
{
u32 from = G_TC_FROM(skb->tc_verd);
int num = skb_get_queue_mapping(skb);
struct ifb_private *dp = ((struct ifb_private *)netdev_priv(dev)) + num;
struct netdev_queue *txq = netdev_get_tx_queue(dev, num);
dp->rx_packets++;
dp->rx_bytes+=skb->len;
if (!(from & (AT_INGRESS|AT_EGRESS)) || !skb->iif) {
dev_kfree_skb(skb);
dp->rx_dropped++;
return NETDEV_TX_OK;
}
if (skb_queue_len(&dp->rq) >= dev->tx_queue_len) {
netif_stop_queue(dev);
}
txq->trans_start = jiffies;
skb_queue_tail(&dp->rq, skb);
if (skb_queue_len(&dp->rq) == 1)
wake_up(&dp->wq);
return NETDEV_TX_OK;
}
static int ifb_close(struct net_device *dev)
{
struct ifb_private *dp = netdev_priv(dev);
int i;
for (i = 0; i < dev->real_num_tx_queues; i++) {
kthread_stop(dp[i].task);
skb_queue_purge(&dp[i].tq);
skb_queue_purge(&dp[i].rq);
}
netif_stop_queue(dev);
return 0;
}
static int ifb_open(struct net_device *dev)
{
struct ifb_private *dp = netdev_priv(dev);
int i;
for (i = 0; i < dev->real_num_tx_queues; i++) {
dp[i].dev = dev;
skb_queue_head_init(&dp[i].rq);
skb_queue_head_init(&dp[i].tq);
init_waitqueue_head(&dp[i].wq);
dp[i].task = kthread_run(ifb_thread, &dp[i], "%s/%d", dev->name,
i);
if (IS_ERR(dp[i].task)) {
int err = PTR_ERR(dp[i].task);
while (--i >= 0)
kthread_stop(dp[i].task);
return err;
}
}
netif_start_queue(dev);
return 0;
}
static u32 simple_tx_hashrnd;
static u16 ifb_select_queue(struct net_device *dev, struct sk_buff *skb)
{
u32 addr1, addr2;
u32 hash, ihl;
union {
u16 in16[2];
u32 in32;
} ports;
u8 ip_proto;
if ((hash = skb_rx_queue_recorded(skb))) {
while (hash >= dev->real_num_tx_queues)
hash -= dev->real_num_tx_queues;
return hash;
}
switch (skb->protocol) {
case __constant_htons(ETH_P_IP):
if (!(ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)))
ip_proto = ip_hdr(skb)->protocol;
else
ip_proto = 0;
addr1 = ip_hdr(skb)->saddr;
addr2 = ip_hdr(skb)->daddr;
ihl = ip_hdr(skb)->ihl << 2;
break;
case __constant_htons(ETH_P_IPV6):
ip_proto = ipv6_hdr(skb)->nexthdr;
addr1 = ipv6_hdr(skb)->saddr.s6_addr32[3];
addr2 = ipv6_hdr(skb)->daddr.s6_addr32[3];
ihl = 10;
break;
default:
return 0;
}
if (addr1 > addr2)
swap(addr1, addr2);
switch (ip_proto) {
case IPPROTO_TCP:
case IPPROTO_UDP:
case IPPROTO_DCCP:
case IPPROTO_ESP:
case IPPROTO_AH:
case IPPROTO_SCTP:
case IPPROTO_UDPLITE:
ports.in32 = *((u32 *) (skb_network_header(skb) + ihl));
if (ports.in16[0] > ports.in16[1])
swap(ports.in16[0], ports.in16[1]);
break;
default:
ports.in32 = 0;
break;
}
hash = jhash_3words(addr1, addr2, ports.in32,
simple_tx_hashrnd ^ ip_proto);
return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
}
static const struct net_device_ops ifb_netdev_ops = {
.ndo_open = ifb_open,
.ndo_stop = ifb_close,
.ndo_start_xmit = ifb_xmit,
.ndo_validate_addr = eth_validate_addr,
.ndo_select_queue = ifb_select_queue,
.ndo_get_stats = ifb_get_stats,
};
static void ifb_setup(struct net_device *dev)
{
/* Initialize the device structure. */
dev->destructor = free_netdev;
dev->netdev_ops = &ifb_netdev_ops;
/* Fill in device structure with ethernet-generic values. */
ether_setup(dev);
dev->tx_queue_len = TX_Q_LIMIT;
dev->flags |= IFF_NOARP;
dev->flags &= ~IFF_MULTICAST;
dev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
random_ether_addr(dev->dev_addr);
}
static int ifb_validate(struct nlattr *tb[], struct nlattr *data[])
{
if (tb[IFLA_ADDRESS]) {
if (nla_len(tb[IFLA_ADDRESS]) != ETH_ALEN)
return -EINVAL;
if (!is_valid_ether_addr(nla_data(tb[IFLA_ADDRESS])))
return -EADDRNOTAVAIL;
}
return 0;
}
static struct rtnl_link_ops ifb_link_ops __read_mostly = {
.kind = "ifb",
.priv_size = sizeof(struct ifb_private),
.setup = ifb_setup,
.validate = ifb_validate,
};
static int __init ifb_init_one(int index)
{
struct net_device *dev_ifb;
int err;
dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
ifb_setup, numtxqs);
if (!dev_ifb)
return -ENOMEM;
err = dev_alloc_name(dev_ifb, dev_ifb->name);
if (err < 0)
goto err;
dev_ifb->rtnl_link_ops = &ifb_link_ops;
err = register_netdevice(dev_ifb);
if (err < 0)
goto err;
return 0;
err:
free_netdev(dev_ifb);
return err;
}
static int __init ifb_init_module(void)
{
int i, err;
get_random_bytes(&simple_tx_hashrnd, 4);
rtnl_lock();
err = __rtnl_link_register(&ifb_link_ops);
for (i = 0; i < numifbs && !err; i++)
err = ifb_init_one(i);
if (err)
__rtnl_link_unregister(&ifb_link_ops);
rtnl_unlock();
return err;
}
static void __exit ifb_cleanup_module(void)
{
rtnl_link_unregister(&ifb_link_ops);
}
module_init(ifb_init_module);
module_exit(ifb_cleanup_module);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Jamal Hadi Salim");
MODULE_ALIAS_RTNL_LINK("ifb");
^ permalink raw reply
* Re: [PATCH] can: fix WARN_ON dump in net/core/rtnetlink.c:rtmsg_ifinfo()
From: Patrick McHardy @ 2009-11-10 11:24 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: David Miller, netdev, socketcan-core
In-Reply-To: <4AF68FCB.4050103@grandegger.com>
Wolfgang Grandegger wrote:
> David Miller wrote:
>> From: Wolfgang Grandegger <wg@grandegger.com>
>> Date: Sat, 07 Nov 2009 10:53:13 +0100
>>
>>> On older kernels, e.g. 2.6.27, a WARN_ON dump in rtmsg_ifinfo()
>>> is thrown when the CAN device is registered due to insufficient
>>> skb space, as reported by various users. This patch adds the
>>> rtnl_link_ops "get_size" to fix the problem. I think this patch
>>> is required for more recent kernels as well, even if no WARN_ON
>>> dumps are triggered. Maybe we also need "get_xstats_size" for
>>> the CAN xstats.
>>>
>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> Applied to net-2.6, thanks Wolfgang.
>
> Thanks, the commit message included some questions. What is the rule
> using the rtnl_link_ops "get_size" or "get_xstats_size". Are these
> mandatory if the corresponding fill functions are used?
Yes. You also need a get_xstats_size() function.
^ permalink raw reply
* Re: [PATCHv9 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-10 11:36 UTC (permalink / raw)
To: Rusty Russell
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm,
hpa, gregory.haskins, s.hetze, Daniel Walker, Eric Dumazet
In-Reply-To: <200911101349.09783.rusty@rustcorp.com.au>
On Tue, Nov 10, 2009 at 01:49:09PM +1030, Rusty Russell wrote:
> One fix:
>
> vhost: fix TUN=m VHOST_NET=y
>
> drivers/built-in.o: In function `get_tun_socket':
> net.c:(.text+0x15436e): undefined reference to `tun_get_socket'
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
> drivers/vhost/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -1,6 +1,6 @@
> config VHOST_NET
> tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
> - depends on NET && EVENTFD && EXPERIMENTAL
> + depends on NET && EVENTFD && TUN && EXPERIMENTAL
> ---help---
> This kernel module can be loaded in host kernel to accelerate
> guest networking with virtio_net. Not to be confused with virtio_net
In fact, vhost can be built with TUN=n VHOST_NET=y as well
(tun_get_socket is stubbed out in that case).
So I think this is better (it looks strange
until you realize that for tristate variables
boolean logic math does not apply):
--->
From: Michael S. Tsirkin <mst@redhat.com>
Subject: vhost: fix TUN=m VHOST_NET=y
drivers/built-in.o: In function `get_tun_socket':
net.c:(.text+0x15436e): undefined reference to `tun_get_socket'
If tun is a module, vhost must be a module, too.
If tun is built-in or disabled, vhost can be built-in.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 9f409f4..9e93553 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,6 +1,6 @@
config VHOST_NET
tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
- depends on NET && EVENTFD && EXPERIMENTAL
+ depends on NET && EVENTFD && (TUN || !TUN) && EXPERIMENTAL
---help---
This kernel module can be loaded in host kernel to accelerate
guest networking with virtio_net. Not to be confused with virtio_net
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 11:41 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100314i33d6a49dl5163b5e8472babcf@mail.gmail.com>
Changli Gao wrote:
> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>> Changli Gao a écrit :
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>> Not sure how to solve this problem (several cpus can updates counter in //)
>>>>
>>> Thanks, and follow your suggestions. I can maintain the counter per TX
>>> queue, and update it in a timer handler like ixgbe or implement our
>>> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
>>> and update the counters when it gets called.
>> Please no timer stuff :)
>
> The whole ifb.c file is attached, please review and test it. Thanks!
> /* Number of TX queues per ifb */
> static int numtxqs = 1;
> module_param(numtxqs, int, 0444);
> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
Module parameters suck as a configuration API. The existing numifbs
option exists purely for compatibility reasons, I'd prefer if you'd
this to the netlink interface.
> static int __init ifb_init_one(int index)
> {
> struct net_device *dev_ifb;
> int err;
>
> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
> ifb_setup, numtxqs);
>
This won't work for the rtnl_link setup since the size must
be constant.
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Jean Delvare @ 2009-11-10 11:42 UTC (permalink / raw)
To: Stephen Rothwell, Ben Hutchings
Cc: David Miller, netdev, linux-next, linux-kernel, Mika Kuoppala
In-Reply-To: <20091026133757.7cf87e49.sfr@canb.auug.org.au>
Hi Stephen, Ben,
On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> Today's linux-next merge of the net tree got a conflict in
> drivers/net/sfc/sfe4001.c between commit
> 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> inversion on top of bus lock") from the i2c tree and commit
> c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> falcon_boards.c") from the net tree.
>
> I have applied the following merge fixup patch (after removing
> drivers/net/sfc/sfe4001.c) and can carry it as necessary.
I've merged the new API to get and release the i2c_adapter mutex:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=afa08974fe80c198b8650f73ed8ab59135ca10d0
Ben, you can adjust your own patches to make use of this API instead of
accessing the i2c_adapter mutex directly. That way, you are no longer
dependent of implementation changes, and this should solve the conflict.
Stephen, you can then drop your fixup patch.
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 11:53 UTC (permalink / raw)
To: Gregory Haskins; +Cc: netdev, linux-kernel, herbert.xu
In-Reply-To: <20091002141407.30224.54207.stgit@dev.haskins.net>
On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
> (Applies to davem/net-2.6.git:4fdb78d30)
>
> Hi David, netdevs,
>
> The following is an RFC for an attempt at addressing a zero-copy solution.
>
> To be perfectly honest, I have no idea if this is the best solution, or if
> there is truly a problem with skb->destructor that requires an alternate
> mechanism. What I do know is that this patch seems to work, and I would
> like to see some kind of solution available upstream. So I thought I would
> send my hack out as at least a point of discussion. FWIW: This has been
> tested heavily in my rig and is technically suitable for inclusion after
> review as is, if that is decided to be the optimal path forward here.
>
> Thanks for your review and consideration,
>
> Kind regards,
> -Greg
>
> ----------------------------------------
> From: Gregory Haskins <ghaskins@novell.com>
> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>
> What: The skb->destructor field is reportedly unreliable for ensuring
> that all shinfo users have dropped their references. Therefore, we add
> a distinct ->release() method for the shinfo structure which is closely
> tied to the underlying page resources we want to protect.
>
> Why: We want to add zero-copy transmit support for AlacrityVM guests.
> In order to support this, the host kernel must map guest pages directly
> into a paged-skb and send it as normal. put_page() alone is not
> sufficient lifetime management since the pages are ultimately allocated
> from within the guest. Therefore, we need higher-level notification
> when the skb is finally freed on the host so we can then inject a proper
> "tx-complete" event into the guest context.
>
> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> ---
>
> include/linux/skbuff.h | 2 ++
> net/core/skbuff.c | 9 +++++++++
> 2 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index df7b23a..02cdab6 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -207,6 +207,8 @@ struct skb_shared_info {
> /* Intermediate layers must ensure that destructor_arg
> * remains valid until skb destructor */
> void * destructor_arg;
> + void * priv;
> + void (*release)(struct sk_buff *skb);
> };
>
> /* We divide dataref into two halves. The higher 16 bits hold references
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 80a9616..a7e40a9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
> shinfo->tx_flags.flags = 0;
> skb_frag_list_init(skb);
> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> + shinfo->release = NULL;
> + shinfo->priv = NULL;
>
> if (fclone) {
> struct sk_buff *child = skb + 1;
> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
> if (skb_has_frags(skb))
> skb_drop_fraglist(skb);
>
> + if (skb_shinfo(skb)->release)
> + skb_shinfo(skb)->release(skb);
> +
> kfree(skb->head);
> }
> }
> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
> shinfo->tx_flags.flags = 0;
> skb_frag_list_init(skb);
> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> + shinfo->release = NULL;
> + shinfo->priv = NULL;
>
> memset(skb, 0, offsetof(struct sk_buff, tail));
> skb->data = skb->head + NET_SKB_PAD;
> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
> skb->hdr_len = 0;
> skb->nohdr = 0;
> atomic_set(&skb_shinfo(skb)->dataref, 1);
> + skb_shinfo(skb)->release = NULL;
> + skb_shinfo(skb)->priv = NULL;
> return 0;
>
> nodata:
This is basically subset of the skb data destructors patch, isn't it?
Last time this was tried, this is the objection that was voiced:
The problem with this patch is that it's tracking skb's, while
you want use it to track pages for zero-copy. That just doesn't
work. Through mechanisms like splice, individual pages in the
skb can be detached and metastasize to other locations, e.g.,
the VFS.
and I think this applies here. In other words, this only *seems*
to work for you because you are not trying to do things like
guest to host communication, with host doing smart things.
Cc Herbert which was involved in the original discussion.
In the specific case, it seems that things like pskb_copy,
skb_split and others will also be broken, won't they?
--
MST
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:14 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95174.3060104@trash.net>
On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
> Changli Gao wrote:
>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>
>> The whole ifb.c file is attached, please review and test it. Thanks!
>
>> /* Number of TX queues per ifb */
>> static int numtxqs = 1;
>> module_param(numtxqs, int, 0444);
>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>
> Module parameters suck as a configuration API. The existing numifbs
> option exists purely for compatibility reasons, I'd prefer if you'd
> this to the netlink interface.
How to do that? I haven't found any examples. Is there a interface to
config the number of the TX queues of a NIC.
>
>> static int __init ifb_init_one(int index)
>> {
>> struct net_device *dev_ifb;
>> int err;
>>
>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>> ifb_setup, numtxqs);
>>
>
> This won't work for the rtnl_link setup since the size must
> be constant.
>
Does this work?
ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
rtnl_lock();
err = __rtnl_link_register(&ifb_link_ops);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 12:19 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100414w7b6f45aap39f6568b7af2c1a7@mail.gmail.com>
Changli Gao wrote:
> On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
>> Changli Gao wrote:
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>
>>> The whole ifb.c file is attached, please review and test it. Thanks!
>>> /* Number of TX queues per ifb */
>>> static int numtxqs = 1;
>>> module_param(numtxqs, int, 0444);
>>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>> Module parameters suck as a configuration API. The existing numifbs
>> option exists purely for compatibility reasons, I'd prefer if you'd
>> this to the netlink interface.
>
> How to do that? I haven't found any examples. Is there a interface to
> config the number of the TX queues of a NIC.
You have to add a get_tx_queues() callback to the rtnl_link_ops.
Additionally you need a new attribute (IFLA_NTXQ or something like
that) that contains the number of queues. The callback has to parse
the attribute and set the number of queues accordingly.
>>> static int __init ifb_init_one(int index)
>>> {
>>> struct net_device *dev_ifb;
>>> int err;
>>>
>>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>>> ifb_setup, numtxqs);
>>>
>> This won't work for the rtnl_link setup since the size must
>> be constant.
>>
>
> Does this work?
>
> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
> rtnl_lock();
> err = __rtnl_link_register(&ifb_link_ops);
Only for the module parameter. For rtnl_link you need to either
allocate the private space seperately or turn priv_size into
a callback that returns the required space based on the number
of queues.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95A3D.5060602@trash.net>
On Tue, Nov 10, 2009 at 8:19 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> You have to add a get_tx_queues() callback to the rtnl_link_ops.
It is used to get the current real_num_tx_queues and num_tx_queues,
not to setting.
> Additionally you need a new attribute (IFLA_NTXQ or something like
> that) that contains the number of queues. The callback has to parse
> the attribute and set the number of queues accordingly.
It seems another patch is needed first.
>> Does this work?
>>
>> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
>> rtnl_lock();
>> err = __rtnl_link_register(&ifb_link_ops);
>
> Only for the module parameter. For rtnl_link you need to either
> allocate the private space seperately or turn priv_size into
> a callback that returns the required space based on the number
> of queues.
Do you means that if module ifb is loaded automatically, parameters
won't be set correctly?
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 12:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110115335.GC6989@redhat.com>
>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
>> (Applies to davem/net-2.6.git:4fdb78d30)
>>
>> Hi David, netdevs,
>>
>> The following is an RFC for an attempt at addressing a zero-copy solution.
>>
>> To be perfectly honest, I have no idea if this is the best solution, or if
>> there is truly a problem with skb->destructor that requires an alternate
>> mechanism. What I do know is that this patch seems to work, and I would
>> like to see some kind of solution available upstream. So I thought I would
>> send my hack out as at least a point of discussion. FWIW: This has been
>> tested heavily in my rig and is technically suitable for inclusion after
>> review as is, if that is decided to be the optimal path forward here.
>>
>> Thanks for your review and consideration,
>>
>> Kind regards,
>> -Greg
>>
>> ----------------------------------------
>> From: Gregory Haskins <ghaskins@novell.com>
>> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>>
>> What: The skb->destructor field is reportedly unreliable for ensuring
>> that all shinfo users have dropped their references. Therefore, we add
>> a distinct ->release() method for the shinfo structure which is closely
>> tied to the underlying page resources we want to protect.
>>
>> Why: We want to add zero-copy transmit support for AlacrityVM guests.
>> In order to support this, the host kernel must map guest pages directly
>> into a paged-skb and send it as normal. put_page() alone is not
>> sufficient lifetime management since the pages are ultimately allocated
>> from within the guest. Therefore, we need higher-level notification
>> when the skb is finally freed on the host so we can then inject a proper
>> "tx-complete" event into the guest context.
>>
>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>> ---
>>
>> include/linux/skbuff.h | 2 ++
>> net/core/skbuff.c | 9 +++++++++
>> 2 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index df7b23a..02cdab6 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -207,6 +207,8 @@ struct skb_shared_info {
>> /* Intermediate layers must ensure that destructor_arg
>> * remains valid until skb destructor */
>> void * destructor_arg;
>> + void * priv;
>> + void (*release)(struct sk_buff *skb);
>> };
>>
>> /* We divide dataref into two halves. The higher 16 bits hold references
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 80a9616..a7e40a9 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> gfp_mask,
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> if (fclone) {
>> struct sk_buff *child = skb + 1;
>> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
>> if (skb_has_frags(skb))
>> skb_drop_fraglist(skb);
>>
>> + if (skb_shinfo(skb)->release)
>> + skb_shinfo(skb)->release(skb);
>> +
>> kfree(skb->head);
>> }
>> }
>> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> memset(skb, 0, offsetof(struct sk_buff, tail));
>> skb->data = skb->head + NET_SKB_PAD;
>> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> ntail,
>> skb->hdr_len = 0;
>> skb->nohdr = 0;
>> atomic_set(&skb_shinfo(skb)->dataref, 1);
>> + skb_shinfo(skb)->release = NULL;
>> + skb_shinfo(skb)->priv = NULL;
>> return 0;
>>
>> nodata:
>
> This is basically subset of the skb data destructors patch, isn't it?
Sort of, but the emphasis is different. Here are the main differences:
1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
"the owner" and is thus only set at creation.
3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
> Last time this was tried, this is the objection that was voiced:
>
> The problem with this patch is that it's tracking skb's, while
> you want use it to track pages for zero-copy. That just doesn't
> work. Through mechanisms like splice, individual pages in the
> skb can be detached and metastasize to other locations, e.g.,
> the VFS.
Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>
> and I think this applies here.
I don't think so, but if you think I missed something, do not be shy (not that you ever are).
> In other words, this only *seems*
> to work for you because you are not trying to do things like
> guest to host communication, with host doing smart things.
I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>
> Cc Herbert which was involved in the original discussion.
>
> In the specific case, it seems that things like pskb_copy,
> skb_split and others will also be broken, won't they?
Not to my knowledge. They up the reference to the shinfo before proceeding.
Kind Regards,
-Greg
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox