* Re: [PATCH 1/9] ipv6: convert temporary address list to list macros
From: David Miller @ 2010-03-20 23:05 UTC (permalink / raw)
To: shemminger; +Cc: netdev
In-Reply-To: <20100318063122.866367333@vyatta.com>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 17 Mar 2010 23:31:09 -0700
> Use list macros instead of open coded linked list.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Applied.
^ permalink raw reply
* [GIT]: Networking
From: David Miller @ 2010-03-20 22:23 UTC (permalink / raw)
To: torvalds; +Cc: akpm, netdev, linux-kernel
1) The TCP TTL limiting extension added to 2.6.34 needs to
check ICMP packets destined for such TCP sockets too.
Fix from Stephen Hemminger.
2) KS8851 regression, OOPS on multicast changes. Fix from
Abraham Arce.
3) TCP started setting POLLIN for pure OOB data accidently.
Noticed and fixed by Alexandra Kossovsky.
4) Add PCI IDs for new IGB device, from Carolyn Wyborny.
5) skb_bond_should_drop() should use ACCESS_ONCE() on
skb->dev_master to avoid potential oops, from Eric Dumazet.
6) TCP can mark the retransmit queue head SKB lost even when
->packets_out == 0, causing early retransmits. Fix from
Lennart Schulte.
7) NET_DMA can wedge a TCP socket if receive water marks are high
and the user does a large read. Fix by periodically submitting
and purging completed NET_DMA requests, instead of doing so only
when exiting tcp_recvmsg(). Fix from Steven J. Magnani.
8) Netlink fixes from Pablo Neira Ayuso:
a) nla_get_be64() needs to use nla_memcpy() since the object
will always be unaligned.
b) Fix NETLINK_RECV_NO_ENOBUFS to elide the error even when
it gets signalled via netlink_set_err().
c) Use the fix #b to cure ctnetlink from losing events.
9) VHOST bug fixes from Jeff Dike, Michael S. Tsirkin:
a) vhost_net_set_backend() can return with mutex held.
b) logic error causes interrupt mitigation to never trigger
c) several vring ioctls accidently return with mutex held
10) JME driver bug fixes from Guo-Fu Tseng:
a) VLAN RX code paths leaks memory
b) VLGRP acceses are racy
11) Fix Fibre link issues and and wake on lan in IXGBE driver.
From Mallikarjuna R Chilakala and Jeff Kirsher.
12) smsc95xx needs workaround for checksum offload of tiny UDP packets.
13) Recent bug fix unearthed an error in IPSEC route updating for
ipv4. We need to implement ipv4_dst_check() properly, otherwise
PMTU updates et al. never propagate any more to IPSEC routes.
Fix from Timo Teras, with help from Herbert Xu.
14) IXGBE indicates netdev feature changes too early, it must
occur after FCOE tx queue setup is done. Fix from Vasu Dev.
15) IXGBE can potentially double unmap DMA mappings, noticed by
Simon Horman and fixed by Mallikarjuna R Chilakala.
16) Multicast code can access vif_table[] outside of array bounds.
Fix from Patrick McHardy.
17) KS8695 updates descriptor head way too often, only hit it at
end of RX processing loop. From Yegor Yefremov with help
from Eric Dumazet.
18) IXGBEVF driver fixes from Greg Rose and Jeff Kirsher:
a) VF stats don't account properly across reset
b) recovery from PF resets take way too long, reduce
watchdog delay
Please pull, thanks a lot!
The following changes since commit 220bf991b0366cc50a94feede3d7341fa5710ee4:
Linus Torvalds (1):
Linux 2.6.34-rc2
are available in the git repository at:
master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master
Abraham Arce (1):
KS8851: Avoid NULL pointer in set rx mode
Adel Gadllah (1):
iwlwifi: Silence tfds_in_queue message
Alexandra Kossovsky (1):
tcp: Fix OOB POLLIN avoidance.
Carolyn Wyborny (1):
igb: Add support for 82576 ET2 Quad Port Server Adapter
David S. Miller (2):
Merge branch 'master' of git://git.kernel.org/.../linville/wireless-2.6
Merge branch 'vhost' of git://git.kernel.org/.../mst/vhost
Eric Dumazet (1):
net: Potential null skb->dev dereference
Felix Fietkau (1):
ath9k: fix BUG_ON triggered by PAE frames
Grazvydas Ignotas (1):
wl1251: fix potential crash
Greg Rose (3):
ixgbevf: Fix VF Stats accounting after reset
ixgbevf: Shorten up delay timer for watchdog task
ixgbevf: Message formatting cleanups
Guo-Fu Tseng (3):
jme: Fix VLAN memory leak
jme: Protect vlgrp structure by pause RX actions.
jme: Advance driver version number
Herbert Xu (1):
ipv6: Remove redundant dst NULL check in ip6_dst_check
Jeff Dike (1):
vhost: fix error path in vhost_net_set_backend
Lennart Schulte (1):
tcp: Fix tcp_mark_head_lost() with packets == 0
Mallikarjuna R Chilakala (3):
ixgbe: Fix 82599 multispeed fiber link issues due to Tx laser flapping
ixgbe: Fix 82599 KX4 Wake on LAN issue after an improper system shutdown
ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address
Michael S. Tsirkin (2):
vhost: fix interrupt mitigation with raw sockets
vhost: fix error handling in vring ioctls
Pablo Neira Ayuso (3):
netlink: fix unaligned access in nla_get_be64()
netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
netfilter: ctnetlink: fix reliable event delivery if message building fails
Patrick McHardy (1):
net: ipmr/ip6mr: fix potential out-of-bounds vif_table access
Steve Glendinning (1):
smsc95xx: Fix tx checksum offload for small packets
Steven J. Magnani (1):
NET_DMA: free skbs periodically
Timo Teräs (1):
ipv4: check rt_genid in dst_check
Vasu Dev (1):
ixgbe: fix for real_num_tx_queues update issue
Yegor Yefremov (1):
KS8695: update ksp->next_rx_desc_read at the end of rx loop
stephen hemminger (1):
TCP: check min TTL on received ICMP packets
drivers/net/arm/ks8695net.c | 24 +++-----
drivers/net/igb/e1000_82575.c | 1 +
drivers/net/igb/e1000_hw.h | 1 +
drivers/net/igb/igb_main.c | 1 +
drivers/net/ixgbe/ixgbe_82599.c | 78 ++++++++++++++------------
drivers/net/ixgbe/ixgbe_fcoe.c | 6 +-
drivers/net/ixgbe/ixgbe_main.c | 22 ++++++--
drivers/net/ixgbe/ixgbe_type.h | 1 +
drivers/net/ixgbevf/ethtool.c | 42 +++++++++-----
drivers/net/ixgbevf/ixgbevf_main.c | 74 +++++++++++++++---------
drivers/net/ixgbevf/vf.h | 6 ++
drivers/net/jme.c | 35 ++++++++++++
drivers/net/jme.h | 2 +-
drivers/net/ks8851.c | 1 -
drivers/net/usb/smsc95xx.c | 18 +++++-
drivers/net/wireless/ath/ath9k/xmit.c | 21 +-------
drivers/net/wireless/iwlwifi/iwl-tx.c | 2 +-
drivers/net/wireless/wl12xx/wl1251_debugfs.c | 3 +-
drivers/vhost/net.c | 10 ++-
drivers/vhost/vhost.c | 18 ++++--
include/linux/netdevice.h | 8 +-
include/linux/netfilter/nfnetlink.h | 2 +-
include/linux/netlink.h | 2 +-
include/net/netlink.h | 6 ++-
net/8021q/vlan_core.c | 4 +-
net/core/dev.c | 8 ++-
net/ipv4/ipmr.c | 3 +
net/ipv4/route.c | 14 +++-
net/ipv4/tcp.c | 65 ++++++++++++++-------
net/ipv4/tcp_input.c | 3 +
net/ipv4/tcp_ipv4.c | 5 ++
net/ipv6/ip6mr.c | 3 +
net/ipv6/route.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 4 +-
net/netfilter/nfnetlink.c | 4 +-
net/netlink/af_netlink.c | 17 +++++-
36 files changed, 335 insertions(+), 181 deletions(-)
^ permalink raw reply
* Re: [GIT PULL] vhost-net fixes for issues in 2.6.34-rc1
From: David Miller @ 2010-03-20 21:41 UTC (permalink / raw)
To: mst
Cc: rusty, kvm, virtualization, netdev, linux-kernel, samudrala,
jirislaby, chavey, quintela, unai.uribarri, jdike
In-Reply-To: <20100318095355.GA24194@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 18 Mar 2010 11:53:55 +0200
> The following tree includes patches fixing issues with vhost-net in
> 2.6.34-rc1. Please pull them for 2.6.34.
Pulled, thanks a lot.
^ permalink raw reply
* Re: [PATCH 1/3] netlink: fix NETLINK_RECV_NO_ENOBUFS in netlink_set_err()
From: David Miller @ 2010-03-20 21:30 UTC (permalink / raw)
To: pablo; +Cc: kaber, netdev
In-Reply-To: <4BA2C44A.1000100@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Fri, 19 Mar 2010 01:24:42 +0100
> Patrick McHardy wrote:
>> Generally the logic seems inverted, you should return an error
>> to conntrack if userspace wasn't notified of the error.
>
> Indeed, thanks. Are you OK with this patch instead?
I went over all of this and now the patches #1 and #2 look
correct to me, so I've applied them to net-2.6
Patrick let me know if you think any follow-on tidy ups
are still necessary and we can add them.
Thanks Pablo!
^ permalink raw reply
* Re: [PATCH] NET_DMA: free skbs periodically
From: David Miller @ 2010-03-20 21:29 UTC (permalink / raw)
To: steve; +Cc: netdev, kuznet, pekkas, jmorris, yoshfuji, kaber, linux-kernel
In-Reply-To: <1268752964-4397-1-git-send-email-steve@digidescorp.com>
From: "Steven J. Magnani" <steve@digidescorp.com>
Date: Tue, 16 Mar 2010 10:22:44 -0500
> Under NET_DMA, data transfer can grind to a halt when userland issues a
> large read on a socket with a high RCVLOWAT (i.e., 512 KB for both).
> This appears to be because the NET_DMA design queues up lots of memcpy
> operations, but doesn't issue or wait for them (and thus free the
> associated skbs) until it is time for tcp_recvmesg() to return.
> The socket hangs when its TCP window goes to zero before enough data is
> available to satisfy the read.
>
> Periodically issue asynchronous memcpy operations, and free skbs for ones
> that have completed, to prevent sockets from going into zero-window mode.
>
> Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
Applied, thanks for fixing this bug Steven.
^ permalink raw reply
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
From: David Miller @ 2010-03-20 21:24 UTC (permalink / raw)
To: darrenrjenkins
Cc: cindy.h.kao, wimax, netdev, kernel-janitors, linux-kernel,
linux-wimax
In-Reply-To: <1268902008.10536.41.camel@ICE-BOX>
From: Darren Jenkins <darrenrjenkins@gmail.com>
Date: Thu, 18 Mar 2010 19:46:48 +1100
>
> On Thu, Mar 18, 2010 at 9:10 AM, Inaky Perez-Gonzalez
> <inaky.perez-gonzalez@intel.com> wrote:
>
>> If krealloc() fails to aallocate a new pointer, the old block is
>> unmodified, so by doing this you are leaking a buffer allocation.
>
> It seems you are right.
> So now understanding correctly how krealloc() works I can see that the
> double kfree() can only actually happen if the el_size parameter to
> i2400m_zrealloc_2x() is zero, and it isn't at the two call sites.
>
> So this was a false positive and I am sorry for the noise.
No problem, at least the code got audited :-)
^ permalink raw reply
* Gianfar: RX Recycle skb->len error
From: Ben Menchaca (ben@bigfootnetworks.com) @ 2010-03-20 19:54 UTC (permalink / raw)
To: netdev@vger.kernel.org
We are seeing some random skb data length errors on RX after long-running, full-gigabit traffic. First, my debugging and solution are based on the following invariant assumption:
(skb->tail - skb->data) == skb->len
If this is wrong, please educate.
After some tracing, here is where the error packets seem to originate:
1. We are cleaning rx, in gfar_clean_rx_ring;
2. A new RX skb is drawn from the rx_recycle queue, and obey the above invariant (so, in gfar_new_skb(), __skb_dequeue returns an skb);
3. At this point skb_reserve is called, which moves data and tail by the same calculated alignamount;
4. So, newskb is not NULL. However, !(bdp->status & RXBD_LAST) || (bdp->status & RXBD_ERR)) is evaluates to true;
5. Since newskb is not NULL, we arrive at the else if (skb), which is true;
6. skb->data = skb->head + NET_SKB_PAD is applied, and then the skb is requeued for recycling.
At this point, skb->data != skb->tail, but skb->len == 0. When this skb is used for the next RX, it is causing issues later when we skb_put trailers, and then trust skb->len.
I would propose something like:
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2540,6 +2540,7 @@
* recycle list.
*/
skb->data = skb->head + NET_SKB_PAD;
+ skb_reset_tail_pointer(skb);
__skb_queue_head(&priv->rx_recycle, skb);
}
} else {
Ben Menchaca
Bigfoot Networks
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Timo Teräs @ 2010-03-20 16:26 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, David S. Miller
In-Reply-To: <20100320151751.GB2950@gondor.apana.org.au>
Herbert Xu wrote:
> On Fri, Mar 19, 2010 at 11:53:44AM +0200, Timo Teräs wrote:
>> But now, thinking more, it would probably make more sense to
>> just cache xfrm_dst's and keep ref to the policy on them. So
>> in general I agree with your recommendation. The only immediate
>> problem I can think now is that the resolved would need to
>> atomically check if xfrm_dst is valid, if not, resolve new one.
>> But creating new xfrm_dst involves taking locks and can sleep
>> so it cannot be inside the main resolver.
>
> OK this brings out my favourite topic :)
>
> The reason we have to sleep is to resolve the template. So if
> we had queueing for pending xfrm_dst objects, we wouldn't have
> to sleep at all when creating the top-level xfrm_dst.
>
> Packets using that xfrm_dst can wait in the queue until it is
> fully resolved. Now obviously this code doesn't exist so this
> is all just a wet dream.
Right. That sounds useful.
> Setting my favourite topic aside, I have to come to the conclusion
> that your patch still doesn't fully resolve the problem you set out
> to fix.
>
> The crux of the issue is the linked list of all bundles in a
> policy and the obvious problems stemming from walking a linked
> list that is unbounded.
>
> The reason I think it doesn't fully resolve this is because of
> the flow cache. Being a per-cpu cache, when you create the xfrm
> dst the first time around, you'll at most put it in one CPU's
> cache.
>
> The next CPU that comes along will still have to walk that same
> bundle linked list. So we're back to square one.
Not exactly, each CPU does one slow lookup after which it
finds it fast. But yes, it's not perfect solution. Especially,
if cpu happens to get switched between the initial lookup and
the update.
> Now Dave, my impression is that we picked the per-cpu design
> because it was the best data structure we had back in 2002,
> right?
>
> If so I'd like us to think about the possibility of switching
> over to a different design, in particular, an RCU-based hash
> table, similar to the one I just used for bridge multicasting.
>
> This would eliminate the need for walking the bundle list apart
> from the case when we're destroying the policy, which can be
> done in process context.
Right. This would speed the bundle lookup in all cases.
Except... we can have override policy on per-socket basis.
We should include the per-socket override in the flow lookups
so that those sockets get also boost from the cache. Though
usual use case is to disable all policies (so e.g. IKE can
be talked without policies applying).
> Actually I just realised that the other way we can fix this is
> to make xfrm_dst objects per-cpu just like IPv4 routes. That
> is, when you fail to find an xfrm_dst object in the per-cpu
> cache, you dont' bother calling xfrm_find_bundle but just make
> a new bundle.
>
> This is probably much easier than replacing the whole flow cache.
> Can any one think of any problems with duplicate xfrm_dst objects?
Sounds like a very good idea. If we instantiate new xfrm_dst,
all that it shares with others is xfrm_state and xfrm_policy
(inner objects will be unique). Since that's what happens anyway
I don't see any problem with this.
So should go ahead and:
1. modify flow cache to be more generic (have virtual put and get
for each object; and remove the atomic_t pointer)
2. modify flow cache to have slow and fast resolvers so we can
copy with the current sleeping requirement
3. cache bundles instead of policies for outgoing stuff
4. kill find_bundle and just instantiate new ones if we get cache
miss
5. put all bundles to global hlist (since only place that walks
through them is gc, and stale bundle can be dst_free'd right
away); use genid's for policy to flush old bundles
6. dst_free and unlink bundle immediately if it's found to be stale
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: cache bundle lookup results in flow cache
From: Herbert Xu @ 2010-03-20 15:17 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, David S. Miller
In-Reply-To: <4BA349A8.9050105@iki.fi>
On Fri, Mar 19, 2010 at 11:53:44AM +0200, Timo Teräs wrote:
>
> But now, thinking more, it would probably make more sense to
> just cache xfrm_dst's and keep ref to the policy on them. So
> in general I agree with your recommendation. The only immediate
> problem I can think now is that the resolved would need to
> atomically check if xfrm_dst is valid, if not, resolve new one.
> But creating new xfrm_dst involves taking locks and can sleep
> so it cannot be inside the main resolver.
OK this brings out my favourite topic :)
The reason we have to sleep is to resolve the template. So if
we had queueing for pending xfrm_dst objects, we wouldn't have
to sleep at all when creating the top-level xfrm_dst.
Packets using that xfrm_dst can wait in the queue until it is
fully resolved. Now obviously this code doesn't exist so this
is all just a wet dream.
Setting my favourite topic aside, I have to come to the conclusion
that your patch still doesn't fully resolve the problem you set out
to fix.
The crux of the issue is the linked list of all bundles in a
policy and the obvious problems stemming from walking a linked
list that is unbounded.
The reason I think it doesn't fully resolve this is because of
the flow cache. Being a per-cpu cache, when you create the xfrm
dst the first time around, you'll at most put it in one CPU's
cache.
The next CPU that comes along will still have to walk that same
bundle linked list. So we're back to square one.
Now Dave, my impression is that we picked the per-cpu design
because it was the best data structure we had back in 2002,
right?
If so I'd like us to think about the possibility of switching
over to a different design, in particular, an RCU-based hash
table, similar to the one I just used for bridge multicasting.
This would eliminate the need for walking the bundle list apart
from the case when we're destroying the policy, which can be
done in process context.
Actually I just realised that the other way we can fix this is
to make xfrm_dst objects per-cpu just like IPv4 routes. That
is, when you fail to find an xfrm_dst object in the per-cpu
cache, you dont' bother calling xfrm_find_bundle but just make
a new bundle.
This is probably much easier than replacing the whole flow cache.
Can any one think of any problems with duplicate xfrm_dst objects?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: Handling receive packets with limited buffer space
From: Andi Kleen @ 2010-03-20 15:01 UTC (permalink / raw)
To: Amit Uttamchandani; +Cc: netdev
In-Reply-To: <20100319094118.GA30249@debian>
Amit Uttamchandani <amit.uttam@gmail.com> writes:
> In my modified ethoc.c driver, I am using only one receive buffer
> descriptor due to limited space. This works fairly well for small
> packets (arp, pings, etc.).
>
> However, doing a 'wget' and downloading a file causes lots of drops and
> thus results in a terminated connection.
>
> How do I go about dealing with this? I tried stopping the queue
> (netif_stop_queue) while I'm processing the rx skb and then resuming it
> once I'm done, but this did not seem to help.
>
> Any ideas on this?
You'll need to allocate a new skb and copy into it before
passing the data up the stack.
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH] ip_gre: include route header_len in max_headroom calculation
From: Herbert Xu @ 2010-03-20 15:00 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev
In-Reply-To: <4BA4E000.1030206@iki.fi>
On Sat, Mar 20, 2010 at 04:47:28PM +0200, Timo Teräs wrote:
>
> Actually, isn't the above right?
>
> max_headroom is calculated with LL_RESERVED_SPACE of the tdev, which
> is the interface to which the gre packet is being sent to, not the
> gre interface. Thus, max_headroom won't include gre devices
> previous needed_headroom.
Indeed you're right. Sorry for the confusion.
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] ip_gre: include route header_len in max_headroom calculation
From: Timo Teräs @ 2010-03-20 14:47 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <4BA4C2ED.9090701@iki.fi>
Timo Teräs wrote:
> Herbert Xu wrote:
>> On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote:
>>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>>> index f47c9f7..f78402d 100644
>>> --- a/net/ipv4/ip_gre.c
>>> +++ b/net/ipv4/ip_gre.c
>>> @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct
>>> sk_buff *skb, struct net_device *dev
>>> tunnel->err_count = 0;
>>> }
>>>
>>> - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen;
>>> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen +
>>> rt->u.dst.header_len;
>>>
>>> if (skb_headroom(skb) < max_headroom || skb_shared(skb)||
>>> (skb_cloned(skb) && !skb_clone_writable(skb, 0))) {
>>> struct sk_buff *new_skb = skb_realloc_headroom(skb,
>>> max_headroom);
>>> + if (max_headroom > dev->needed_headroom)
>>> + dev->needed_headroom = max_headroom;
>>
>> Are you sure about this? LL_RESERVED_SPACE already includes
>> dev->needed_headroom so won't this get bigger each time?
>
> Whoops. Must've been after-midnight issue. And not noticed since the
> max_headroom won't change many times.
>
> dev->needed_headroom should be compared against
> gre_hlen+rt->u.dst.header_len.
> I'll send a fixed patch in a jiffy.
Actually, isn't the above right?
max_headroom is calculated with LL_RESERVED_SPACE of the tdev, which
is the interface to which the gre packet is being sent to, not the
gre interface. Thus, max_headroom won't include gre devices
previous needed_headroom.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Timo Teräs @ 2010-03-20 13:34 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, Neil Horman
In-Reply-To: <20100320131340.GB2243@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:54:31PM +0200, Timo Teräs wrote:
>> Those are more or less timer based too. I guess it would be
>> better to put to dst core's function then. It can see hanging
>> refs, and call xfrm gc in that case.
>
> The IPv4 one appears to be invoked from dst_alloc just like
> xfrm.
Yes. But under normal circumstances that's not used. It also
has a separate timer based gc that goes through the buckets
periodically. That e.g. how the expired pmtu routes are kicked
out and how routes with old genid are purged before we start
to reach the gc_thresh limit. This happens in
rt_worker_func() which is called every ip_rt_gc_interval
(defaults to one minute).
>> To even minimize that it would help a lot if xfrm_bundle_ok
>> could release (or swap to dummies) the referenced dst->route
>> and dst->child entries. xfrm_bundle_ok is mostly called for
>> all bundles regularly by xfrm_find_bundle before new ones are
>> created.
>
> Yes I agree that's what we should do once your other patch is
> applied.
>
> So every top-level xfrm_dst that's not referenced by some user
> should sit in the flow cache. When it's needed and we find it
> to be stale we kick it out of the cache and free it along with
> the rest of its constituents.
Right. Ok, just to get road map of what I should do and in which
order and how.
xfrm gc:
- should dst core call it?
- should xfrm do it periodically?
- or do we rely that we get new bundles and let
xfrm[46]_find_bundle do the clean up?
- should xfrm_bundle_ok() release the inner dst's right away
instead of waiting any of the above to happen?
caching of bundles instead of policies for outgoing traffic:
- should flow cache be per-netns?
- since it will now have two types of objects, would it make
sense to have virtual put and get callbacks instead of
atomic_t pointer. this way qw can BUG_ON() if the refcount
goes to zero unexpectedly (or call the appropriate destructor)
and call the real _put function anway (e.g. dst_release does
WARN_ON stuff); the _get function can also do additional
checking if the object is valid or not; this way the flow
cache resolver output is always a valid object
- resolver to have "resolve_fast" and "resolve_slow". if fast
fails, the slow gets called with bh enabled so it can sleep,
since bundle creation needs that.
- it would probably be then useful to have dummy xfrm_dst for
policy reference when the policy forbids traffic?
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Neil Horman @ 2010-03-20 13:26 UTC (permalink / raw)
To: Herbert Xu; +Cc: Timo Teras, netdev
In-Reply-To: <20100320123247.GB1930@gondor.apana.org.au>
On Sat, Mar 20, 2010 at 08:32:48PM +0800, Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:15:41PM +0200, Timo Teras wrote:
> > The dst core calls garbage collection only from dst_alloc when
> > the dst entry threshold is exceeded. Xfrm core currently checks
> > bundles only on NETDEV_DOWN event.
> >
> > Previously this has not been a big problem since xfrm gc threshold
> > was small, and they were generated all the time due to another bug.
> >
> > Since after a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
> > ("xfrm: select sane defaults for xfrm[4|6] gc_thresh") we can have
> > large gc threshold sizes (>200000 on machines with normal amount
> > of memory) the garbage collection does not get triggered under
> > normal circumstances. This can result in enormous amount of stale
> > bundles. Further more, each of these stale bundles keep a reference
> > to ipv4/ipv6 rtable entries which are already gargage collected and
> > put to dst core "destroy free'd dst's" list. Now this list can grow
> > to be very large, and the dst core periodic job can bring even a fast
> > machine to it's knees.
>
> So why do we need this larger threshold in the first place? Neil?
My initial reasoning is here:
http://git.kernel.org/?p=linux/kernel/git/davem/net-next-2.6.git;a=commit;h=a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
I'd had a bug claiming that ipsec didn't scale to thousands of SA's, and the
reason turned out to be that xfrm code capped their entry threshold at 1024
entries:
https://bugzilla.redhat.com/show_bug.cgi?id=253053
I exported the gc thresholds via sysctls, and then used the above commit to
select sane values, reasoning that we should be able to support as many tunnels
as routes when possible.
If its too high, I have no qualm with lowering it, given that those that need it
can bump it up higher via the sysctl.
Regards
Neil
> --
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
>
>
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Herbert Xu @ 2010-03-20 13:13 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, Neil Horman
In-Reply-To: <4BA4C587.60504@iki.fi>
On Sat, Mar 20, 2010 at 02:54:31PM +0200, Timo Teräs wrote:
>
> Those are more or less timer based too. I guess it would be
> better to put to dst core's function then. It can see hanging
> refs, and call xfrm gc in that case.
The IPv4 one appears to be invoked from dst_alloc just like
xfrm.
> To even minimize that it would help a lot if xfrm_bundle_ok
> could release (or swap to dummies) the referenced dst->route
> and dst->child entries. xfrm_bundle_ok is mostly called for
> all bundles regularly by xfrm_find_bundle before new ones are
> created.
Yes I agree that's what we should do once your other patch is
applied.
So every top-level xfrm_dst that's not referenced by some user
should sit in the flow cache. When it's needed and we find it
to be stale we kick it out of the cache and free it along with
the rest of its constituents.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Timo Teräs @ 2010-03-20 12:54 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, Neil Horman
In-Reply-To: <20100320124919.GA2243@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:42:02PM +0200, Timo Teräs wrote:
>> But even if we have the gc_thresh back to 1024 or similar size,
>> it is still a good thing to do some basic gc on xfrm bundles so
>> that the underlaying rtable dst's can be freed before they end up
>> in the dst core list.
>
> But I'm not sure if a timer-based one really makes sense though.
>
> If you're worried about stale entries preventing IPv4/IPv6 rt
> entries from being collected, perhaps we should invoke the xfrm
> GC process from the IPv4/IPv6 GC function?
Those are more or less timer based too. I guess it would be
better to put to dst core's function then. It can see hanging
refs, and call xfrm gc in that case.
To even minimize that it would help a lot if xfrm_bundle_ok
could release (or swap to dummies) the referenced dst->route
and dst->child entries. xfrm_bundle_ok is mostly called for
all bundles regularly by xfrm_find_bundle before new ones are
created.
- Timo
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Herbert Xu @ 2010-03-20 12:49 UTC (permalink / raw)
To: Timo Teräs; +Cc: netdev, Neil Horman
In-Reply-To: <4BA4C29A.8000806@iki.fi>
On Sat, Mar 20, 2010 at 02:42:02PM +0200, Timo Teräs wrote:
>
> But even if we have the gc_thresh back to 1024 or similar size,
> it is still a good thing to do some basic gc on xfrm bundles so
> that the underlaying rtable dst's can be freed before they end up
> in the dst core list.
But I'm not sure if a timer-based one really makes sense though.
If you're worried about stale entries preventing IPv4/IPv6 rt
entries from being collected, perhaps we should invoke the xfrm
GC process from the IPv4/IPv6 GC function?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] ip_gre: include route header_len in max_headroom calculation
From: Timo Teräs @ 2010-03-20 12:43 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20100320123736.GC1930@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote:
>> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
>> index f47c9f7..f78402d 100644
>> --- a/net/ipv4/ip_gre.c
>> +++ b/net/ipv4/ip_gre.c
>> @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
>> tunnel->err_count = 0;
>> }
>>
>> - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen;
>> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len;
>>
>> if (skb_headroom(skb) < max_headroom || skb_shared(skb)||
>> (skb_cloned(skb) && !skb_clone_writable(skb, 0))) {
>> struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
>> + if (max_headroom > dev->needed_headroom)
>> + dev->needed_headroom = max_headroom;
>
> Are you sure about this? LL_RESERVED_SPACE already includes
> dev->needed_headroom so won't this get bigger each time?
Whoops. Must've been after-midnight issue. And not noticed since the
max_headroom won't change many times.
dev->needed_headroom should be compared against gre_hlen+rt->u.dst.header_len.
I'll send a fixed patch in a jiffy.
- timo
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Timo Teräs @ 2010-03-20 12:42 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev, Neil Horman
In-Reply-To: <20100320123247.GB1930@gondor.apana.org.au>
Herbert Xu wrote:
> On Sat, Mar 20, 2010 at 02:15:41PM +0200, Timo Teras wrote:
>> The dst core calls garbage collection only from dst_alloc when
>> the dst entry threshold is exceeded. Xfrm core currently checks
>> bundles only on NETDEV_DOWN event.
>>
>> Previously this has not been a big problem since xfrm gc threshold
>> was small, and they were generated all the time due to another bug.
>>
>> Since after a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
>> ("xfrm: select sane defaults for xfrm[4|6] gc_thresh") we can have
>> large gc threshold sizes (>200000 on machines with normal amount
>> of memory) the garbage collection does not get triggered under
>> normal circumstances. This can result in enormous amount of stale
>> bundles. Further more, each of these stale bundles keep a reference
>> to ipv4/ipv6 rtable entries which are already gargage collected and
>> put to dst core "destroy free'd dst's" list. Now this list can grow
>> to be very large, and the dst core periodic job can bring even a fast
>> machine to it's knees.
>
> So why do we need this larger threshold in the first place? Neil?
Actually it looks like that on ipv6 side the gc_thresh is something
more normal. On ipv4 side it's insanely big. The 1/2 ratio is not
what ipv4 rtable uses for it's own gc_thresh. Looks like it's using
1/16 ratio which yields much better value.
But even if we have the gc_thresh back to 1024 or similar size,
it is still a good thing to do some basic gc on xfrm bundles so
that the underlaying rtable dst's can be freed before they end up
in the dst core list.
- Timo
^ permalink raw reply
* Re: [PATCH] ip_gre: include route header_len in max_headroom calculation
From: Herbert Xu @ 2010-03-20 12:37 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev
In-Reply-To: <1269088078-7343-1-git-send-email-timo.teras@iki.fi>
On Sat, Mar 20, 2010 at 02:27:58PM +0200, Timo Teras wrote:
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index f47c9f7..f78402d 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
> tunnel->err_count = 0;
> }
>
> - max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen;
> + max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len;
>
> if (skb_headroom(skb) < max_headroom || skb_shared(skb)||
> (skb_cloned(skb) && !skb_clone_writable(skb, 0))) {
> struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
> + if (max_headroom > dev->needed_headroom)
> + dev->needed_headroom = max_headroom;
Are you sure about this? LL_RESERVED_SPACE already includes
dev->needed_headroom so won't this get bigger each time?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] xfrm: implement basic garbage collection for bundles
From: Herbert Xu @ 2010-03-20 12:32 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev, Neil Horman
In-Reply-To: <1269087341-7009-1-git-send-email-timo.teras@iki.fi>
On Sat, Mar 20, 2010 at 02:15:41PM +0200, Timo Teras wrote:
> The dst core calls garbage collection only from dst_alloc when
> the dst entry threshold is exceeded. Xfrm core currently checks
> bundles only on NETDEV_DOWN event.
>
> Previously this has not been a big problem since xfrm gc threshold
> was small, and they were generated all the time due to another bug.
>
> Since after a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
> ("xfrm: select sane defaults for xfrm[4|6] gc_thresh") we can have
> large gc threshold sizes (>200000 on machines with normal amount
> of memory) the garbage collection does not get triggered under
> normal circumstances. This can result in enormous amount of stale
> bundles. Further more, each of these stale bundles keep a reference
> to ipv4/ipv6 rtable entries which are already gargage collected and
> put to dst core "destroy free'd dst's" list. Now this list can grow
> to be very large, and the dst core periodic job can bring even a fast
> machine to it's knees.
So why do we need this larger threshold in the first place? Neil?
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] ip_gre: include route header_len in max_headroom calculation
From: Timo Teras @ 2010-03-20 12:27 UTC (permalink / raw)
To: netdev; +Cc: Timo Teras, Herbert Xu
Taking route's header_len into account, and updating gre device
needed_headroom will give better hints on upper bound of required
headroom. This is useful if the gre traffic is xfrm'ed.
Signed-off-by: Timo Teras <timo.teras@iki.fi>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
net/ipv4/ip_gre.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
This was earlier discussed in netdev thread:
http://marc.info/?t=124470870200004&r=1&w=2
I just never got to writing the patch until now.
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index f47c9f7..f78402d 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -810,11 +810,13 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
tunnel->err_count = 0;
}
- max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen;
+ max_headroom = LL_RESERVED_SPACE(tdev) + gre_hlen + rt->u.dst.header_len;
if (skb_headroom(skb) < max_headroom || skb_shared(skb)||
(skb_cloned(skb) && !skb_clone_writable(skb, 0))) {
struct sk_buff *new_skb = skb_realloc_headroom(skb, max_headroom);
+ if (max_headroom > dev->needed_headroom)
+ dev->needed_headroom = max_headroom;
if (!new_skb) {
ip_rt_put(rt);
txq->tx_dropped++;
--
1.6.3.3
^ permalink raw reply related
* [PATCH] xfrm: implement basic garbage collection for bundles
From: Timo Teras @ 2010-03-20 12:15 UTC (permalink / raw)
To: netdev; +Cc: Timo Teras, Neil Horman, Herbert Xu
The dst core calls garbage collection only from dst_alloc when
the dst entry threshold is exceeded. Xfrm core currently checks
bundles only on NETDEV_DOWN event.
Previously this has not been a big problem since xfrm gc threshold
was small, and they were generated all the time due to another bug.
Since after a33bc5c15154c835aae26f16e6a3a7d9ad4acb45
("xfrm: select sane defaults for xfrm[4|6] gc_thresh") we can have
large gc threshold sizes (>200000 on machines with normal amount
of memory) the garbage collection does not get triggered under
normal circumstances. This can result in enormous amount of stale
bundles. Further more, each of these stale bundles keep a reference
to ipv4/ipv6 rtable entries which are already gargage collected and
put to dst core "destroy free'd dst's" list. Now this list can grow
to be very large, and the dst core periodic job can bring even a fast
machine to it's knees.
Signed-off-by: Timo Teras <timo.teras@iki.fi>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
net/xfrm/xfrm_policy.c | 20 +++++++++++++++++++-
1 files changed, 19 insertions(+), 1 deletions(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 843e066..f3f3d36 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -44,6 +44,9 @@ static struct xfrm_policy_afinfo *xfrm_policy_afinfo[NPROTO];
static struct kmem_cache *xfrm_dst_cache __read_mostly;
+static int xfrm_gc_interval __read_mostly = 5 * 60 * HZ;
+static struct delayed_work expires_work;
+
static HLIST_HEAD(xfrm_policy_gc_list);
static DEFINE_SPINLOCK(xfrm_policy_gc_lock);
@@ -2203,6 +2206,16 @@ static int xfrm_flush_bundles(struct net *net)
return 0;
}
+static void xfrm_gc_worker_func(struct work_struct *work)
+{
+ struct net *net;
+
+ for_each_net(net)
+ xfrm_flush_bundles(net);
+
+ schedule_delayed_work(&expires_work, xfrm_gc_interval);
+}
+
static void xfrm_init_pmtu(struct dst_entry *dst)
{
do {
@@ -2498,8 +2511,13 @@ static int __net_init xfrm_policy_init(struct net *net)
INIT_LIST_HEAD(&net->xfrm.policy_all);
INIT_WORK(&net->xfrm.policy_hash_work, xfrm_hash_resize);
- if (net_eq(net, &init_net))
+ if (net_eq(net, &init_net)) {
register_netdevice_notifier(&xfrm_dev_notifier);
+
+ INIT_DELAYED_WORK_DEFERRABLE(&expires_work, xfrm_gc_worker_func);
+ schedule_delayed_work(&expires_work,
+ net_random() % xfrm_gc_interval + xfrm_gc_interval);
+ }
return 0;
out_bydst:
--
1.6.3.3
^ permalink raw reply related
* Re: lartc is dead?
From: ctxspi @ 2010-03-20 12:04 UTC (permalink / raw)
To: netdev; +Cc: ahu
Hi, my name is Marco.
I want know how I can have help for small question about bug (nexthdr in u32).
Can you give me a small help?
Thanks
^ permalink raw reply
* Re: [patch] bridge: cleanup: remove unused assignment
From: Herbert Xu @ 2010-03-20 11:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: David S. Miller, Stephen Hemminger, YOSHIFUJI Hideaki,
Paul E. McKenney, Michael Braun, bridge, netdev, kernel-janitors
In-Reply-To: <20100320112049.GU5331@bicker>
On Sat, Mar 20, 2010 at 02:20:49PM +0300, Dan Carpenter wrote:
> We never actually use iph again so this assignment can be removed.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ 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