* Re: [PATCH v3 net-next 2/2] netem: add cell concept to simulate special MAC behavior
From: Rick Jones @ 2011-12-01 18:25 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Laight, Hagen Paul Pfeifer, Eric Dumazet, netdev
In-Reply-To: <20111201085701.02f885c5@nehalam.linuxnetplumber.net>
On 12/01/2011 08:57 AM, Stephen Hemminger wrote:
> On Thu, 1 Dec 2011 16:38:51 -0000
> "David Laight"<David.Laight@ACULAB.COM> wrote:
>
>>
>>> One idea to do small delays at higher speed is to insert
>>> dummy pad frames into the device.
>>> It would mean generating garbage, but would allow for
>>> highly accurate fine grain delay.
>>
>> Not a good idea.
>> They would have to be sent to a known MAC address
>> otherwise all the ethernet switches would forward them
>> on all output ports.
>>
>> David
>>
>>
>
> Yes it would have to be a constant destination, not sure if there
> is a discard value in Ethernet protocol spec.
Aren't there special addresses that aren't supposed to be forwarded by
(intelligent) switches? IIRC LLDP uses such things. Though the IEEE
may take a dim view of using it for such a purpose, and knuth only knows
what switch bugs would be uncovered that way...
http://standards.ieee.org/develop/regauth/grpmac/public.html
http://en.wikipedia.org/wiki/Link_Layer_Discovery_Protocol
rick jones
^ permalink raw reply
* Re: [PATCH] drivers/net/ethernet/tile: use skb_frag_page() API
From: David Miller @ 2011-12-01 18:31 UTC (permalink / raw)
To: cmetcalf; +Cc: linux-kernel, netdev
In-Reply-To: <201112011815.pB1IFD2t004910@farm-0002.internal.tilera.com>
From: Chris Metcalf <cmetcalf@tilera.com>
Date: Thu, 1 Dec 2011 12:56:03 -0500
> This replaces raw access to the "page" field of the skb_frag_t.
>
> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
Please merge these changes via your Tile tree, you can add my:
Acked-by: David S. Miller <davem@davemloft.net>
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] ipv4: use a 64bit load/store in output path
From: David Miller @ 2011-12-01 18:32 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1322724521.2577.29.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 01 Dec 2011 08:28:41 +0100
> Le jeudi 01 décembre 2011 à 01:48 -0500, David Miller a écrit :
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Thu, 01 Dec 2011 06:00:53 +0100
>>
>> > gcc compiler is smart enough to use a single load/store if we
>> > memcpy(dptr, sptr, 8) on x86_64, regardless of
>> > CONFIG_CC_OPTIMIZE_FOR_SIZE
>> >
>> > In IP header, daddr immediately follows saddr, this wont change in the
>> > future. We only need to make sure our flowi4 (saddr,daddr) fields wont
>> > break the rule.
>> >
>> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>
>> Hmmm, this triggers a strange new build warning:
>>
>> net/dccp/ipv4.c: In function ‘dccp_v4_route_skb’:
>> net/dccp/ipv4.c:481:3: warning: initialized field with side-effects overwritten [enabled by default]
>> net/dccp/ipv4.c:481:3: warning: (near initialization for ‘fl4’) [enabled by default]
>
> Hmm, seems a compiler bug, since following fixes the warning ?
I stared at this for some time and I couldn't figure out even what GCC might
be thinking. I wondered if, for example, the WARN_ON() in the skb_dst()
implementation was part of the issue. Since that ends up calling functions.
Anyways, I applied the "iph" thing first, then your 64bit load/store change
afterwards.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/4] ipv4: Fix pmtu propagating
From: David Miller @ 2011-12-01 18:40 UTC (permalink / raw)
To: steffen.klassert; +Cc: gaofeng, netdev
In-Reply-To: <20111121075621.GF20943@secunet.com>
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Mon, 21 Nov 2011 08:56:21 +0100
> I still don't see any problems with both of the patches.
> So I assume that both patches would fix the issue. However,
> your patch is probaply less fragile as this does not depend
> on a certain value of dst->obsolete.
Great, thanks for checking it out.
I'll add the following to the net tree and queue it up for -stable
too.
--------------------
ipv4: Perform peer validation on cached route lookup.
Otherwise we won't notice the peer GENID change.
Reported-by: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/ipv4/route.c | 26 +++++++++++++++++++-------
1 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 57e01bc..ca5e237 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1693,12 +1693,8 @@ static void ip_rt_update_pmtu(struct dst_entry *dst, u32 mtu)
}
-static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
+static struct rtable *ipv4_validate_peer(struct rtable *rt)
{
- struct rtable *rt = (struct rtable *) dst;
-
- if (rt_is_expired(rt))
- return NULL;
if (rt->rt_peer_genid != rt_peer_genid()) {
struct inet_peer *peer;
@@ -1707,19 +1703,29 @@ static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
peer = rt->peer;
if (peer) {
- check_peer_pmtu(dst, peer);
+ check_peer_pmtu(&rt->dst, peer);
if (peer->redirect_genid != redirect_genid)
peer->redirect_learned.a4 = 0;
if (peer->redirect_learned.a4 &&
peer->redirect_learned.a4 != rt->rt_gateway) {
- if (check_peer_redir(dst, peer))
+ if (check_peer_redir(&rt->dst, peer))
return NULL;
}
}
rt->rt_peer_genid = rt_peer_genid();
}
+ return rt;
+}
+
+static struct dst_entry *ipv4_dst_check(struct dst_entry *dst, u32 cookie)
+{
+ struct rtable *rt = (struct rtable *) dst;
+
+ if (rt_is_expired(rt))
+ return NULL;
+ dst = (struct dst_entry *) ipv4_validate_peer(rt);
return dst;
}
@@ -2374,6 +2380,9 @@ int ip_route_input_common(struct sk_buff *skb, __be32 daddr, __be32 saddr,
rth->rt_mark == skb->mark &&
net_eq(dev_net(rth->dst.dev), net) &&
!rt_is_expired(rth)) {
+ rth = ipv4_validate_peer(rth);
+ if (!rth)
+ continue;
if (noref) {
dst_use_noref(&rth->dst, jiffies);
skb_dst_set_noref(skb, &rth->dst);
@@ -2749,6 +2758,9 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *flp4)
(IPTOS_RT_MASK | RTO_ONLINK)) &&
net_eq(dev_net(rth->dst.dev), net) &&
!rt_is_expired(rth)) {
+ rth = ipv4_validate_peer(rth);
+ if (!rth)
+ continue;
dst_use(&rth->dst, jiffies);
RT_CACHE_STAT_INC(out_hit);
rcu_read_unlock_bh();
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH] bridge: master device stuck in no-carrier state forever when in user-stp mode
From: Stephen Hemminger @ 2011-12-01 18:44 UTC (permalink / raw)
To: Vitalii Demianets; +Cc: netdev, bridge
In-Reply-To: <201111251216.37466.vitas@nppfactor.kiev.ua>
On Fri, 25 Nov 2011 12:16:37 +0200
Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:
> When in user-stp mode, bridge master do not follow state of its slaves, so
> after the following sequence of events it can stuck forever in no-carrier
> state:
> 1) turn stp off
> 2) put all slaves down - master device will follow their state and also go in
> no-carrier state
> 3) turn stp on with bridge-stp script returning 0 (go to the user-stp mode)
> Now bridge master won't follow slaves' state and will never reach running
> state.
>
> This patch solves the problem by making user-stp and kernel-stp behavior
> similar regarding master following slaves' states.
>
> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
This method works fine. David please apply to -net.
Acked-by: Stephen Hemminger <shemminger@vyatta.com>
^ permalink raw reply
* Re: [PATCH 1/4] nfc: Use standard logging styles
From: Joe Perches @ 2011-12-01 18:48 UTC (permalink / raw)
To: Samuel Ortiz
Cc: Lauro Ramos Venancio, Aloisio Almeida Jr, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <20111201155951.GF22609@sortiz-mobl>
On Thu, 2011-12-01 at 16:59 +0100, Samuel Ortiz wrote:
> Hi Joe,
Hello Samuel.
> On Tue, Nov 29, 2011 at 11:37:32AM -0800, Joe Perches wrote:
> > Using the normal logging styles is preferred over
> > subsystem specific styles when the subsystem does
> > not take a specific struct.
> > Convert nfc_<level> specific messages to pr_<level>
> > Add newlines to uses.
> I would be fine with moving to pr_debug, but I'd like the pr_fmt() routine to
> include the function name as well.
If you really want it I can add it back but
if you are not aware, dynamic debug can add
this by using:
echo "module nfc +fp" > <debugfs>/dynamic_debug/control
echo "module nci +fp" > <debugfs>/dynamic_debug/control
If you're getting dmesg logs from customers
that's a bit harder to track though.
^ permalink raw reply
* Re: [PATCH v2] icplus: mdio_write(), remove unnecessary for loop
From: Francois Romieu @ 2011-12-01 18:53 UTC (permalink / raw)
To: Patrick Kelle; +Cc: netdev, davem, sorbica
In-Reply-To: <1322746016-2835-1-git-send-email-patrick.kelle81@gmail.com>
Patrick Kelle <patrick.kelle81@gmail.com> :
[...]
> As suggested by David Miller:
> "You can simply this even further since p[7] is what is used here,
> and this means len is one, the inner loop therefore executes only
> once, and the p[7].field value is not used (it's zero in the table)
> and the write to it is completely thrown away."
Acked-by: Francois Romieu <romieu@fr.zoreil.com>
However you can:
- shorten p[] as p[7] is never used
- use read_phy_bit() to avoid the last three lines
- ARRAY_SIZE(p) the first loop (so the first limit - 7 - appears)
--
Ueimor
^ permalink raw reply
* Re: [PATCH] bridge: master device stuck in no-carrier state forever when in user-stp mode
From: David Miller @ 2011-12-01 19:05 UTC (permalink / raw)
To: shemminger; +Cc: netdev, bridge
In-Reply-To: <20111201104409.2d04ca06@nehalam.linuxnetplumber.net>
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 1 Dec 2011 10:44:09 -0800
> On Fri, 25 Nov 2011 12:16:37 +0200
> Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:
>
>> When in user-stp mode, bridge master do not follow state of its slaves, so
>> after the following sequence of events it can stuck forever in no-carrier
>> state:
>> 1) turn stp off
>> 2) put all slaves down - master device will follow their state and also go in
>> no-carrier state
>> 3) turn stp on with bridge-stp script returning 0 (go to the user-stp mode)
>> Now bridge master won't follow slaves' state and will never reach running
>> state.
>>
>> This patch solves the problem by making user-stp and kernel-stp behavior
>> similar regarding master following slaves' states.
>>
>> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
>
> This method works fine. David please apply to -net.
>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
Done.
^ permalink raw reply
* Re: [PATCH 6/7] neigh: Add device constructor/destructor capability.
From: Ben Hutchings @ 2011-12-01 19:08 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20111130.224151.1479303742039627135.davem@davemloft.net>
On Wed, 2011-11-30 at 22:41 -0500, David Miller wrote:
> If the neigh entry has device private state, it will need
> constructor/destructor ops.
>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
> include/linux/netdevice.h | 2 ++
> net/core/neighbour.c | 15 ++++++++++++++-
> 2 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5462c2c..1c4ddb3 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -974,6 +974,8 @@ struct net_device_ops {
> netdev_features_t features);
> int (*ndo_set_features)(struct net_device *dev,
> netdev_features_t features);
> + int (*ndo_neigh_construct)(struct neighbour *n);
> + int (*ndo_neigh_destroy)(struct neighbour *n);
ndo_neigh_destroy should return void, since the return value is not
used:
[...]
> @@ -707,7 +717,10 @@ void neigh_destroy(struct neighbour *neigh)
> skb_queue_purge(&neigh->arp_queue);
> neigh->arp_queue_len_bytes = 0;
>
> - dev_put(neigh->dev);
> + if (dev->netdev_ops->ndo_neigh_destroy)
> + dev->netdev_ops->ndo_neigh_destroy(neigh);
> +
> + dev_put(dev);
> neigh_parms_put(neigh->parms);
>
> NEIGH_PRINTK2("neigh %p is destroyed.\n", neigh);
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH 1/4] nfc: Use standard logging styles
From: Samuel Ortiz @ 2011-12-01 19:09 UTC (permalink / raw)
To: Joe Perches
Cc: Lauro Ramos Venancio, Aloisio Almeida Jr, David S. Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <1322765318.30801.9.camel@joe2Laptop>
Hi Joe,
On Thu, Dec 01, 2011 at 10:48:38AM -0800, Joe Perches wrote:
> > I would be fine with moving to pr_debug, but I'd like the pr_fmt() routine to
> > include the function name as well.
>
> If you really want it I can add it back but
> if you are not aware, dynamic debug can add
> this by using:
>
> echo "module nfc +fp" > <debugfs>/dynamic_debug/control
> echo "module nci +fp" > <debugfs>/dynamic_debug/control
Yes, I'm aware of that.
> If you're getting dmesg logs from customers
> that's a bit harder to track though.
Exactly. I'd prefer to have the __func__ being part of the pr_fmt().
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
^ permalink raw reply
* Re: Udp packets received with improper length
From: David Miller @ 2011-12-01 19:11 UTC (permalink / raw)
To: fsmail; +Cc: netdev, gerrit
In-Reply-To: <4ED3FBE8.2010006@conspiracy.net>
From: paul bilke <fsmail@conspiracy.net>
Date: Mon, 28 Nov 2011 15:23:52 -0600
> On 11/23/2011 2:14 PM, paul bilke wrote:
>> We recently updated an embedded powerpc platform from 2.6.32 to 2.6.37. When deployed in the field devices with the new kernel have started receiving truncated UDP packets from their mates across noisy links. To test we wrote a simple client and
>> server. The client sends 512 byte packets with a sequence number to the server listening on a UDP socket. On the client box we use netem to corrupt 100% of the packets sent(after transferring some data so arp cache is populated). The server then
>> dumps the length received and the serial number from any packets that are received. Netem sometimes corrupts bits in the source MAC address so these packets arrive with valid UDP checksums and are delivered to the user application. With the
>> server running on the 2.6.32 box we send a few million packets to it and only receive packets that are exactly 512 bytes long. When we do the same on the box running 2.6.37 we receive hundred of short packets, zero length and also 504 byte packets.
>> When I use TCPdump on the box running 2.6.37 the truncate packets have valid checksums (Source MAC was corrupted by NETEM) and are of proper length (554 byte ethernet frame, 540 Byte IP portion and 520 byte UDP length) but the userland receives 504
>> or 0 length in recvfrom. To see if this was just a powerpc related issue I repeated the test on x86 virtual machines. A vm running 2.6.18 (Centos 5) receives only 512 byte packets. On a vm running 2.6.40 (Fedora 15) I receive 512, 504 and 0 length
>> packets.
> <clip>
>
> Reverting commit 81d54ec8479a2c695760da81f05b5a9fb2dbe40a makes this problem disappear. The patch looks sane, the results are problematic.
I think that commit is buggy. If we do a goto to "try_again", the length has already been
truncated the first time around, so the calculation is not the same as what the original code
calculates
And indeed, checksum errors are how we can end up taking this code path.
I'm reverting.
^ permalink raw reply
* Re: [PATCH 6/7] neigh: Add device constructor/destructor capability.
From: David Miller @ 2011-12-01 19:16 UTC (permalink / raw)
To: bhutchings; +Cc: netdev
In-Reply-To: <1322766515.2797.0.camel@bwh-desktop>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Thu, 1 Dec 2011 19:08:35 +0000
> On Wed, 2011-11-30 at 22:41 -0500, David Miller wrote:
>> If the neigh entry has device private state, it will need
>> constructor/destructor ops.
>>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> ---
>> include/linux/netdevice.h | 2 ++
>> net/core/neighbour.c | 15 ++++++++++++++-
>> 2 files changed, 16 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 5462c2c..1c4ddb3 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -974,6 +974,8 @@ struct net_device_ops {
>> netdev_features_t features);
>> int (*ndo_set_features)(struct net_device *dev,
>> netdev_features_t features);
>> + int (*ndo_neigh_construct)(struct neighbour *n);
>> + int (*ndo_neigh_destroy)(struct neighbour *n);
>
> ndo_neigh_destroy should return void, since the return value is not
> used:
Agreed:
--
net: Make ndo_neigh_destroy return void.
The return value isn't used.
Suggested by Ben Hucthings.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/linux/netdevice.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1c4ddb3..21440e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -975,7 +975,7 @@ struct net_device_ops {
int (*ndo_set_features)(struct net_device *dev,
netdev_features_t features);
int (*ndo_neigh_construct)(struct neighbour *n);
- int (*ndo_neigh_destroy)(struct neighbour *n);
+ void (*ndo_neigh_destroy)(struct neighbour *n);
};
/*
--
1.7.7.3
^ permalink raw reply related
* [PATCH] dccp: Fix compile warning in probe code.
From: David Miller @ 2011-12-01 19:46 UTC (permalink / raw)
To: netdev; +Cc: gerrit, gerrit
Commit 1386be55e32a3c5d8ef4a2b243c530a7b664c02c ("dccp: fix
auto-loading of dccp(_probe)") fixed a bug but created a new
compiler warning:
net/dccp/probe.c: In function ‘dccpprobe_init’:
net/dccp/probe.c:166:2: warning: the omitted middle operand in ?: will always be ‘true’, suggest explicit middle operand [-Wparentheses]
try_then_request_module() is built for situations where the
"existence" test is some lookup function that returns a non-NULL
object on success, and with a reference count of some kind held.
Here we're looking for a success return of zero from the jprobe
registry.
Instead of fighting the way try_then_request_module() works, simply
open code what we want to happen in a local helper function.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/dccp/probe.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/dccp/probe.c b/net/dccp/probe.c
index 33d0e62..0a8d6eb 100644
--- a/net/dccp/probe.c
+++ b/net/dccp/probe.c
@@ -152,6 +152,17 @@ static const struct file_operations dccpprobe_fops = {
.llseek = noop_llseek,
};
+static __init int setup_jprobe(void)
+{
+ int ret = register_jprobe(&dccp_send_probe);
+
+ if (ret) {
+ request_module("dccp");
+ ret = register_jprobe(&dccp_send_probe);
+ }
+ return ret;
+}
+
static __init int dccpprobe_init(void)
{
int ret = -ENOMEM;
@@ -163,8 +174,7 @@ static __init int dccpprobe_init(void)
if (!proc_net_fops_create(&init_net, procname, S_IRUSR, &dccpprobe_fops))
goto err0;
- try_then_request_module((ret = register_jprobe(&dccp_send_probe)) == 0,
- "dccp");
+ ret = setup_jprobe();
if (ret)
goto err1;
--
1.7.7.3
^ permalink raw reply related
* Re: [PATCH next-next 0/2] can: cc770: add support for the Bosch CC770 and Intel AN82527
From: Marc Kleine-Budde @ 2011-12-01 20:04 UTC (permalink / raw)
To: David Miller; +Cc: socketcan, wg, netdev, linux-can, urs
In-Reply-To: <20111201.130907.1276930495409347261.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]
On 12/01/2011 07:09 PM, David Miller wrote:
>> We'll continue the discussion on linux-can ML and give an update to the
>> MAINTAINERS file if we sorted out any changes. So far Wolfgang and me remain
>> the 'only interface' for you. Sorry for the traffic & confusion on netdev.
>
> I said I want one person. One person who periodically send me GIT pull requests
> or a patch series for "CAN updates" of all kinds.
We're currently discussing the problem and we understood the one person
contact policy.
> Just like how the wireless stuff is maintained. They don't send me driver vs.
> protcol layer vs. rfcomm vs. bluetooth changes seperately, John Linville collects
> everything and sends me periodic pull requests.
>
> And this is what I want from you guys. One person who explicitly says "Dave, please
> take in these CAN changes" rather than the confusing manner in which changes get
> submitted currently.
regards, Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference at 000000000000002c
From: Eric Dumazet @ 2011-12-01 20:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Stefan Priebe, netdev, Dave Chinner, stable
In-Reply-To: <20111201182309.GA10686@infradead.org>
Le jeudi 01 décembre 2011 à 13:23 -0500, Christoph Hellwig a écrit :
> On Thu, Dec 01, 2011 at 07:11:51PM +0100, Stefan Priebe wrote:
> > Hi,
> >
> > i'm not quite sure if this is an XFS thing or a tcp thing. As the
> > stacktrace confuses me a little but as it contains
> > xfs_bmap_search_extents and xfs_iunlock. Kernel is 3.0.11 running as
> > 2.6.40.11.
> >
> > [171464.091567] BUG: unable to handle kernel NULL pointer
> > dereference at 000000000000002c
> > [171464.092073] IP: [<ffffffff815494f6>] ipv4_dst_check+0xb6/0x190
> > [171464.092073] PGD 15344e067 PUD 14c821067 PMD 0
> > [171464.092073] Oops: 0002 [#1] SMP
> >
> > Full Stacktrace:
> > http://pastebin.com/raw.php?i=QNg6jD2t
>
> The ? entries mean even the backtrace code things they are unreliable,
> and mos of the cases I've seen they are wrong. In this case it looks
> like the process was doing XFS work before, but the current system call
> indeed is purely in the networking code.
>
> I've changed the cc list to the netdev list which is more appropinquate.
Probably fixed by commit 81d54ec8479a2c695760da
(net: fix NULL dereferences in check_peer_redir())
But this patch needs some followup patches (some of them in David net
tree, not yet in Linus tree)
^ permalink raw reply
* nuclear neigh patch
From: David Miller @ 2011-12-01 20:20 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
Eric, how lucky are you feeling today? :-)
Here is a patch that gets rid of the neighbour reference for ipv4 routes.
Neigh entries are computed at packet send time in ip_output.c
Some minor work is needed before we can flat out use something like this,
the main issue being that there are places that assume that if we have
a 'dst' entry then there is a non-NULL neighbour unconditionally available.
Mainly this happens in the infiniband layer.
So we can't create a situation where dst_get_neighbour() might return NULL
just yet.
This is worth sorting out, because then we can get rid of the neigh
pointer entirely from dst entries.
But anyways, if this patch works it would be interesting to see what
it does to the UDP tests on your test machine(s).
diff --git a/include/net/arp.h b/include/net/arp.h
index 4979af8..41152df 100644
--- a/include/net/arp.h
+++ b/include/net/arp.h
@@ -15,24 +15,33 @@ static inline u32 arp_hashfn(u32 key, const struct net_device *dev, u32 hash_rnd
return val * hash_rnd;
}
-static inline struct neighbour *__ipv4_neigh_lookup(struct neigh_table *tbl, struct net_device *dev, u32 key)
+static inline struct neighbour *__ipv4_neigh_lookup_noref(struct neigh_table *tbl, struct net_device *dev, u32 key)
{
- struct neigh_hash_table *nht;
+ struct neigh_hash_table *nht = rcu_dereference_bh(tbl->nht);
struct neighbour *n;
u32 hash_val;
- rcu_read_lock_bh();
- nht = rcu_dereference_bh(tbl->nht);
+ if (dev->flags & (IFF_LOOPBACK | IFF_POINTOPOINT))
+ key = 0;
+
hash_val = arp_hashfn(key, dev, nht->hash_rnd) >> (32 - nht->hash_shift);
for (n = rcu_dereference_bh(nht->hash_buckets[hash_val]);
n != NULL;
n = rcu_dereference_bh(n->next)) {
- if (n->dev == dev && *(u32 *)n->primary_key == key) {
- if (!atomic_inc_not_zero(&n->refcnt))
- n = NULL;
- break;
- }
+ if (n->dev == dev && *(u32 *)n->primary_key == key)
+ return n;
}
+ return NULL;
+}
+
+static inline struct neighbour *__ipv4_neigh_lookup(struct neigh_table *tbl, struct net_device *dev, u32 key)
+{
+ struct neighbour *n;
+
+ rcu_read_lock_bh();
+ n = __ipv4_neigh_lookup_noref(tbl, dev, key);
+ if (n && !atomic_inc_not_zero(&n->refcnt))
+ n = NULL;
rcu_read_unlock_bh();
return n;
diff --git a/include/net/dst.h b/include/net/dst.h
index 6faec1a..15f0bac 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -86,14 +86,25 @@ struct dst_entry {
};
};
+static inline struct neighbour *dst_neigh_lookup(const struct dst_entry *dst)
+{
+ return dst->ops->neigh_lookup(dst);
+}
+
static inline struct neighbour *dst_get_neighbour(struct dst_entry *dst)
{
- return rcu_dereference(dst->_neighbour);
+ struct neighbour *n = rcu_dereference(dst->_neighbour);
+ if (!n)
+ n = dst_neigh_lookup(dst);
+ return n;
}
static inline struct neighbour *dst_get_neighbour_raw(struct dst_entry *dst)
{
- return rcu_dereference_raw(dst->_neighbour);
+ struct neighbour *n = rcu_dereference_raw(dst->_neighbour);
+ if (!n)
+ n = dst_neigh_lookup(dst);
+ return n;
}
static inline void dst_set_neighbour(struct dst_entry *dst, struct neighbour *neigh)
@@ -398,11 +409,6 @@ static inline void dst_confirm(struct dst_entry *dst)
}
}
-static inline struct neighbour *dst_neigh_lookup(const struct dst_entry *dst, const void *daddr)
-{
- return dst->ops->neigh_lookup(dst, daddr);
-}
-
static inline void dst_link_failure(struct sk_buff *skb)
{
struct dst_entry *dst = skb_dst(skb);
diff --git a/include/net/dst_ops.h b/include/net/dst_ops.h
index e1c2ee0..3a03d52 100644
--- a/include/net/dst_ops.h
+++ b/include/net/dst_ops.h
@@ -26,7 +26,7 @@ struct dst_ops {
void (*link_failure)(struct sk_buff *);
void (*update_pmtu)(struct dst_entry *dst, u32 mtu);
int (*local_out)(struct sk_buff *skb);
- struct neighbour * (*neigh_lookup)(const struct dst_entry *dst, const void *daddr);
+ struct neighbour * (*neigh_lookup)(const struct dst_entry *dst);
struct kmem_cache *kmem_cachep;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 0d5e567..72cc41a 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -205,15 +205,15 @@ static inline int ip_finish_output2(struct sk_buff *skb)
skb = skb2;
}
- rcu_read_lock();
- neigh = dst_get_neighbour(dst);
+ rcu_read_lock_bh();
+ neigh = __ipv4_neigh_lookup_noref(&arp_tbl, dev, rt->rt_gateway);
if (neigh) {
int res = neigh_output(neigh, skb);
- rcu_read_unlock();
+ rcu_read_unlock_bh();
return res;
}
- rcu_read_unlock();
+ rcu_read_unlock_bh();
if (net_ratelimit())
printk(KERN_DEBUG "ip_finish_output2: No header cache and no neighbour!\n");
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 9a20663..885dc3d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -184,7 +184,7 @@ static u32 *ipv4_cow_metrics(struct dst_entry *dst, unsigned long old)
return p;
}
-static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst, const void *daddr);
+static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst);
static struct dst_ops ipv4_dst_ops = {
.family = AF_INET,
@@ -1010,30 +1010,16 @@ static int slow_chain_length(const struct rtable *head)
return length >> FRACT_BITS;
}
-static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst, const void *daddr)
+static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst)
{
- static const __be32 inaddr_any = 0;
+ struct rtable *rt = (struct rtable *) dst;
struct net_device *dev = dst->dev;
- const __be32 *pkey = daddr;
struct neighbour *n;
- if (dev->flags & (IFF_LOOPBACK | IFF_POINTOPOINT))
- pkey = &inaddr_any;
-
- n = __ipv4_neigh_lookup(&arp_tbl, dev, *(__force u32 *)pkey);
+ n = __ipv4_neigh_lookup(&arp_tbl, dev, rt->rt_gateway);
if (n)
return n;
- return neigh_create(&arp_tbl, pkey, dev);
-}
-
-static int rt_bind_neighbour(struct rtable *rt)
-{
- struct neighbour *n = ipv4_neigh_lookup(&rt->dst, &rt->rt_gateway);
- if (IS_ERR(n))
- return PTR_ERR(n);
- dst_set_neighbour(&rt->dst, n);
-
- return 0;
+ return neigh_create(&arp_tbl, &rt->rt_gateway, dev);
}
static struct rtable *rt_intern_hash(unsigned hash, struct rtable *rt,
@@ -1044,7 +1030,6 @@ static struct rtable *rt_intern_hash(unsigned hash, struct rtable *rt,
unsigned long now;
u32 min_score;
int chain_length;
- int attempts = !in_softirq();
restart:
chain_length = 0;
@@ -1071,17 +1056,6 @@ restart:
*/
rt->dst.flags |= DST_NOCACHE;
- if (rt->rt_type == RTN_UNICAST || rt_is_output_route(rt)) {
- int err = rt_bind_neighbour(rt);
- if (err) {
- if (net_ratelimit())
- printk(KERN_WARNING
- "Neighbour table failure & not caching routes.\n");
- ip_rt_put(rt);
- return ERR_PTR(err);
- }
- }
-
goto skip_hashing;
}
@@ -1164,41 +1138,6 @@ restart:
}
}
- /* Try to bind route to arp only if it is output
- route or unicast forwarding path.
- */
- if (rt->rt_type == RTN_UNICAST || rt_is_output_route(rt)) {
- int err = rt_bind_neighbour(rt);
- if (err) {
- spin_unlock_bh(rt_hash_lock_addr(hash));
-
- if (err != -ENOBUFS) {
- rt_drop(rt);
- return ERR_PTR(err);
- }
-
- /* Neighbour tables are full and nothing
- can be released. Try to shrink route cache,
- it is most likely it holds some neighbour records.
- */
- if (attempts-- > 0) {
- int saved_elasticity = ip_rt_gc_elasticity;
- int saved_int = ip_rt_gc_min_interval;
- ip_rt_gc_elasticity = 1;
- ip_rt_gc_min_interval = 0;
- rt_garbage_collect(&ipv4_dst_ops);
- ip_rt_gc_min_interval = saved_int;
- ip_rt_gc_elasticity = saved_elasticity;
- goto restart;
- }
-
- if (net_ratelimit())
- printk(KERN_WARNING "ipv4: Neighbour table overflow.\n");
- rt_drop(rt);
- return ERR_PTR(-ENOBUFS);
- }
- }
-
rt->dst.rt_next = rt_hash_table[hash].chain;
/*
@@ -1308,7 +1247,7 @@ static int check_peer_redir(struct dst_entry *dst, struct inet_peer *peer)
rt->rt_gateway = peer->redirect_learned.a4;
- n = ipv4_neigh_lookup(&rt->dst, &rt->rt_gateway);
+ n = ipv4_neigh_lookup(&rt->dst);
if (IS_ERR(n))
return PTR_ERR(n);
old_n = xchg(&rt->dst._neighbour, n);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 0e381bb..9723e90 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -132,9 +132,11 @@ static u32 *ipv6_cow_metrics(struct dst_entry *dst, unsigned long old)
return p;
}
-static struct neighbour *ip6_neigh_lookup(const struct dst_entry *dst, const void *daddr)
+static struct neighbour *ip6_neigh_lookup(const struct dst_entry *dst)
{
- return __neigh_lookup_errno(&nd_tbl, daddr, dst->dev);
+ struct rt6_info *rt = (struct rt6_info *) dst;
+
+ return __neigh_lookup_errno(&nd_tbl, &rt->rt6i_gateway, dst->dev);
}
static struct dst_ops ip6_dst_ops_template = {
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4fce1ce..acac0a7 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2389,9 +2389,9 @@ static unsigned int xfrm_mtu(const struct dst_entry *dst)
return mtu ? : dst_mtu(dst->path);
}
-static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst, const void *daddr)
+static struct neighbour *xfrm_neigh_lookup(const struct dst_entry *dst)
{
- return dst_neigh_lookup(dst->path, daddr);
+ return dst_neigh_lookup(dst->path);
}
int xfrm_policy_register_afinfo(struct xfrm_policy_afinfo *afinfo)
^ permalink raw reply related
* Re: BUG: unable to handle kernel NULL pointer dereference at 000000000000002c
From: Josh Boyer @ 2011-12-01 20:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Christoph Hellwig, Stefan Priebe, netdev, Dave Chinner, stable
In-Reply-To: <1322770844.2750.1.camel@edumazet-laptop>
On Thu, Dec 1, 2011 at 3:20 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 01 décembre 2011 à 13:23 -0500, Christoph Hellwig a écrit :
>> On Thu, Dec 01, 2011 at 07:11:51PM +0100, Stefan Priebe wrote:
>> > Hi,
>> >
>> > i'm not quite sure if this is an XFS thing or a tcp thing. As the
>> > stacktrace confuses me a little but as it contains
>> > xfs_bmap_search_extents and xfs_iunlock. Kernel is 3.0.11 running as
>> > 2.6.40.11.
>> >
>> > [171464.091567] BUG: unable to handle kernel NULL pointer
>> > dereference at 000000000000002c
>> > [171464.092073] IP: [<ffffffff815494f6>] ipv4_dst_check+0xb6/0x190
>> > [171464.092073] PGD 15344e067 PUD 14c821067 PMD 0
>> > [171464.092073] Oops: 0002 [#1] SMP
>> >
>> > Full Stacktrace:
>> > http://pastebin.com/raw.php?i=QNg6jD2t
>>
>> The ? entries mean even the backtrace code things they are unreliable,
>> and mos of the cases I've seen they are wrong. In this case it looks
>> like the process was doing XFS work before, but the current system call
>> indeed is purely in the networking code.
>>
>> I've changed the cc list to the netdev list which is more appropinquate.
>
> Probably fixed by commit 81d54ec8479a2c695760da
> (net: fix NULL dereferences in check_peer_redir())
Did you mean f2c31e32b378a665 ? 81d54ec84 is:
udp: remove redundant variable
in Linus' tree.
> But this patch needs some followup patches (some of them in David net
> tree, not yet in Linus tree)
9de79c127cccecb11ae6a21ab1499e87aa222880 (net: fix potential neighbour
race in dst_ifdown()) is in Linus' tree. Which others are missing?
josh
^ permalink raw reply
* Re: Bug in computing data_len in tcp_sendmsg?
From: Eric Dumazet @ 2011-12-01 20:37 UTC (permalink / raw)
To: Jesse Brandeburg
Cc: Tom Herbert, Linux Netdev List, David Miller, alexander.h.duyck
In-Reply-To: <20111201092950.00006ce8@unknown>
Le jeudi 01 décembre 2011 à 09:29 -0800, Jesse Brandeburg a écrit :
> On Thu, 1 Dec 2011 01:15:37 -0800
> Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Tom, thanks very much for finding this subtle bug! I bet that this
> commit (which I had missed) broke a lot of other drivers in
> very subtle ways due to changing a long standing behavior. Auditing
> every driver's tx path is going to be a lot of work unless a way can be
> discovered to automate finding incorrect assumptions in drivers.
>
I did a quick check an counted 3 intel drivers.
Others are fine. I'll redo a check.
> > I dont know why its even necessary :
> >
> > TSO enabled NIC all provide hardware counters, so why even bother
> > computing tx_bytes ourself ?
>
> because we have runtime logic for adjusting interrupt rate that depends
> on knowing how many bytes and packets were cleaned up in an interrupt
> and reading MMIO to get latest stats causes CPU stall in hot path.
> Counting the bytes sent on the wire via a segmented SKB was hard to get
> right to begin with, I think we took a different approach in ixgbe
> (recent versions) and compute the math in hard_start_xmit instead of tx
> cleanup.
>
> > skb->len is appropriate for BQL, as long as producers/consumer use the
> > same skb->len. 1 or 2% error is not a problem if not cumulative ?
>
> if skb->len access doesn't cause a cache miss in hot path (or at
> least doesn't increase misses) then I say sure.
You still can cache it like now in your bytecount field in start_xmit(),
but since you call skb_free(), I doubt this matters a lot.
^ permalink raw reply
* Re: [bug?] r8169: hangs under heavy load
From: booster @ 2011-12-01 20:42 UTC (permalink / raw)
To: Francois Romieu
Cc: hayeswang, 'Jonathan Nieder', 'Eric Dumazet',
netdev, 'nic_swsd', linux-kernel, 'Armin Kazmi'
In-Reply-To: <20111201102000.GA14013@electric-eye.fr.zoreil.com>
Francois Romieu wrote:
> booster@wolke7.net <booster@wolke7.net> :
> [...]
>
>> Any more ideas ?
>>
>
> Give me a brain.
>
> Fixed patch below.
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 6f06aa1..e776bf2 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4885,8 +4894,7 @@ static void rtl_hw_start_8168(struct net_device *dev)
> RTL_W16(IntrMitigate, 0x5151);
>
> /* Work around for RxFIFO overflow. */
> - if (tp->mac_version == RTL_GIGA_MAC_VER_11 ||
> - tp->mac_version == RTL_GIGA_MAC_VER_22) {
> + if (tp->mac_version == RTL_GIGA_MAC_VER_11) {
> tp->intr_event |= RxFIFOOver | PCSTimeout;
> tp->intr_event &= ~RxOverflow;
> }
> @@ -5804,6 +5812,10 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> */
> status = RTL_R16(IntrStatus);
> while (status && status != 0xffff) {
> + status &= tp->intr_event;
> + if (!status)
> + break;
> +
> handled = 1;
>
> /* Handle all of the error cases first. These will reset
> @@ -5818,7 +5830,6 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> switch (tp->mac_version) {
> /* Work around for rx fifo overflow */
> case RTL_GIGA_MAC_VER_11:
> - case RTL_GIGA_MAC_VER_22:
> case RTL_GIGA_MAC_VER_26:
> netif_stop_queue(dev);
> rtl8169_tx_timeout(dev);
> @@ -5828,6 +5839,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
> case RTL_GIGA_MAC_VER_19:
> case RTL_GIGA_MAC_VER_20:
> case RTL_GIGA_MAC_VER_21:
> + case RTL_GIGA_MAC_VER_22:
> case RTL_GIGA_MAC_VER_23:
> case RTL_GIGA_MAC_VER_24:
> case RTL_GIGA_MAC_VER_27:
>
>
Hello Francois,
it looks OK now - no more "eth0: link up" messages and network hangs any
more.
I've send and received 60 GB to and from the system and the performance
is ~30 MB/s.
I'll keep the system up and running and do some more stability tests.
The only thing that's missing now is the transfer LED: the yellow link
LED is on,
but the green transfer LED is off and doesn't blink.
Thanks a lot for your great support.
Regards,
Gerd
Output:
[ 2.334588] r8169 Gigabit Ethernet driver 2.3LK-NAPI-patch3 loaded
[ 2.334639] r8169 0000:02:00.0: PCI INT A -> GSI 16 (level, low) ->
IRQ 16
[ 2.334689] r8169 0000:02:00.0: setting latency timer to 64
[ 2.334767] r8169 0000:02:00.0: irq 40 for MSI/MSI-X
[ 2.387618] r8169 0000:02:00.0: eth0: RTL8168c/8111c[21] at
0xf8224000, 00:01:c0:08:aa:31, XID 1c4000c0 IRQ 40
[ 2.387630] r8169 0000:02:00.0: eth0: jumbo features [frames: 6128
bytes, tx checksumming: ko]
[ 2.392954] r8169 Gigabit Ethernet driver 2.3LK-NAPI-patch3 loaded
[ 2.393015] r8169 0000:03:00.0: PCI INT A -> GSI 17 (level, low) ->
IRQ 17
[ 2.393082] r8169 0000:03:00.0: setting latency timer to 64
[ 2.393166] r8169 0000:03:00.0: irq 41 for MSI/MSI-X
[ 2.394707] r8169 0000:03:00.0: eth1: RTL8168c/8111c[21] at
0xf8238000, 00:01:c0:08:aa:32, XID 1c4000c0 IRQ 41
[ 2.394718] r8169 0000:03:00.0: eth1: jumbo features [frames: 6128
bytes, tx checksumming: ko]
[ 14.324097] r8169 0000:03:00.0: eth1: link down
[ 14.324555] ADDRCONF(NETDEV_UP): eth1: link is not ready
[ 14.419038] r8169 0000:02:00.0: eth0: link down
[ 14.419049] r8169 0000:02:00.0: eth0: link down
[ 14.419419] ADDRCONF(NETDEV_UP): eth0: link is not ready
[ 16.789915] r8169 0000:02:00.0: eth0: link up
[ 16.791046] ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
driver: r8169
version: 2.3LK-NAPI-patch3
firmware-version: N/A
bus-info: 0000:02:00.0
Settings for eth0:
Supported ports: [ TP MII ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Half 1000baseT/Full
Supports auto-negotiation: Yes
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Half 1000baseT/Full
Advertised pause frame use: Symmetric Receive-only
Advertised auto-negotiation: Yes
Link partner advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Link partner advertised pause frame use: Symmetric
Link partner advertised auto-negotiation: Yes
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: pumbg
Wake-on: g
Current message level: 0x00000033 (51)
Link detected: yes
^ permalink raw reply
* Re: ipv4: broadcast sometimes leaves wrong interface (since commit e066008b38ca9ace1b6de8dbbac8ed460640791d)
From: Julian Anastasov @ 2011-12-01 21:00 UTC (permalink / raw)
To: Jeroen van Ingen; +Cc: David Miller, netdev
In-Reply-To: <1322756728.12482.52.camel@icts-sp-039>
Hello,
On Thu, 1 Dec 2011, Jeroen van Ingen wrote:
> FYI: we successfully tested two scenarios that provide a workaround with
> the current kernel versions:
>
> 1) Explicitly configuring Radius to use one of the secondary IPs as
> source for the DHCP broadcast. Since the IP we chose is only bound to
> eth0, this broadcast goes out the correct interface. Other
> system-generated broadcast would probably still go out the wrong
> interface, but at least it allows us to accept more than one PPTP
> client.
>
> 2) Adding an option to the "pppd" config, so the ppp-devices it creates
> do not use the primary IP from eth0 but rather one of the secondary
> addresses. This way we don't have to modify any other software that
> might generate a broadcast.
>
>
> While the second option provides a workable solution for us, we're still
> under the impression that this change in behavior might cause a problem
> for other users and/or configurations as well. We'll leave the rest of
> the considerations to the real experts, while remaining curious about
> the outcome :)
Yes, thanks for confirming that this is the actual
problem! I hope David will find the best way to restore
this feature.
> Thanks again for your assistance.
>
>
> Regards,
>
> Jeroen van Ingen
> ICT Service Centre
> University of Twente, P.O.Box 217, 7500 AE Enschede, The Netherlands
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference at 000000000000002c
From: Eric Dumazet @ 2011-12-01 21:05 UTC (permalink / raw)
To: Josh Boyer; +Cc: Christoph Hellwig, Stefan Priebe, netdev, Dave Chinner, stable
In-Reply-To: <CA+5PVA6kZkgYus4698Up8Q8uMTetCU2=DnTD0RpgXuHkJj6KKg@mail.gmail.com>
Le jeudi 01 décembre 2011 à 15:37 -0500, Josh Boyer a écrit :
> Did you mean f2c31e32bf2c31e32b378a665 ?
Oh well, yes ;)
> > But this patch needs some followup patches (some of them in David net
> > tree, not yet in Linus tree)
>
> 9de79c127cccecb11ae6a21ab1499e87aa222880 (net: fix potential neighbour
> race in dst_ifdown()) is in Linus' tree. Which others are missing?
Hmm
Linus tree :
commit 580da35a31f91a594f3090b7a2c39b85cb051a12
IB: Fix RCU lockdep splats
David tree :
commit 218fa90f072e4aeff9003d57e390857f4f35513e
ipv4: fix lockdep splat in rt_cache_seq_show
commit f7e57044eeb1841847c24aa06766c8290c202583
sch_teql: fix lockdep splat
^ permalink raw reply
* [PATCH] sch_red: fix red_change
From: Eric Dumazet @ 2011-12-01 21:06 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Thomas Graf, netdev
In-Reply-To: <20111130143642.7130aa2b@nehalam.linuxnetplumber.net>
Le mercredi 30 novembre 2011 à 14:36 -0800, Stephen Hemminger a écrit :
> (Almost) nobody uses RED because they can't figure it out.
> According to Wikipedia, VJ says that:
> "there are not one, but two bugs in classic RED."
RED is useful for high throughput routers, I doubt many linux machines
act as such devices.
I was considering adding Adaptative RED (Sally Floyd, Ramakrishna
Gummadi, Scott Shender), August 2001
In this version, maxp is dynamic (from 1% to 50%), and user only have to
setup min_th (target average queue size)
(max_th and wq (burst in linux RED) are automatically setup)
By the way it seems we have a small bug in red_change()
if (skb_queue_empty(&sch->q))
red_end_of_idle_period(&q->parms);
First, if queue is empty, we should call
red_start_of_idle_period(&q->parms);
Second, since we dont use anymore sch->q, but q->qdisc, the test is
meaningless.
Oh well...
[PATCH] sch_red: fix red_change()
Now RED is classful, we must check q->qdisc->q.qlen, and if queue is empty,
we start an idle period, not end it.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 6649463..d617161 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -209,8 +209,8 @@ static int red_change(struct Qdisc *sch, struct nlattr *opt)
ctl->Plog, ctl->Scell_log,
nla_data(tb[TCA_RED_STAB]));
- if (skb_queue_empty(&sch->q))
- red_end_of_idle_period(&q->parms);
+ if (!q->qdisc->q.qlen)
+ red_start_of_idle_period(&q->parms);
sch_tree_unlock(sch);
return 0;
^ permalink raw reply related
* Re: nuclear neigh patch
From: Eric Dumazet @ 2011-12-01 21:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20111201.152057.981138658170403943.davem@davemloft.net>
Le jeudi 01 décembre 2011 à 15:20 -0500, David Miller a écrit :
> Eric, how lucky are you feeling today? :-)
>
> Here is a patch that gets rid of the neighbour reference for ipv4 routes.
> Neigh entries are computed at packet send time in ip_output.c
>
> Some minor work is needed before we can flat out use something like this,
> the main issue being that there are places that assume that if we have
> a 'dst' entry then there is a non-NULL neighbour unconditionally available.
> Mainly this happens in the infiniband layer.
>
> So we can't create a situation where dst_get_neighbour() might return NULL
> just yet.
>
> This is worth sorting out, because then we can get rid of the neigh
> pointer entirely from dst entries.
>
> But anyways, if this patch works it would be interesting to see what
> it does to the UDP tests on your test machine(s).
Sure, this looks great.
I'll try this tomorrow on my test machines, its 10:15pm here ;)
^ permalink raw reply
* Re: [PATCH] sch_red: fix red_change
From: Dave Taht @ 2011-12-01 21:35 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stephen Hemminger, Thomas Graf, netdev, Jim Gettys
In-Reply-To: <1322773594.2750.28.camel@edumazet-laptop>
On Thu, Dec 1, 2011 at 10:06 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 30 novembre 2011 à 14:36 -0800, Stephen Hemminger a écrit :
>
>> (Almost) nobody uses RED because they can't figure it out.
>> According to Wikipedia, VJ says that:
>> "there are not one, but two bugs in classic RED."
Heh. "There were not two, but four bugs in Linux red".
Now reduced to 2. :)
> RED is useful for high throughput routers, I doubt many linux machines
> act as such devices.
"High throughput" at the time red was designed was not much faster
than a T1 line.
RED appears to be used by default in both gargoyle's and openwrt's QoS systems,
underneath unholy combinations of HTB, HSFC, and SFQ
so it's more widely used than you might think. Not that works well.
RED doesn't work worth beans on variable bandwidth links (cable
modems/wireless).
Once you are simulating a fixed rate link (e.g with HTB), then it sort of
kinda maybe can apply.
RED was also designed at a time when long distance traffic was fixed rate
and bidirectional, so the 'average packet' parameter made sense.
Modern day traffic is far more asymmetric.
RED might still be fairly effective on a modern day fixed rate line,
such as DSL,
and on DSLAMS and the like.
Linux's red has an additional problem in that it seems byte oriented
rather than packet
oriented, and most folk even trying to do simulations with red seem to be
choosing packet oriented - which ties into the asymmetric problem noted above
mildly better.
All that said, it's time came, and is rapidly ending.
I do look forward to a replacement for the algorithm one day soon.
> I was considering adding Adaptative RED (Sally Floyd, Ramakrishna
> Gummadi, Scott Shender), August 2001
>
> In this version, maxp is dynamic (from 1% to 50%), and user only have to
> setup min_th (target average queue size)
> (max_th and wq (burst in linux RED) are automatically setup)
I currently have no opinion. There are hundreds of papers on red
and red-like algorithms. cc'ing jim for an opinion. Will read paper.
I'd like to find something that dealt with superpackets sanely.
>
> By the way it seems we have a small bug in red_change()
>
> if (skb_queue_empty(&sch->q))
> red_end_of_idle_period(&q->parms);
>
> First, if queue is empty, we should call
> red_start_of_idle_period(&q->parms);
>
> Second, since we dont use anymore sch->q, but q->qdisc, the test is
> meaningless.
>
> Oh well...
>
> [PATCH] sch_red: fix red_change()
>
> Now RED is classful, we must check q->qdisc->q.qlen, and if queue is empty,
> we start an idle period, not end it.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
> index 6649463..d617161 100644
> --- a/net/sched/sch_red.c
> +++ b/net/sched/sch_red.c
> @@ -209,8 +209,8 @@ static int red_change(struct Qdisc *sch, struct nlattr *opt)
> ctl->Plog, ctl->Scell_log,
> nla_data(tb[TCA_RED_STAB]));
>
> - if (skb_queue_empty(&sch->q))
> - red_end_of_idle_period(&q->parms);
> + if (!q->qdisc->q.qlen)
> + red_start_of_idle_period(&q->parms);
>
> sch_tree_unlock(sch);
> return 0;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Dave Täht
SKYPE: davetaht
US Tel: 1-239-829-5608
FR Tel: 0638645374
http://www.bufferbloat.net
^ permalink raw reply
* Re: [PATCH] sch_red: fix red_change
From: Jim Gettys @ 2011-12-01 21:48 UTC (permalink / raw)
To: Dave Taht; +Cc: Eric Dumazet, Stephen Hemminger, Thomas Graf, netdev
In-Reply-To: <CAA93jw7wAL75pVrB-q+xGAcpQtiZz4iVW0Z2W9E8VGSyUFOo-w@mail.gmail.com>
On 12/01/2011 04:35 PM, Dave Taht wrote:
> On Thu, Dec 1, 2011 at 10:06 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> Le mercredi 30 novembre 2011 à 14:36 -0800, Stephen Hemminger a écrit :
>>
>>> (Almost) nobody uses RED because they can't figure it out.
>>> According to Wikipedia, VJ says that:
>>> "there are not one, but two bugs in classic RED."
> Heh. "There were not two, but four bugs in Linux red".
>
> Now reduced to 2. :)
That's good. There may be enough use to be worth fixing... RED itself
is buggy in a couple ways, including it's fundamental idea of keying off
the queue length.
>
>> RED is useful for high throughput routers, I doubt many linux machines
>> act as such devices.
> "High throughput" at the time red was designed was not much faster
> than a T1 line.
>
> RED appears to be used by default in both gargoyle's and openwrt's QoS systems,
> underneath unholy combinations of HTB, HSFC, and SFQ
> so it's more widely used than you might think. Not that works well.
>
> RED doesn't work worth beans on variable bandwidth links (cable
> modems/wireless).
>
> Once you are simulating a fixed rate link (e.g with HTB), then it sort of
> kinda maybe can apply.
>
> RED was also designed at a time when long distance traffic was fixed rate
> and bidirectional, so the 'average packet' parameter made sense.
> Modern day traffic is far more asymmetric.
Not to mention is that RED is fundamentally flawed: there is almost no
information in the instantaneous length of the queue. You can see how
variable bandwidth screws this idea to the wall, can't you?
So the closer to the edge you get (where BDP variation in flows will
dominate), the worse classic RED can possibly work. In the home, you
are at the extreme, along with variable bandwidth now with Powerboost
and wireless.
>
> RED might still be fairly effective on a modern day fixed rate line,
> such as DSL,
> and on DSLAMS and the like.
Might is the operative word; the resurrection of using N TCP connections
in the web, and the ICW change to 10 makes me highly skeptical.
>
> Linux's red has an additional problem in that it seems byte oriented
> rather than packet
> oriented, and most folk even trying to do simulations with red seem to be
> choosing packet oriented - which ties into the asymmetric problem noted above
> mildly better.
>
> All that said, it's time came, and is rapidly ending.
>
> I do look forward to a replacement for the algorithm one day soon.
Kathie and Van have been simulating their new algorithm; hopeful
results, but still too soon to tell as not enough situations have been
simulated. And they need to talk to Eben Moglen yet about ensuring
proper publication so that no one can patent it out from under them.
They want no barriers to widespread adoption in free software (and
anywhere else).
I get the sense that maybe there might be something to try in a couple
months; probably first best to do it on ethernet rather than wireless.
- Jim
>
>> I was considering adding Adaptative RED (Sally Floyd, Ramakrishna
>> Gummadi, Scott Shender), August 2001
>>
>> In this version, maxp is dynamic (from 1% to 50%), and user only have to
>> setup min_th (target average queue size)
>> (max_th and wq (burst in linux RED) are automatically setup)
> I currently have no opinion. There are hundreds of papers on red
> and red-like algorithms. cc'ing jim for an opinion. Will read paper.
>
> I'd like to find something that dealt with superpackets sanely.
>
>> By the way it seems we have a small bug in red_change()
>>
>> if (skb_queue_empty(&sch->q))
>> red_end_of_idle_period(&q->parms);
>>
>> First, if queue is empty, we should call
>> red_start_of_idle_period(&q->parms);
>>
>> Second, since we dont use anymore sch->q, but q->qdisc, the test is
>> meaningless.
>>
>> Oh well...
>>
>> [PATCH] sch_red: fix red_change()
>>
>> Now RED is classful, we must check q->qdisc->q.qlen, and if queue is empty,
>> we start an idle period, not end it.
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
>> index 6649463..d617161 100644
>> --- a/net/sched/sch_red.c
>> +++ b/net/sched/sch_red.c
>> @@ -209,8 +209,8 @@ static int red_change(struct Qdisc *sch, struct nlattr *opt)
>> ctl->Plog, ctl->Scell_log,
>> nla_data(tb[TCA_RED_STAB]));
>>
>> - if (skb_queue_empty(&sch->q))
>> - red_end_of_idle_period(&q->parms);
>> + if (!q->qdisc->q.qlen)
>> + red_start_of_idle_period(&q->parms);
>>
>> sch_tree_unlock(sch);
>> return 0;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" 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
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