* Re: [PATCH ver2] Avoid enqueuing skb for default qdiscs
From: Maxime Bizon @ 2010-02-01 14:16 UTC (permalink / raw)
To: Krishna Kumar2; +Cc: davem, herbert, Jarek Poplawski, kaber, netdev
In-Reply-To: <OF6B2AD304.3AD05621-ON652576BD.0047556A-652576BD.004B1F23@in.ibm.com>
On Mon, 2010-02-01 at 19:12 +0530, Krishna Kumar2 wrote:
Hi,
> sch_direct_xmit can be called from dev_queue_xmit for a
> stopped device only if the device had xmit the previous
> skb, stopped the device and returned OK. Then the next
Yes, that's what happen in my case.
> BTW, I don't think this patch would change the earlier
> behavior. The old code would have done the same thing.
Oh, my mistake then. I always thought the requeue counter was indicating
the driver returned TX_BUSY (and failed to stop the queue before), which
is not polite.
If the current behavior is the expected one, then there is no problem
here.
> As I explained, this should happen only once per stop event.
> Could you tell which driver is having this problem? Is it
> waking up too early, eg, it might be stopping when the hw tx
> descriptor is full but waking up when a few slots open up,
> and those will get filled up immediately on fast systems.
> Then you will see a lot of requeue's.
This is an atm driver which is not mainlined. The system is not that
fast, but the link is (very) slow (< 1 Mbit/s).
The driver is waking up the queue once the hardware queue is not full
anymore (at least one free tx desc), and the free slot gets filled
immediately since the link is slow.
Thanks for explaining !
--
Maxime
^ permalink raw reply
* Re: [PATCH] af_key: fix netns ops ordering on module load/unload
From: Eric Dumazet @ 2010-02-01 13:56 UTC (permalink / raw)
To: Luca Tettamanti; +Cc: Alexey Dobriyan, linux-kernel, netdev, David Miller
In-Reply-To: <68676e01002010550l2654208dged864973a996c381@mail.gmail.com>
Le lundi 01 février 2010 à 14:50 +0100, Luca Tettamanti a écrit :
> On Sat, Jan 30, 2010 at 1:53 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > [PATCH] af_key: fix netns ops ordering on module load/unload
> >
> > 1. After sock_register() returns, it's possible to create sockets,
> > even if module still not initialized fully (blame generic module code
> > for that!)
> > 2. Consequently, pfkey_create() can be called with pfkey_net_id still not
> > initialized which will BUG_ON in net_generic():
> > kernel BUG at include/net/netns/generic.h:43!
> > 3. During netns shutdown, netns ops should be unregistered after
> > key manager unregistered because key manager calls can be triggered
> > from xfrm_user module:
> >
> > general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> > pfkey_broadcast+0x111/0x210 [af_key]
> > pfkey_send_notify+0x16a/0x300 [af_key]
> > km_state_notify+0x41/0x70
> > xfrm_flush_sa+0x75/0x90 [xfrm_user]
> > 4. Unregister netns ops after socket ops just in case and for symmetry.
> >
> > Reported by Luca Tettamanti.
> >
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>
> Tested-by: Luca Tettamanti <kronos.it@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
^ permalink raw reply
* Re: [PATCH] af_key: fix netns ops ordering on module load/unload
From: Luca Tettamanti @ 2010-02-01 13:50 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: Eric Dumazet, linux-kernel, netdev, David Miller
In-Reply-To: <20100130125327.GA4258@x200>
On Sat, Jan 30, 2010 at 1:53 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Fri, Jan 29, 2010 at 05:33:26PM +0100, Eric Dumazet wrote:
>> @@ -3807,21 +3807,24 @@ static int __init ipsec_pfkey_init(void)
>> if (err != 0)
>> goto out;
>>
>> - err = sock_register(&pfkey_family_ops);
>> - if (err != 0)
>> - goto out_unregister_key_proto;
>> err = xfrm_register_km(&pfkeyv2_mgr);
>> if (err != 0)
>> - goto out_sock_unregister;
>> + goto out_unregister_key_proto;
>> +
>> err = register_pernet_subsys(&pfkey_net_ops);
>> if (err != 0)
>> goto out_xfrm_unregister_km;
>> +
>> + err = sock_register(&pfkey_family_ops);
>> + if (err != 0)
>> + goto out_unregister_pernet;
>> out:
>> return err;
>> +
>> +out_unregister_pernet:
>> + unregister_pernet_subsys(&pfkey_net_ops);
>> out_xfrm_unregister_km:
>> xfrm_unregister_km(&pfkeyv2_mgr);
>> -out_sock_unregister:
>> - sock_unregister(PF_KEY);
>> out_unregister_key_proto:
>> proto_unregister(&key_proto);
>> goto out;
>
> ACK analysis, except this is not enough.
>
> Here is patch which survived netns start/stop/modprobe/rmmod cycles.
>
> Alexey, who still doesn't get why bug reproduces so easily for bug reporter.
>
> Luca, please confirm.
Seems to work fine.
> [PATCH] af_key: fix netns ops ordering on module load/unload
>
> 1. After sock_register() returns, it's possible to create sockets,
> even if module still not initialized fully (blame generic module code
> for that!)
> 2. Consequently, pfkey_create() can be called with pfkey_net_id still not
> initialized which will BUG_ON in net_generic():
> kernel BUG at include/net/netns/generic.h:43!
> 3. During netns shutdown, netns ops should be unregistered after
> key manager unregistered because key manager calls can be triggered
> from xfrm_user module:
>
> general protection fault: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
> pfkey_broadcast+0x111/0x210 [af_key]
> pfkey_send_notify+0x16a/0x300 [af_key]
> km_state_notify+0x41/0x70
> xfrm_flush_sa+0x75/0x90 [xfrm_user]
> 4. Unregister netns ops after socket ops just in case and for symmetry.
>
> Reported by Luca Tettamanti.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Tested-by: Luca Tettamanti <kronos.it@gmail.com>
thanks,
Luca
^ permalink raw reply
* Re: [PATCH ver2] Avoid enqueuing skb for default qdiscs
From: Krishna Kumar2 @ 2010-02-01 13:42 UTC (permalink / raw)
To: mbizon; +Cc: davem, herbert, Jarek Poplawski, kaber, netdev
In-Reply-To: <1265017940.32192.23.camel@sakura.staff.proxad.net>
Hi Maxime,
Maxime Bizon <mbizon@freebox.fr> wrote on 02/01/2010 03:22:20 PM:
> Sorry old stuff, but I just noticed this.
>
> One side effect of this patch is that tc now shows a lot of requeue for
> slow interfaces (slow dsl link in my case), because sch_direct_xmit is
> called even if device stopped its queue.
sch_direct_xmit can be called from dev_queue_xmit for a
stopped device only if the device had xmit the previous
skb, stopped the device and returned OK. Then the next
sch_d_x will dequeue and requeue the new skb. But from
now on, sch_d_x is never called from dev_queue_xmit
since qdisc_qlen(q) > 0. Instead dev_queue_xmit enqueue's
the skb and calls qdisc_restart, which calls dequeue_skb.
dequeue_skb finds there is an earlier gso_skb but since
the device is stopped, it returns NULL. Hence there
should be only one requeue (and dequeue) for every stop.
BTW, I don't think this patch would change the earlier
behavior. The old code would have done the same thing.
> My first reflex was too investigate for a misbehaving driver (returning
> TX_BUSY), but start_xmit is not even called, sch_direct_xmit notices
> that queue is stopped, and just does a dequeue/requeue.
As I explained, this should happen only once per stop event.
Could you tell which driver is having this problem? Is it
waking up too early, eg, it might be stopping when the hw tx
descriptor is full but waking up when a few slots open up,
and those will get filled up immediately on fast systems.
Then you will see a lot of requeue's.
> Since we're talking about slow interfaces, this has no impact on
> performance, the requeue counter incrementing is just a bit scary.
>
> Shouldn't we check for stopped queue earlier to avoid this ?
dev_queue_xmit is checking for qdisc_qlen == 0 before calling
sch_direct_xmit, which is enough for all stopped queue except
the first time. Also, it's not possible to check for stopped
Q earlier in qdisc_restart() due to multiqueue implementation.
Please let me know if you find something different from what
I have described.
thanks,
- KK
^ permalink raw reply
* Re: [PATCH 0/3] vbus: venet macvlan rt netlink changes
From: Gregory Haskins @ 2010-02-01 12:53 UTC (permalink / raw)
To: Patrick Mullaney; +Cc: alacrityvm-devel, linux-kernel, kaber, arnd, netdev
In-Reply-To: <20100114215058.4958.87746.stgit@mimic.site>
[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]
On 1/14/10 4:52 PM, Patrick Mullaney wrote:
> (Applies to alacrityvm.git/master:a725f9950)
>
> This series changes the venet macvlan device such that it can
> be created via rt netlink and no longer requires any extensive
> changes to the macvlan device itself. The first patch removes
> prior changes to the macvlan device introduced in the alacrityvm
> tree and also implements the changes needed for creation and
> management via rt netlink. The subsequent patches(2 and 3) are
> changes to the macvlan device for GRO support and for type
> specific release. These 2 patches don't appear to be upstream.
> If desired and approved, I'll regenerate these last 2 for
> net-next.
Applied to alacrityvm.git/master. Thanks Pat!
>
> ---
>
> Patrick Mullaney (3):
> macvlan: use rtnl_link_ops->dellink during unregister notications
> macvlan: add GRO bit to features mask
> venet-macvlan: allow rt netlink to create venet macvlan devices
>
>
> drivers/net/macvlan.c | 72 ++----
> include/linux/macvlan.h | 6 -
> kernel/vbus/devices/venet/device.c | 8 +
> kernel/vbus/devices/venet/macvlan.c | 360 ++++++++++++++++++++-----------
> kernel/vbus/devices/venet/tap.c | 3
> kernel/vbus/devices/venet/venetdevice.h | 3
> 6 files changed, 265 insertions(+), 187 deletions(-)
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* Re: [PATCH 02/86] dccp: revert buggy auto-loading of dccp module
From: Neil Horman @ 2010-02-01 12:21 UTC (permalink / raw)
To: Gerrit Renker; +Cc: davem, dccp, netdev
In-Reply-To: <1265005087-5549-3-git-send-email-gerrit@erg.abdn.ac.uk>
On Mon, Feb 01, 2010 at 07:18:07AM +0100, Gerrit Renker wrote:
> This reverts commit (38ff3e6bb987ec583268da8eb22628293095d43b) ("dccp_probe:
> Fix module load dependencies between dccp and dccp_probe", from 15 Jan). Not
> only does it not work:
>
> % modprobe -v dccp_probe
> kernel: [ 1431.442912] sys_init_module: 'dccp_probe'->init suspiciously \
> returned 1, it should follow 0/-E convention
> kernel: [ 1431.442915] sys_init_module: loading module anyway...
>
>
> ... but it also causes a crash:
>
> % rmmod dccp_probe
> kernel: [ 1777.305846] kernel BUG at /usr/src/davem-2.6/mm/slab.c:521!
> kernel: [ 1777.305852] invalid opcode: 0000 [#1] SMP
> kernel: [ 1777.305861] last sysfs file: /sys/class/power_supply/BAT0/energy_full
> kernel: [ 1777.305867] Modules linked in: dccp_probe(-) iwl3945 iwlcore [last unloaded: dccp]
> kernel: [ 1777.305883]
> kernel: [ 1777.305891] Pid: 12912, comm: rmmod Tainted: G R 2.6.33-rc5 #6 2008URG/2008URG
> kernel: [ 1777.305899] EIP: 0060:[<c01d5e43>] EFLAGS: 00010046 CPU: 1
> kernel: [ 1777.305910] EIP is at kfree+0x73/0x150
> kernel: [ 1777.305916] EAX: c1678c00 EBX: 00000000 ECX: c01d5e15 EDX: 40080000
> kernel: [ 1777.305922] ESI: c015cb9a EDI: 080488a0 EBP: f4ffbf34 ESP: f4ffbf10
> kernel: [ 1777.305929] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> kernel: [ 1777.305936] Process rmmod (pid: 12912, ti=f4ffb000 task=f61e8620 task.ti=f4ffb000)
>
> ==> After reverting the commit:
>
> % modprobe -v dccp_probe
> insmod /lib/modules/2.6.33-rc5/kernel/net/dccp/dccp.ko
> insmod /lib/modules/2.6.33-rc5/kernel/net/dccp/dccp_probe.ko
>
> % lsmod
> Module Size Used by
> dccp_probe 2345 0
> dccp 120233 1 dccp_probe
>
> Previously (during about 4 years of this module's history) there had never
> been a problem with the 'silent dependency' that the commit tried to fix:
> this dependency is deliberate and required, since dccp_probe performs probing
> of dccp connections and hence needs to know about dccp internals.
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
This doesn't make any sense. Gerrit, you don't understand what the patch was
trying to do. There is a silent dependency, in that this module requires the
dccp module to be loaded, but the reference to the dccp_send_probe symbol isn't
one that depmod can see. If you don't load dccp first, dccp_probe fails, why
bother to allow that when try_then_request_module can avoid it?
The problem here is the construction of the first argument,
try_then_request_module should only return valid return codes from the first
argument, and my first argument is malformed. register_jprobe returns zero on
success, so I need to check its return in the call for 0, in case we need to
trigger the request_module action, but in so doing ret gets the value of
(register_jprobe(&dccp_send_probe) == 0), which will always be 0 or 1. What we
actually need to do is assign the result of register_jprobe to ret, without the
side effect of the comparison. I've not tested it, but this should do it,
without re-breaking the silent dependency.
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
probe.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/dccp/probe.c b/net/dccp/probe.c
index bace1d8..a8f5fdf 100644
--- a/net/dccp/probe.c
+++ b/net/dccp/probe.c
@@ -161,7 +161,8 @@ static __init int dccpprobe_init(void)
if (!proc_net_fops_create(&init_net, procname, S_IRUSR, &dccpprobe_fops))
goto err0;
- ret = try_then_request_module((register_jprobe(&dccp_send_probe) == 0),
+ try_then_request_module(
+ ((ret = register_jprobe(&dccp_send_probe)) == 0),
"dccp");
if (ret)
goto err1;
^ permalink raw reply related
* Re: [PATCH 01/86] dccp: fix bug in cache allocation
From: Gerrit Renker @ 2010-02-01 12:08 UTC (permalink / raw)
To: Neil Horman; +Cc: davem, dccp, netdev
In-Reply-To: <20100201115638.GB25094@hmsreliant.think-freely.org>
| These changes make sense, sorry for not seeing that earlier. Thanks!
| Acked-by: Neil Horman <nhorman@tuxdriver.com>
Thanks a lot for the quick response, I was just in the middle of composing
a message to re-send the patches (the count '0{1,2}/86' was wrong).
Apologies for forgetting to CC:, thanks for catching this.
^ permalink raw reply
* [Bug-Fix][PATCH 2/2] dccp: revert buggy auto-loading of dccp module
From: Gerrit Renker @ 2010-02-01 12:12 UTC (permalink / raw)
To: davem; +Cc: nhorman, dccp, netdev, --cc, Gerrit Renker
In-Reply-To: <1265026340-5282-2-git-send-email-gerrit@erg.abdn.ac.uk>
This reverts commit (38ff3e6bb987ec583268da8eb22628293095d43b) ("dccp_probe:
Fix module load dependencies between dccp and dccp_probe", from 15 Jan). Not
only does it not work:
% modprobe -v dccp_probe
kernel: [ 1431.442912] sys_init_module: 'dccp_probe'->init suspiciously \
returned 1, it should follow 0/-E convention
kernel: [ 1431.442915] sys_init_module: loading module anyway...
... but it also causes a crash:
% rmmod dccp_probe
kernel: [ 1777.305846] kernel BUG at /usr/src/davem-2.6/mm/slab.c:521!
kernel: [ 1777.305852] invalid opcode: 0000 [#1] SMP
kernel: [ 1777.305861] last sysfs file: /sys/class/power_supply/BAT0/energy_full
kernel: [ 1777.305867] Modules linked in: dccp_probe(-) iwl3945 iwlcore [last unloaded: dccp]
kernel: [ 1777.305883]
kernel: [ 1777.305891] Pid: 12912, comm: rmmod Tainted: G R 2.6.33-rc5 #6 2008URG/2008URG
kernel: [ 1777.305899] EIP: 0060:[<c01d5e43>] EFLAGS: 00010046 CPU: 1
kernel: [ 1777.305910] EIP is at kfree+0x73/0x150
kernel: [ 1777.305916] EAX: c1678c00 EBX: 00000000 ECX: c01d5e15 EDX: 40080000
kernel: [ 1777.305922] ESI: c015cb9a EDI: 080488a0 EBP: f4ffbf34 ESP: f4ffbf10
kernel: [ 1777.305929] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
kernel: [ 1777.305936] Process rmmod (pid: 12912, ti=f4ffb000 task=f61e8620 task.ti=f4ffb000)
==> After reverting the commit:
% modprobe -v dccp_probe
insmod /lib/modules/2.6.33-rc5/kernel/net/dccp/dccp.ko
insmod /lib/modules/2.6.33-rc5/kernel/net/dccp/dccp_probe.ko
% lsmod
Module Size Used by
dccp_probe 2345 0
dccp 120233 1 dccp_probe
Previously (during about 4 years of this module's history) there had never
been a problem with the 'silent dependency' that the commit tried to fix:
this dependency is deliberate and required, since dccp_probe performs probing
of dccp connections and hence needs to know about dccp internals.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
net/dccp/probe.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
--- a/net/dccp/probe.c
+++ b/net/dccp/probe.c
@@ -161,8 +161,7 @@ static __init int dccpprobe_init(void)
if (!proc_net_fops_create(&init_net, procname, S_IRUSR, &dccpprobe_fops))
goto err0;
- ret = try_then_request_module((register_jprobe(&dccp_send_probe) == 0),
- "dccp");
+ ret = register_jprobe(&dccp_send_probe);
if (ret)
goto err1;
^ permalink raw reply
* [Bug-Fix][PATCH 1/2] dccp: fix bug in cache allocation
From: Gerrit Renker @ 2010-02-01 12:12 UTC (permalink / raw)
To: davem; +Cc: nhorman, dccp, netdev, --cc, Gerrit Renker
In-Reply-To: <1265026340-5282-1-git-send-email-gerrit@erg.abdn.ac.uk>
This fixes a bug introduced in commit de4ef86cfce60d2250111f34f8a084e769f23b16
("dccp: fix dccp rmmod when kernel configured to use slub", 17 Jan): the
vsnprintf used sizeof(slab_name_fmt), which became truncated to 4 bytes, since
slab_name_fmt is now a 4-byte pointer and no longer a 32-character array.
This lead to error messages such as
FATAL: Error inserting dccp: No buffer space available
>> kernel: [ 1456.341501] kmem_cache_create: duplicate cache cci
generated due to the truncation after the 3rd character.
Fixed for the moment by introducing a symbolic constant. Tested to fix the bug.
Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
---
net/dccp/ccid.c | 2 +-
net/dccp/ccid.h | 8 +++++---
2 files changed, 6 insertions(+), 4 deletions(-)
--- a/net/dccp/ccid.c
+++ b/net/dccp/ccid.c
@@ -83,7 +83,7 @@ static struct kmem_cache *ccid_kmem_cache_create(int obj_size, char *slab_name_f
va_list args;
va_start(args, fmt);
- vsnprintf(slab_name_fmt, sizeof(slab_name_fmt), fmt, args);
+ vsnprintf(slab_name_fmt, CCID_SLAB_NAME_LENGTH, fmt, args);
va_end(args);
slab = kmem_cache_create(slab_name_fmt, sizeof(struct ccid) + obj_size, 0,
--- a/net/dccp/ccid.h
+++ b/net/dccp/ccid.h
@@ -19,7 +19,9 @@
#include <linux/list.h>
#include <linux/module.h>
-#define CCID_MAX 255
+/* maximum value for a CCID (RFC 4340, 19.5) */
+#define CCID_MAX 255
+#define CCID_SLAB_NAME_LENGTH 32
struct tcp_info;
@@ -49,8 +51,8 @@ struct ccid_operations {
const char *ccid_name;
struct kmem_cache *ccid_hc_rx_slab,
*ccid_hc_tx_slab;
- char ccid_hc_rx_slab_name[32];
- char ccid_hc_tx_slab_name[32];
+ char ccid_hc_rx_slab_name[CCID_SLAB_NAME_LENGTH];
+ char ccid_hc_tx_slab_name[CCID_SLAB_NAME_LENGTH];
__u32 ccid_hc_rx_obj_size,
ccid_hc_tx_obj_size;
/* Interface Routines */
^ permalink raw reply
* [BUG-FIX v2][PATCH 0/2]: recent dccp fixes
From: Gerrit Renker @ 2010-02-01 12:12 UTC (permalink / raw)
To: davem; +Cc: nhorman, dccp, netdev, --cc
In-Reply-To: <20100201120815.GA4424@gerrit.erg.abdn.ac.uk>
Hi Dave,
as per your request, I am resending this CC:-ed to Neil (who already acked
the first patch), and also adding Arnaldo.
These are two fixes for recent dccp commits which addressed issues, but
introduced bugs which I discovered when rebasing the test tree last week.
Patch #1: fixes a bug resulting from truncating cache name length.
Patch #2: reverts a commit which introduced a module-loading bug.
This being bugs, would you please consider this for stable too.
The test-tree also has been fixed accordingly,
http://eden-feed.erg.abdn.ac.uk/cgi-bin/gitweb.cgi?p=dccp_exp.git;a=shortlog;h=ready
^ permalink raw reply
* Re: [PATCH 01/86] dccp: fix bug in cache allocation
From: Neil Horman @ 2010-02-01 11:56 UTC (permalink / raw)
To: Gerrit Renker; +Cc: davem, dccp, netdev
In-Reply-To: <1265005087-5549-2-git-send-email-gerrit@erg.abdn.ac.uk>
On Mon, Feb 01, 2010 at 07:18:06AM +0100, Gerrit Renker wrote:
> This fixes a bug introduced in commit de4ef86cfce60d2250111f34f8a084e769f23b16
> ("dccp: fix dccp rmmod when kernel configured to use slub", 17 Jan): the
> vsnprintf used sizeof(slab_name_fmt), which became truncated to 4 bytes, since
> slab_name_fmt is now a 4-byte pointer and no longer a 32-character array.
>
> This lead to error messages such as
> FATAL: Error inserting dccp: No buffer space available
>
> >> kernel: [ 1456.341501] kmem_cache_create: duplicate cache cci
> generated due to the truncation after the 3rrdc character.
>
> Fixed for the moment by introducing a symbolic constant. Tested to fix the bug.
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
These changes make sense, sorry for not seeing that earlier. Thanks!
Acked-by: Neil Horman <nhorman@tuxdriver.com>
>
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Eric Dumazet @ 2010-02-01 11:23 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Jon Masters, linux-kernel, netdev, netfilter-devel,
Patrick McHardy, Paul E. McKenney
In-Reply-To: <b6fcc0a1002010225u4e74f9f0q633d73038234dc37@mail.gmail.com>
Le lundi 01 février 2010 à 12:25 +0200, Alexey Dobriyan a écrit :
> > 2) nf_conntrack_cachep is shared, it should be not shared.
>
> There is no need for it to be shared, unless you measured something.
>
I wrote the algos, I know that we need different slab caches, for sure,
this is not something I can _measure_, but theory can predict.
SLAB_DESTROY_BY_RCU has very special semantics, you can ask Paul E.
McKenny for details if you dont trust me.
If you use a shared slab cache, one object can instantly flight between
one hash table (netns ONE) to another one (netns TWO), and concurrent
reader (doing a lookup in netns ONE, 'finding' an object of netns TWO)
can be fooled without notice, because no RCU grace period has to be
observed between object freeing and its reuse.
We dont have this problem with UDP/TCP slab caches because TCP/UDP
hashtables are global to the machine (and each object has a pointer to
its netns).
If we use per netns conntrack hash tables, we also *must* use per netns
conntrack slab caches, to guarantee an object can not escape from one
namespace to another one.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Jon Masters @ 2010-02-01 10:53 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010249y19e2edc3g7974f44633859bd3@mail.gmail.com>
On Mon, 2010-02-01 at 12:49 +0200, Alexey Dobriyan wrote:
> On Mon, Feb 1, 2010 at 12:47 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Or even printk nf_conntrack_htable_size during bootup and panic/oops.
>
> Oh, sorry, I see it was indeed corrupted.
Right. And it's not just the set size function that changes the value
(it wasn't in this case) so it's not sufficient to say that there is a
generic function to set the size. That only applies to the hashtable in
the global init_net anyway, changing the hashtable sizes and then
leaving the existing hashes as they were.
Additionally, the size is writeable in the module sysfs entry. So it's
not read only, even though I agree that the procfs visable entry is. I
really think this does need fixing, and I'm not just being a pain in the
ass - I have another issue here clearly with memory corruption, but the
need for this to be cleaned up is real.
Jon.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Alexey Dobriyan @ 2010-02-01 10:49 UTC (permalink / raw)
To: Jon Masters
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010247s70b855b6kea66c99c9326b6a@mail.gmail.com>
On Mon, Feb 1, 2010 at 12:47 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Or even printk nf_conntrack_htable_size during bootup and panic/oops.
Oh, sorry, I see it was indeed corrupted.
^ permalink raw reply
* Re: [PATCH] sky2: receive dma mapping error handling
From: Jarek Poplawski @ 2010-02-01 10:47 UTC (permalink / raw)
To: Michael Breuer
Cc: Stephen Hemminger, David Miller, akpm, flyboy, linux-kernel,
netdev, Michael Chan, Don Fry, Francois Romieu, Matt Carlson
In-Reply-To: <4B6657DB.3010008@majjas.com>
On Sun, Jan 31, 2010 at 11:26:03PM -0500, Michael Breuer wrote:
> FYI - tried generating lots of extra tx traffic... found a way to
> generate the rx status messages on demand:
> ping -i .0000001 -s 8000 -t 2 <host> >/dev/null
>
> Yields:
> Jan 31 23:08:07 mail kernel: sky2 eth0: rx error, status 0x1f6a0010
> length 1518
...
> Jan 31 23:08:12 mail kernel: net_ratelimit: 316 callbacks suppressed
> etc.
...
> Understanding that the other side is out of spec, I'd still wonder why
> the sky2 driver generates rx errors. Perhaps overruns should be tossed
> silently... by the hardware if possible.
Of course it's a matter of taste, but it seems such errors shouldn't
be tolerated in a local network. I'd rather prefer doing them more
explicit (like e.g. some other kind of length errors).
Jarek P.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Alexey Dobriyan @ 2010-02-01 10:47 UTC (permalink / raw)
To: Jon Masters
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010244j161f7f0bob46053efc0d65630@mail.gmail.com>
On Mon, Feb 1, 2010 at 12:44 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> So, insert printk into nf_conntrack_set_hashsize().
Or even printk nf_conntrack_htable_size during bootup and panic/oops.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Alexey Dobriyan @ 2010-02-01 10:44 UTC (permalink / raw)
To: Jon Masters
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <1265020552.7499.147.camel@tonnant>
On Mon, Feb 1, 2010 at 12:35 PM, Jon Masters <jonathan@jonmasters.org> wrote:
> On Mon, 2010-02-01 at 11:36 +0200, Alexey Dobriyan wrote:
>> On Mon, Feb 1, 2010 at 11:32 AM, Jon Masters <jonathan@jonmasters.org> wrote:
>> > I hacked up a per-namespace version of hashtables (this needs doing
>> > anyway, since the global stuff is just waiting to break)
>>
>> Which ones? Conntrack hashtables are per-netns.
>
> They are, but the metadata is not. Sorry for not being clear, but my
> previous mail was. i.e. there is a per-netns hashtable that is indexed
> using a global that might change at any time underneath. The htable size
> and max should be per-netns too.
So, insert printk into nf_conntrack_set_hashsize().
> An existing sysctl/module parameter affects these and should also
> ultimately either iterate through namespaces, or only affect the global
> init_net (as it almost does now, except it changes the data used by the
> others and doesn't resize them).
Those sysctls are readonly, nobody else changes hashtable size legitimately.
Alexey, hoping sysfs code for modules is not too smart.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Jon Masters @ 2010-02-01 10:38 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010225u4e74f9f0q633d73038234dc37@mail.gmail.com>
On Mon, 2010-02-01 at 12:25 +0200, Alexey Dobriyan wrote:
> On Mon, Feb 1, 2010 at 12:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Le lundi 01 février 2010 à 11:36 +0200, Alexey Dobriyan a écrit :
> >> On Mon, Feb 1, 2010 at 11:32 AM, Jon Masters <jonathan@jonmasters.org> wrote:
> >> > I hacked up a per-namespace version of hashtables (this needs doing
> >> > anyway, since the global stuff is just waiting to break)
> >>
> >> Which ones? Conntrack hashtables are per-netns.
> >
> > It seems they are, but this is not a complete work :
That's my point.
> They are per-netns.
>
> It's not "complete", because right now there is no point in doing more.
> nf_conntrack_max was rejected given the absense of per-netns kernel
> memory consumption limiting.
>
> > 1) Global settings (shared by all netns)
>
> Only hashtable size which is module parameter and
> there is no generic way to limit kernel memory (like beancounters).
And can be changed at any time you like (also an exported symbol) such
that existing hashtable indexing will fail and corrupt memory. There is
clearly a need for each of these hashtables to have its own metadata.
Jon.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Jon Masters @ 2010-02-01 10:35 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Eric Dumazet, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010136k7e78a998p31a9e7464c2e8d44@mail.gmail.com>
On Mon, 2010-02-01 at 11:36 +0200, Alexey Dobriyan wrote:
> On Mon, Feb 1, 2010 at 11:32 AM, Jon Masters <jonathan@jonmasters.org> wrote:
> > I hacked up a per-namespace version of hashtables (this needs doing
> > anyway, since the global stuff is just waiting to break)
>
> Which ones? Conntrack hashtables are per-netns.
They are, but the metadata is not. Sorry for not being clear, but my
previous mail was. i.e. there is a per-netns hashtable that is indexed
using a global that might change at any time underneath. The htable size
and max should be per-netns too.
An existing sysctl/module parameter affects these and should also
ultimately either iterate through namespaces, or only affect the global
init_net (as it almost does now, except it changes the data used by the
others and doesn't resize them).
Jon.
^ permalink raw reply
* Re: debug: nt_conntrack and KVM crash
From: Alexey Dobriyan @ 2010-02-01 10:25 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jon Masters, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <1265019160.2848.14.camel@edumazet-laptop>
On Mon, Feb 1, 2010 at 12:12 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 01 février 2010 à 11:36 +0200, Alexey Dobriyan a écrit :
>> On Mon, Feb 1, 2010 at 11:32 AM, Jon Masters <jonathan@jonmasters.org> wrote:
>> > I hacked up a per-namespace version of hashtables (this needs doing
>> > anyway, since the global stuff is just waiting to break)
>>
>> Which ones? Conntrack hashtables are per-netns.
>
> It seems they are, but this is not a complete work :
They are per-netns.
It's not "complete", because right now there is no point in doing more.
nf_conntrack_max was rejected given the absense of per-netns kernel
memory consumption limiting.
> 1) Global settings (shared by all netns)
Only hashtable size which is module parameter and
there is no generic way to limit kernel memory (like beancounters).
> 2) nf_conntrack_cachep is shared, it should be not shared.
There is no need for it to be shared, unless you measured something.
> 3) nf_conntrack_untracked shared by all netns, it should be local.
>
> nf_conntrack_cleanup_net() can block forever because of this.
>
> while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
> schedule();
This is known, yes, thinking on it, as naive way was agreed to suck.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Patch v2] net: export TCP send buffer size via netlink
From: Cong Wang @ 2010-02-01 10:21 UTC (permalink / raw)
To: David Miller; +Cc: linux-kernel, netdev, eric.dumazet
In-Reply-To: <20100201.020622.91421099.davem@davemloft.net>
David Miller wrote:
> Before you spend the next couple hours trying to figure out what to
> do...
>
> We already provide the information you're trying to export via
> linux/inet_diag.h:inet_diag_meminfo
>
> I hope the light goes on now :-)
>
Oh, I was just looking for a pointer.
Ok, ->idiag_wmem is gotten from sk->sk_wmem_queued, so ->sk_wmem_queued
is the TCP send buffer size that we want? I thought it is ->sk_sndbuf...
Thanks for your help!
^ permalink raw reply
* [PATCH net-next] sky2: Fix TX_MAP_PAGE misspelling
From: Jarek Poplawski @ 2010-02-01 10:19 UTC (permalink / raw)
To: David Miller; +Cc: Michael Breuer, Stephen Hemminger, netdev
Btw of the dma-debug problem reported by Michael Breuer I spotted
a tiny misspelling in TX_MAP_PAGE definition introduced by commit
6b84dacadbdc3.
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---
drivers/net/sky2.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/sky2.h b/drivers/net/sky2.h
index 365d79c..54cb303 100644
--- a/drivers/net/sky2.h
+++ b/drivers/net/sky2.h
@@ -2156,7 +2156,7 @@ struct tx_ring_info {
struct sk_buff *skb;
unsigned long flags;
#define TX_MAP_SINGLE 0x0001
-#define TX_MAP_PAGE 000002
+#define TX_MAP_PAGE 0x0002
DECLARE_PCI_UNMAP_ADDR(mapaddr);
DECLARE_PCI_UNMAP_LEN(maplen);
};
^ permalink raw reply related
* Re: debug: nt_conntrack and KVM crash
From: Eric Dumazet @ 2010-02-01 10:12 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Jon Masters, linux-kernel, netdev, netfilter-devel,
Patrick McHardy
In-Reply-To: <b6fcc0a1002010136k7e78a998p31a9e7464c2e8d44@mail.gmail.com>
Le lundi 01 février 2010 à 11:36 +0200, Alexey Dobriyan a écrit :
> On Mon, Feb 1, 2010 at 11:32 AM, Jon Masters <jonathan@jonmasters.org> wrote:
> > I hacked up a per-namespace version of hashtables (this needs doing
> > anyway, since the global stuff is just waiting to break)
>
> Which ones? Conntrack hashtables are per-netns.
It seems they are, but this is not a complete work :
1) Global settings (shared by all netns)
2) nf_conntrack_cachep is shared, it should be not shared.
3) nf_conntrack_untracked shared by all netns, it should be local.
nf_conntrack_cleanup_net() can block forever because of this.
while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
schedule();
...
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Patch v2] net: export TCP send buffer size via netlink
From: David Miller @ 2010-02-01 10:06 UTC (permalink / raw)
To: amwang; +Cc: linux-kernel, netdev, eric.dumazet
In-Reply-To: <20100201.020055.57470143.davem@davemloft.net>
Before you spend the next couple hours trying to figure out what to
do...
We already provide the information you're trying to export via
linux/inet_diag.h:inet_diag_meminfo
I hope the light goes on now :-)
^ permalink raw reply
* Re: [Patch v2] net: export TCP send buffer size via netlink
From: David Miller @ 2010-02-01 10:00 UTC (permalink / raw)
To: amwang; +Cc: linux-kernel, netdev, eric.dumazet
In-Reply-To: <20100201092153.4194.91507.sendpatchset@localhost.localdomain>
From: Amerigo Wang <amwang@redhat.com>
Date: Mon, 1 Feb 2010 04:18:28 -0500
> V1 -> V2:
> Keep ABI compatiblity of struct tcp_info.
>
> Currently, we can only get TCP send buffer size by
> getsockopt (SO_SNDBUF or TCP_INFO), this is not enough
> for the tools like netstat or ss to read.
>
> Show TCP send buffer size via netlink NETLINK_INET_DIAG.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
If the user has a "struct tcp_info info" on it's stack,
and it's compiled against the header file before your
changes, guess what the kernel is going to do?
It's going to write past the end of the user's variable
into random stack locations.
You can't change the layout, at all.
^ 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