* [PATCH 2/2] socket: add minimum listen queue length sysctl
From: Hagen Paul Pfeifer @ 2011-03-25 18:31 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet
In-Reply-To: <1301077899-16482-1-git-send-email-hagen@jauu.net>
In the case that a server programmer misjudge network characteristic the
backlog parameter for listen(2) may not adequate to utilize hosts
capabilities and lead to unrequired SYN retransmission - thus a
underestimated backlog value can form an artificial limitation.
A listen queue length of 8 is often a way to small, but several
server authors does not about know this limitation (from Erics server
setup):
ss -a | head
State Recv-Q Send-Q Local Address:Port Peer
Address:Port
LISTEN 0 8 *:imaps *:*
LISTEN 0 8 *:pop3s *:*
LISTEN 0 50 *:mysql *:*
LISTEN 0 8 *:pop3 *:*
LISTEN 0 8 *:imap2 *:*
LISTEN 0 511 *:www *:*
Until now it was not possible for the system (network) administrator to
increase this value. A bug report must be filled, the backlog increased,
a new version released or even worse: if using closed source software
you cannot make anything.
sysctl_min_syn_backlog provides the ability to increase the minimum
queue length.
Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
include/net/request_sock.h | 1 +
net/core/request_sock.c | 6 +++++-
net/ipv4/af_inet.c | 2 +-
net/ipv4/sysctl_net_ipv4.c | 7 +++++++
4 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/request_sock.h b/include/net/request_sock.h
index 99e6e19..3e8865f 100644
--- a/include/net/request_sock.h
+++ b/include/net/request_sock.h
@@ -89,6 +89,7 @@ static inline void reqsk_free(struct request_sock *req)
}
extern int sysctl_max_syn_backlog;
+extern int sysctl_min_syn_backlog;
/** struct listen_sock - listen state
*
diff --git a/net/core/request_sock.c b/net/core/request_sock.c
index 182236b..0e968b6 100644
--- a/net/core/request_sock.c
+++ b/net/core/request_sock.c
@@ -35,6 +35,9 @@
int sysctl_max_syn_backlog = 256;
EXPORT_SYMBOL(sysctl_max_syn_backlog);
+int sysctl_min_syn_backlog = 0;
+EXPORT_SYMBOL(sysctl_min_syn_backlog);
+
int reqsk_queue_alloc(struct request_sock_queue *queue,
unsigned int nr_table_entries)
{
@@ -42,7 +45,8 @@ int reqsk_queue_alloc(struct request_sock_queue *queue,
struct listen_sock *lopt;
nr_table_entries = min_t(u32, nr_table_entries, sysctl_max_syn_backlog);
- nr_table_entries = max_t(u32, nr_table_entries, 8);
+ nr_table_entries = max_t(u32, nr_table_entries,
+ max_t(u32, 8, sysctl_min_syn_backlog));
nr_table_entries = roundup_pow_of_two(nr_table_entries + 1);
lopt_size += nr_table_entries * sizeof(struct request_sock *);
if (lopt_size > PAGE_SIZE)
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 807d83c..c580d7c 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -213,7 +213,7 @@ int inet_listen(struct socket *sock, int backlog)
if (err)
goto out;
}
- sk->sk_max_ack_backlog = backlog;
+ sk->sk_max_ack_backlog = max_t(u32, backlog, sysctl_min_syn_backlog);
err = 0;
out:
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 1a45665..cc03c62 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -298,6 +298,13 @@ static struct ctl_table ipv4_table[] = {
.proc_handler = proc_dointvec
},
{
+ .procname = "tcp_min_syn_backlog",
+ .data = &sysctl_min_syn_backlog,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec
+ },
+ {
.procname = "ip_local_port_range",
.data = &sysctl_local_ports.range,
.maxlen = sizeof(sysctl_local_ports.range),
--
1.7.2.3
^ permalink raw reply related
* [PATCH 1/2] socket: increase default maximum listen queue length
From: Hagen Paul Pfeifer @ 2011-03-25 18:31 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet
sysctl_somaxconn specifies the maximum number of sockets in state
SYN_RECV per listen socket and is initialized with 128 (SOMAXCONN).
sysctl_max_syn_backlog on the other hand provides similar functionality:
provides a system wide upper limit of request sockets per listen socket.
But sysctl_max_syn_backlog provides a more accurate value by considerate
the actual memory situation of the system. 256 by default, 128 for
systems with low memory and up to 1024 for larger systems.
This patch increase sysctl_somaxconn to 256 and provide environments with
a increased RTT and many connections/second a better default value by
simultaneously provides the fallback that smaller systems will not suffer
of an increased memory usage - sysctl_max_syn_backlog is already a good
guard.
Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
include/linux/socket.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/socket.h b/include/linux/socket.h
index edbb1d0..bf35ce2 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -237,7 +237,7 @@ struct ucred {
#define PF_MAX AF_MAX
/* Maximum queue length specifiable by listen. */
-#define SOMAXCONN 128
+#define SOMAXCONN 256
/* Flags we can use with send/ and recv.
Added those for 1003.1g not all are supported yet
--
1.7.2.3
^ permalink raw reply related
* disabling ipv6 (when ipv6 module is already loaded or built in)
From: Arkadiusz Miskiewicz @ 2011-03-25 17:17 UTC (permalink / raw)
To: netdev
Hi,
There are two options for disabling some ipv6 functionality in ipv6 module -
disable and disable_ipv6. The second option is also available as sysctl and
can be switched runtime.
First is nicer because it also prevents apps from creating sockets by using
socket(AF_INET6, ...). Various apps use AF_INET6 socket creation to deterine
if ipv6 is supported on the system. Unfortunately "disable" one doesn't exist
as sysctl and this is a problem.
Is it possible to make "disable" sysctl option, too? Currently there is no
runtime way to disable ipv6 (or I'm unaware of such way).
Thanks,
--
Arkadiusz Miśkiewicz PLD/Linux Team
arekm / maven.pl http://ftp.pld-linux.org/
^ permalink raw reply
* Re: [RFC] usbnet: use eth%d name for known ethernet devices
From: Arnd Bergmann @ 2011-03-25 16:43 UTC (permalink / raw)
To: Alexey Orishko
Cc: Ben Hutchings, Steve Calfee, Michal Nazarewicz, Randy Dunlap,
broonie, lkml, Nicolas Pitre, Greg KH, David Brownell, Alan Cox,
grant.likely, Linux USB list, andy.green, netdev,
Benjamin Herrenschmidt, roger.quadros, Jaswinder Singh, patches
In-Reply-To: <AANLkTikEJi7bg5GG3Ero_Poe9F6WiKo8rW+Y1KtxhQ4B@mail.gmail.com>
On Friday 25 March 2011, Alexey Orishko wrote:
> On Fri, Mar 25, 2011 at 12:57 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > That would be a different way of looking at it. FLAG_POINTTOPOINT
> > describes what the device is (a USB cable connecting two hosts), and
> > that flag can be used for various things, where the only thing
> > we currently do is the netif naming.
> >
>
> For example, cdc_ether and cdc-ncm drivers can be used in different use cases:
> a) when device terminates the IP traffic
> or
> b) where device is a wireless router.
>
> In both cases ethernet frames are sent over usb cable and terminated
> in device (eth header stripped), so it is point-to-point link for ethernet, but
> looking from IP layer is not p2p link for case b).
>
> Please, explain, based on your idea, do we set this flag in both cases or not?
> Do you want to use the same netif name for both use cases described above?
>
Most importantly, I want to keep the current rules, so that nothing breaks
for existing users.
For cdc_ether and cdc-ncm devices, my patch always sets both FLAG_ETHER and
FLAG_POINTTOPOINT, because the driver has no way to find out which of the
two is actually there.
The usb-net core driver interprets this as meaning that it has to decide
for the name based on something else, and that happens to be the presence
of a globally assigned MAC address. I don't think that keying off the MAC
address here is a particularly good idea, but that's what the driver has
always done.
Arnd
^ permalink raw reply
* Re: [RFC] usbnet: use eth%d name for known ethernet devices
From: Alexey Orishko @ 2011-03-25 16:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Hutchings, Steve Calfee, Michal Nazarewicz, Randy Dunlap,
broonie, lkml, Nicolas Pitre, Greg KH, David Brownell, Alan Cox,
grant.likely, Linux USB list, andy.green, netdev,
Benjamin Herrenschmidt, roger.quadros, Jaswinder Singh, patches
In-Reply-To: <201103251257.32714.arnd@arndb.de>
On Fri, Mar 25, 2011 at 12:57 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> That would be a different way of looking at it. FLAG_POINTTOPOINT
> describes what the device is (a USB cable connecting two hosts), and
> that flag can be used for various things, where the only thing
> we currently do is the netif naming.
>
For example, cdc_ether and cdc-ncm drivers can be used in different use cases:
a) when device terminates the IP traffic
or
b) where device is a wireless router.
In both cases ethernet frames are sent over usb cable and terminated
in device (eth header stripped), so it is point-to-point link for ethernet, but
looking from IP layer is not p2p link for case b).
Please, explain, based on your idea, do we set this flag in both cases or not?
Do you want to use the same netif name for both use cases described above?
/alexey
^ permalink raw reply
* Re: [RFC v3 5/6] j1939: add documentation and MAINTAINERS
From: Kurt Van Dijck @ 2011-03-25 13:55 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4D8623BE.2080807-fJ+pQTUTwRTk1uMJSBkQmQ@public.gmane.org>
On Sun, Mar 20, 2011 at 04:56:46PM +0100, Oliver Hartkopp wrote:
> On 14.03.2011 14:59, Kurt Van Dijck wrote:
>
> Hello Kurt,
hello Oliver :-)
>
> even after our F2F-discussion on the Embedded World i'm still not convinced,
ok
Are you aware of the fact that, since your experience with dynamic addressing
is limited, ignoring this completely will leave you yet with a working,
operational J1939 stack?
When address claiming is not in place, just work with the static addresses. It'll
work. The addition of address claiming in kernel just looses you a few
cpu clock cycles.
At least, that's the intention of the API.
The patches I sent have proven to have some faults yet. I'm again working on an
update.
> why it should be a good idea to handle all the address claiming process inside
> the kernel.
IMO, it's a good idea to have the kernel support the address claiming process.
Why:
* IMO, J1939 implicitely puts address claiming below Transport protocol.
As such, you can't really put Transport protocol in kernel & address claim
in userspace.
* In kernel, an ECU can 'atomically' switch addresses,
AND have pending transport sessions (in kernel) use the new address.
* in kernel, a sendto() can be held (block) during address claim delays (not yet
implemented).
* In kernel, you can let userspace participate on a J1939 bus _correctly_
in an imperative way, rather than the volutary userspace library setups.
The above are arguments to minimize the required overhead in userspace for
supporting address claiming.
I don't want the kernel to handle _all_ the address claiming.
Why:
* address claiming _is_ a policy related thing. I don't put policies
in kernel. I learned that as soon that a policy is implemented, our customer
wants it different.
But our customer should _not_ try modifying the non-policy related part of
address claiming.
Think of this scenario: we put only transport sessions in kernel.
So, you instruct the kernel to send few kB on j1939 bus, from (A)0x80.
What will happen when (B)0x80 kicks (A) from its 0x80 using address claiming.
(A) will use (eg.) 0x82. But your transport session is still pending,
and using 0x80 ==>> illegal.
My implementation lets the kernel follow the address claiming traffic, that was
initiated from userspace, so it can handle this situation correctly.
Also note that this is the smallest example to make the point. More elaborated and
complex examples exist.
The important thing, IMO, is how this is controlled from userspace. Yes, you
can instruct the kernel to do magic trickery then, but my implementation
uses the regular API's for this. I just accepted the fact that the 64bit name
is the real reference to indicate an ECU. So I put that in the sockaddr_can too.
>
> Besides the fact, that other j1939 implementation are *completely* implemented
> in userspace (and can cope with the time restrictions),
They probably do, on a voluntary basis.
When spread across different processes cooperating to the same ECU,
they have no means to guarantee anything. So, every application should
carefully respect the guidelines for the userspace library.
And after that is accomplished, do they take care of the following scenario:
Process A sends a PGN regularly
Process B deals with address claims.
When process B emits a new address, process A should _not_ send new PGN's for
250msec. That is the 'time restriction'
However, their interprocess communication will race with the in-kernel CAN queue's.
So, A may emit PGN's after the address claim, but before the 'address notification'
I don't see a proper way to deal with it.
> i do not see why you
> put the address claiming into the kernel and not only the transport layer stuff.
see above.
>
> The address claiming can be compared to something like DHCP or DNS in the
> internet protocol world, that are both handled and implemented in userspace
> apps or userspace libraries.
Yes and no.
Yes: DHCP also deals with 'dynamic addresses'. The policies applied there could
be used for J1939 too.
No: DHCP operates in a master-slave way, J1939 does not.
DHCP does not handle conflicts, since the master should not give conflicting leases.
J1939 must deal with conflicts.
A proper comparison on IP would be to eliminate the DHCP server, and let each
client 'just use an IP', and when another ethernet card happens to have chosen
the exact same IP, the winner is decided (on both ends) based on the MAC address.
This would introduce loads of problems in IP too.
The atomic operation & conflict handling are kernel parts IMHO.
another comparison would be ARP, where each node holds its own table, carefully
built from passing network traffic.
The 64bit 'name' is not a name as in DNS, it's a unique ID.
>
> E.g. these bits from the documentation look like you are starting some kind of
> 'addressing service' daemon:
Yep, to implement the 'address choosing' policy.
I'm still testing all J1939 requirements here, but when this is ready, I feel
I will need to publish such program.
>
> > +4.1 rtnetlink interface
> > +
> > + Per default j1939 is not active. Specifying can_ifindex != 0 in bind(2)
> > + or connect(2) needs an active j1939 on that interface. You must have done
> > + $ ip link set canX j1939 on
> > + on that interface.
> > +
> > + $ ip link set canX j1939 down
> > + disables j1939 on canX.
>
> You are activating the 'addressing service' on specific CAN interfaces.
Since no autoprobing of protocols is possible on CAN (in contrast to Ethernet).
>
> Then you suggest to attach static and/or dynamic addresses to the interface.
Correction: you assign a static address, or a 'name' for use with dynamic addressing.
the name for dynamic addressing is not a 'dynamic address'.
>
> > + Assigning addresses is done via
> > + $ ip addr add dev canX j1939 0xXX
> > + statically or
> > + $ ip addr add dev canX j1939 name 0xXX
> > + dynamically. In the latter case, address claiming must take place
> > + before other traffic can leave.
>
> like you would have using DHCP/DNS (adapted for j1939) ...
The userspace part for address claiming (in fact, the right CAN frames should appear
to fullfill the address claiming requirements) can then bind to the name, and claim
addresses for it. The kernel will reccognize those, and act upon them. The kernel
will at no point initiate an address claim.
>
> > + Removing addresses is done similarly via
> > + $ ip addr del dev canX j1939 0xXX
> > + $ ip addr del dev canX j1939 name 0xXX
> > +
> > + A static address cannot be assigned together with a 64bit name.
>
> Ah. You provide two kernel interfaces
Yep, because there are two distinct use cases to address.
With dynamic addressing, the 8bit address is just a temporary placeholder, which may
change in the future. This makes a bad discriminator to differentiate between hosts.
therefore, to specify a host to the kernel (as source or destination), the 8bit
address is irrelevant at that point in time.
Therefore, the kernel _needs_ knowledge of the 64bit name, just to provide a
decent interface to userspace for source/destination identification.
> instead of handling the address claiming
> in userspace and provide only one simple (static) interface to the kernel.
And this is no option IMO.
Just above, I indicated that the kernel needs the 64bit name. Pairing with the 8bit
address in userspace only will:
* add a serious amount of userspace code & userspace timing requirements.
* create a complex library/IPC API.
* not solve the case for pending transport sessions.
* still unspecified how the kernel should treat messages that have specified
a 64bit destination name with different 8bit destination address.
IMO, doing these things in userspace are unnecessarily complicated.
>
> This artifact brings this fact out again:
>
> > + can_addr.j1939.name contains the 64-bit J1939 NAME.
> > +
> > + can_addr.j1939.addr contains the source address.
> > +
> > + When sending data, the source address is applied as follows: If
> > + can_addr.j1939.name != 0 the NAME is looked up by the kernel and the
> > + corresponding Source Address is used. If can_addr.j1939.name == 0,
> > + can_addr.j1939.addr is used.
>
> Yes. You are providing two programming interfaces to the kernel that can be
> used exclusive-OR only.
ack.
>
> As other j1939 implementations implement the address claiming in userspace
> too, there's no necessity to implement this inside the kernel. DHCP and DNS
> can also lead to address changes and some 'new j1939 addressing daemon' could
> be implemented in a way that allows to tell registered j1939 apps address
> changes in a fast way (trigger/signal/select/whatever). I don't know how much
This is like saying: the race condition could be improved. But it's still there, isn't it?
> of the 4838 lines of code from your RFC can be removed - but i assume it would
+/- 700
which will move to every userspace application, and they'd still not be correct.
> vastly reduce the complexity of your posted j1939 implementation.
>
> If you would implement only the j1939 transport layer stuff (using static
> addressing) together with some userspace 'address claiming' daemon and/or
> library (compared to DNS) this would make more sense to me. Especially it
> makes the kernel API clear and simple(!).
and crippled.
Being clear and simple is no justification for introducing race conditions!
> As a side-effect this would remove
> the need for a separate system-wide configuration
Why is that a problem. I look at it as a powerfull administrative tool.
The fact that J1939 is the first CAN protocol with a 'real' addressing scheme
seems a major problem.
> which would need to be added
> with your suggested netlink extensions in af_can.c, iproute2
It works, and IMO fits in the policies.
> and the other adaptions to the current code to extend the sockaddr_can in size.
Sooner or later, when CAN protocols are added, the sockaddr_can will grow.
Not letting this grow is equivalent of saying that no CAN protocols will be added.
This fits in the current policies. why do BSD socket functions invented
the af_family & socklen_t stuff. Because they faced this problem before.
Putting J1939 inside AF_CAN implies handling this kind of stuff. And I handled it.
I haven't recompiled my old can tools for ages now, but they still work.
>
> I know you're a fan of j1939 address claiming inside the kernel
I'm no fan. I only respect the fact that it's the only right place.
> but IMO this does not fit other networking addressing concepts inside the kernel
IPv6 automatic router detection?
At least, I disagree.
> and messes up the kernel API for j1939, sigh.
I don't think my proposed API is a mess. It my way of addressing existing problems.
The fact that some userspace library for tons of dollars does not solve my problem
is IMO no reason to make the same fault.
>
> If it makes sense to you, i would offer my help to implement the address
> claiming daemon based on CAN_RAW and CAN_BCM ... 8-)
Thanks for the offer.
I have no clue yet how to do the necessary IPC in a way that userspace would
be easy to work with _UNDER ALL CIRCUMSTANCES_ (sorry for the capitals).
>
> Anyway: Thanks for you contribution!
Thanks for looking at the API.
For what it's worth, since the discussion started, I also think the API has improved
a lot. Thanks for that!
Kurt
>
> Oliver
^ permalink raw reply
* Re: [PATCH] myri10ge: small rx_done refactoring
From: Andrew Gallatin @ 2011-03-25 13:03 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev, Brice Goglin, Stephen Hemminger, David Howells,
Ben Hutchings
In-Reply-To: <20110325112123.GA6218@redhat.com>
On 03/25/11 07:21, Stanislaw Gruszka wrote:
> Avoid theoretical race condition regarding accessing dev->features
> NETIF_F_LRO flag, which is illustrated below.
>
> CPU1 CPU2
>
> myri10ge_clean_rx_done(): myri10ge_set_flags():
> or
> myri10ge_set_rx_csum():
>
> if (dev->features& NETIF_F_LRO)
> setup lro
> dev->features |= NETIF_F_LRO
> or
> dev->features&= ~NETIF_F_LRO;
> if (dev->features& NETIF_F_LRO)
> flush lro
>
> On the way reduce myri10ge_rx_done() number of arguments and calls by
> moving mgp->small_bytes check into that function. That reduce code size
>
> from:
> text data bss dec hex filename
> 36644 248 100 36992 9080 drivers/net/myri10ge/myri10ge.o
>
> to:
> text data bss dec hex filename
> 36037 247 100 36384 8e20 drivers/net/myri10ge/myri10ge.o
>
> on my i686 system, what should also make myri10ge_clean_rx_done()
> being faster.
>
> Signed-off-by: Stanislaw Gruszka<sgruszka@redhat.com>
Thank you very much!
Acked by: Andrew Gallatin <gallatin@myri.com>
^ permalink raw reply
* Re: [RFC] usbnet: use eth%d name for known ethernet devices
From: Arnd Bergmann @ 2011-03-25 11:57 UTC (permalink / raw)
To: Alexey Orishko
Cc: Ben Hutchings, Steve Calfee, Michal Nazarewicz, Randy Dunlap,
broonie, lkml, Nicolas Pitre, Greg KH, David Brownell, Alan Cox,
grant.likely, Linux USB list, andy.green, netdev,
Benjamin Herrenschmidt, roger.quadros, Jaswinder Singh, patches
In-Reply-To: <AANLkTikZ76VXdPf46FHxhCyf3n4rNZOQW4Rp2yUZWP2h@mail.gmail.com>
On Thursday 24 March 2011, Alexey Orishko wrote:
> On Thu, Mar 24, 2011 at 2:15 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> >
> > The approach taken here is to flag whether a device might be a
> > point-to-point link with the new FLAG_PTP setting in the usbnet
> > driver_info. A driver can set both FLAG_PTP and FLAG_ETHER if
> > it is not sure (e.g. cdc_ether), or just one of the two.
>
> > The usbnet framework only looks at the MAC address for device
> > naming if both flags are set, otherwise it trusts the flag.
>
> Should this paragraph above be a clue for the flag name?
> Sorry for late comment, but having flag called FLAG_POINTTOPOINT is really
> confusing. ptp, p2p terms are heavily used and will mislead folks.
>
> Would it be better to call it something like IGNORE_MAC_ADDRESS if this is the
> feature you are targeting?
That would be a different way of looking at it. FLAG_POINTTOPOINT
describes what the device is (a USB cable connecting two hosts), and
that flag can be used for various things, where the only thing
we currently do is the netif naming.
FLAG_IGNORE_MAC_ADDRESS as you suggest describes the implementation
of the device naming, not why that is done.
The intent here was to some something that makes sense next to
FLAG_ETHER, FLAG_WWAN and FLAG_WLAN. I think FLAG_POINTTOPOINT
describes this best, although I'd also be happy with FLAG_PTP,
FLAG_P2P, FLAG_CABLE or FLAG_USBCABLE.
Arnd
^ permalink raw reply
* Re: dccp test-tree [RFC] [Patch 1/1] dccp: Only activate NN values after receiving the Confirm option
From: Gerrit Renker @ 2011-03-25 11:39 UTC (permalink / raw)
To: Samuel Jero; +Cc: dccp, netdev
In-Reply-To: <4D88000F.80704@ohio.edu>
| Below is the very last patch you added to the test tree. I believe both
| of the changes made are not needed. See comments inline.
| > dccp ccid-2: replace remaining references to local Sequence Window value
| >
Thank you for the explanation. I have removed this patch from the test tree
and resynchronized.
^ permalink raw reply
* [PATCH] myri10ge: small rx_done refactoring
From: Stanislaw Gruszka @ 2011-03-25 11:21 UTC (permalink / raw)
To: netdev
Cc: Andrew Gallatin, Brice Goglin, Stephen Hemminger, David Howells,
Ben Hutchings
Avoid theoretical race condition regarding accessing dev->features
NETIF_F_LRO flag, which is illustrated below.
CPU1 CPU2
myri10ge_clean_rx_done(): myri10ge_set_flags():
or
myri10ge_set_rx_csum():
if (dev->features & NETIF_F_LRO)
setup lro
dev->features |= NETIF_F_LRO
or
dev->features &= ~NETIF_F_LRO;
if (dev->features & NETIF_F_LRO)
flush lro
On the way reduce myri10ge_rx_done() number of arguments and calls by
moving mgp->small_bytes check into that function. That reduce code size
from:
text data bss dec hex filename
36644 248 100 36992 9080 drivers/net/myri10ge/myri10ge.o
to:
text data bss dec hex filename
36037 247 100 36384 8e20 drivers/net/myri10ge/myri10ge.o
on my i686 system, what should also make myri10ge_clean_rx_done()
being faster.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
Thanks to Stephen, David and Ben for pointing missing ACCESS_ONCE() issue
and teach me multi-thread programming.
drivers/net/myri10ge/myri10ge.c | 37 +++++++++++++++++++++++--------------
1 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
index a7f2eed..4023363 100644
--- a/drivers/net/myri10ge/myri10ge.c
+++ b/drivers/net/myri10ge/myri10ge.c
@@ -1312,17 +1312,26 @@ myri10ge_unmap_rx_page(struct pci_dev *pdev,
* page into an skb */
static inline int
-myri10ge_rx_done(struct myri10ge_slice_state *ss, struct myri10ge_rx_buf *rx,
- int bytes, int len, __wsum csum)
+myri10ge_rx_done(struct myri10ge_slice_state *ss, int len, __wsum csum,
+ int lro_enabled)
{
struct myri10ge_priv *mgp = ss->mgp;
struct sk_buff *skb;
struct skb_frag_struct rx_frags[MYRI10GE_MAX_FRAGS_PER_FRAME];
- int i, idx, hlen, remainder;
+ struct myri10ge_rx_buf *rx;
+ int i, idx, hlen, remainder, bytes;
struct pci_dev *pdev = mgp->pdev;
struct net_device *dev = mgp->dev;
u8 *va;
+ if (len <= mgp->small_bytes) {
+ rx = &ss->rx_small;
+ bytes = mgp->small_bytes;
+ } else {
+ rx = &ss->rx_big;
+ bytes = mgp->big_bytes;
+ }
+
len += MXGEFW_PAD;
idx = rx->cnt & rx->mask;
va = page_address(rx->info[idx].page) + rx->info[idx].page_offset;
@@ -1341,7 +1350,7 @@ myri10ge_rx_done(struct myri10ge_slice_state *ss, struct myri10ge_rx_buf *rx,
remainder -= MYRI10GE_ALLOC_SIZE;
}
- if (dev->features & NETIF_F_LRO) {
+ if (lro_enabled) {
rx_frags[0].page_offset += MXGEFW_PAD;
rx_frags[0].size -= MXGEFW_PAD;
len -= MXGEFW_PAD;
@@ -1463,7 +1472,7 @@ myri10ge_clean_rx_done(struct myri10ge_slice_state *ss, int budget)
{
struct myri10ge_rx_done *rx_done = &ss->rx_done;
struct myri10ge_priv *mgp = ss->mgp;
- struct net_device *netdev = mgp->dev;
+
unsigned long rx_bytes = 0;
unsigned long rx_packets = 0;
unsigned long rx_ok;
@@ -1474,18 +1483,18 @@ myri10ge_clean_rx_done(struct myri10ge_slice_state *ss, int budget)
u16 length;
__wsum checksum;
+ /*
+ * Prevent compiler from generating more than one ->features memory
+ * access to avoid theoretical race condition with functions that
+ * change NETIF_F_LRO flag at runtime.
+ */
+ bool lro_enabled = ACCESS_ONCE(mgp->dev->features) & NETIF_F_LRO;
+
while (rx_done->entry[idx].length != 0 && work_done < budget) {
length = ntohs(rx_done->entry[idx].length);
rx_done->entry[idx].length = 0;
checksum = csum_unfold(rx_done->entry[idx].checksum);
- if (length <= mgp->small_bytes)
- rx_ok = myri10ge_rx_done(ss, &ss->rx_small,
- mgp->small_bytes,
- length, checksum);
- else
- rx_ok = myri10ge_rx_done(ss, &ss->rx_big,
- mgp->big_bytes,
- length, checksum);
+ rx_ok = myri10ge_rx_done(ss, length, checksum, lro_enabled);
rx_packets += rx_ok;
rx_bytes += rx_ok * (unsigned long)length;
cnt++;
@@ -1497,7 +1506,7 @@ myri10ge_clean_rx_done(struct myri10ge_slice_state *ss, int budget)
ss->stats.rx_packets += rx_packets;
ss->stats.rx_bytes += rx_bytes;
- if (netdev->features & NETIF_F_LRO)
+ if (lro_enabled)
lro_flush_all(&rx_done->lro_mgr);
/* restock receive rings if needed */
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 2/2] virtio_net: remove send completion interrupts and avoid TX queue overrun through packet drop
From: Rusty Russell @ 2011-03-25 4:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Shirley Ma, Herbert Xu, davem, kvm, netdev
In-Reply-To: <20110324142822.GD12958@redhat.com>
On Thu, 24 Mar 2011 16:28:22 +0200, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Mar 24, 2011 at 11:00:53AM +1030, Rusty Russell wrote:
> > > With simply removing the notify here, it does help the case when TX
> > > overrun hits too often, for example for 1K message size, the single
> > > TCP_STREAM performance improved from 2.xGb/s to 4.xGb/s.
> >
> > OK, we'll be getting rid of the "kick on full", so please delete that on
> > all benchmarks.
> >
> > Now, does the capacity check before add_buf() still win anything? I
> > can't see how unless we have some weird bug.
> >
> > Once we've sorted that out, we should look at the more radical change
> > of publishing last_used and using that to intuit whether interrupts
> > should be sent. If we're not careful with ordering and barriers that
> > could introduce more bugs.
>
> Right. I am working on this, and trying to be careful.
> One thing I'm in doubt about: sometimes we just want to
> disable interrupts. Should still use flags in that case?
> I thought that if we make the published index 0 to vq->num - 1,
> then a special value in the index field could disable
> interrupts completely. We could even reuse the space
> for the flags field to stick the index in. Too complex?
Making the index free-running avoids the "full or empty" confusion, plus
offers and extra debugging insight.
I think that if they really want to disable interrrupts, the flag should
still work, and when the client accepts the "publish last_idx" feature
they are accepting that interrupts may be omitted if they haven't
updated last_idx yet.
> > Anything else on the optimization agenda I've missed?
> >
> > Thanks,
> > Rusty.
>
> Several other things I am looking at, wellcome cooperation:
> 1. It's probably a good idea to update avail index
> immediately instead of upon kick: for RX
> this might help parallelism with the host.
Yes, once we've done everything else, we should measure this. It makes
sense.
> 2. Adding an API to add a single buffer instead of s/g,
> seems to help a bit.
This goes last, since it's kind of an ugly hack, but all internal to
Linux if we decide it's a win.
> 3. For TX sometimes we free a single buffer, sometimes
> a ton of them, which might make the transmit latency
> vary. It's probably a good idea to limit this,
> maybe free the minimal number possible to keep the device
> going without stops, maybe free up to MAX_SKB_FRAGS.
This kind of heuristic is going to be quite variable depending on
circumstance, I think, so it's a lot of work to make sure we get it
right.
> 4. If the ring is full, we now notify right after
> the first entry is consumed. For TX this is suboptimal,
> we should try delaying the interrupt on host.
Lguest already does that: only sends an interrupt when it's run out of
things to do. It does update the used ring, however, as it processes
them.
This seems sensible to me, but needs to be measured separately as well.
> More ideas, would be nice if someone can try them out:
> 1. We are allocating/freeing buffers for indirect descriptors.
> Use some kind of pool instead?
> And we could preformat part of the descriptor.
We need some poolish mechanism for virtio_blk too; perhaps an allocation
callback which both can use (virtio_blk to alloc from a pool, virtio_net
to recycle?).
Along similar lines to preformatting, we could actually try to prepend
the skb_vnet_hdr to the vnet data, and use a single descriptor for the
hdr and the first part of the packet.
Though IIRC, qemu's virtio barfs if the first descriptor isn't just the
hdr (barf...).
> 2. I didn't have time to work on virtio2 ideas presented
> at the kvm forum yet, any takers?
I didn't even attend. But I think that virtio is moribund for the
moment; there wasn't enough demand and it's clear that there are
optimization unexplored in virtio1.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 2/2] virtio_net: remove send completion interrupts and avoid TX queue overrun through packet drop
From: Rusty Russell @ 2011-03-25 4:51 UTC (permalink / raw)
To: Shirley Ma, Michael S. Tsirkin; +Cc: Herbert Xu, davem, kvm, netdev
In-Reply-To: <1300988809.3441.48.camel@localhost.localdomain>
On Thu, 24 Mar 2011 10:46:49 -0700, Shirley Ma <mashirle@us.ibm.com> wrote:
> On Thu, 2011-03-24 at 16:28 +0200, Michael S. Tsirkin wrote:
> > Several other things I am looking at, wellcome cooperation:
> > 1. It's probably a good idea to update avail index
> > immediately instead of upon kick: for RX
> > this might help parallelism with the host.
> Is that possible to use the same idea for publishing last used idx to
> publish avail idx? Then we can save guest iowrite/exits.
Yes, it should be symmetrical. Test independently of course, but the
same logic applies.
Thanks!
Rusty.
^ permalink raw reply
* Re: slow tcp connect when using IPsec
From: Steffen Klassert @ 2011-03-25 8:58 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110325.012749.235670347.davem@davemloft.net>
On Fri, Mar 25, 2011 at 01:27:49AM -0700, David Miller wrote:
>
> > So now I know why it did not behave as expected, but unfortunately I
> > still don't know why it magically started to work after 20
> > seconds...
>
> After some time, TCP will mark routing path as having trouble, then it
> will relookup the route. At this point source and dest will no longer
> be wildcarded in the socket, and thus neither will be the resulting
> route keys in the relooked-up route.
>
> Look for the dst_negative_advice() paths to see where this happens.
Ok, I see. Thanks for the explanation.
^ permalink raw reply
* Re: [PATCH] route: Take the right src and dst addresses in ip_route_newports
From: David Miller @ 2011-03-25 8:29 UTC (permalink / raw)
To: steffen.klassert; +Cc: netdev
In-Reply-To: <20110325064203.GF1290@secunet.com>
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Fri, 25 Mar 2011 07:42:03 +0100
> When we set up the flow informations in ip_route_newports(), we take the
> address informations from the the rt_key_src and rt_key_dst fields of the
> rtable. They appear to be empty. So take the address informations from
> rt_src and rt_dst instead. This issue was introduced by
> commit 5e2b61f78411be25f0b84f97d5b5d312f184dfd1
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
Applied, thank you.
^ permalink raw reply
* Re: slow tcp connect when using IPsec
From: David Miller @ 2011-03-25 8:27 UTC (permalink / raw)
To: steffen.klassert; +Cc: netdev
In-Reply-To: <20110325064116.GE1290@secunet.com>
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Fri, 25 Mar 2011 07:41:16 +0100
> commit 5e2b61f78411be25f0b84f97d5b5d312f184dfd1
> Author: David S. Miller <davem@davemloft.net>
> Date: Fri Mar 4 21:47:09 2011 -0800
>
> ipv4: Remove flowi from struct rtable.
>
> Some time and a lot of trace_printks later I found that we set up
> the flow informations without source _and_ destination address in
> ip_route_newports(). That is because we take the address informations
> from the the rt_key_src and rt_key_dst fields of the rtable here
> and they appear to be empty. If I restore the behaviour before the bisected
> commit by taking the address informations from rt_src and rt_dst the issue
> is gone.
Indeed, it is wrong to use the key values, since they can be
wildcards. Thanks for tracking this down.
> So now I know why it did not behave as expected, but unfortunately I
> still don't know why it magically started to work after 20
> seconds...
After some time, TCP will mark routing path as having trouble, then it
will relookup the route. At this point source and dest will no longer
be wildcarded in the socket, and thus neither will be the resulting
route keys in the relooked-up route.
Look for the dst_negative_advice() paths to see where this happens.
^ permalink raw reply
* Re: synchronous netlink
From: Patrick McHardy @ 2011-03-25 7:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110324.205549.186286411.davem@davemloft.net>
Am 25.03.2011 04:55, schrieb David Miller:
>
> Patrick, just a reminder that we have to deal with that
> connector regression that exists ever since you added code
> to netlink which assumes synchronous processing.
Yes, I didn't get to that yet, sorry. I'm going to take care
of it this weekend.
^ permalink raw reply
* Re: On Linux rate limiting and the magic value of 34.64 Gbps...
From: Eric Dumazet @ 2011-03-25 7:17 UTC (permalink / raw)
To: Maciej Żenczykowski; +Cc: Linux NetDev, David Miller
In-Reply-To: <AANLkTi=UTcZ8Q-ULk4CAkbZztYKkDeZHK55LHVDRFH2r@mail.gmail.com>
Le vendredi 25 mars 2011 à 00:14 -0700, Maciej Żenczykowski a écrit :
> Hey,
>
> The Linux rate limiting code relies on the rate field of struct tc_ratespec.
> This field is a __u32 and measures rate in "bytes per second".
>
> This basically means maximum representable rate is 4GB per second.
> This is equivalent to 34.36 Gbps and I just ran across that limit with
> 40 Gbps (which behaves like 5.64 Gbps because of overflow/truncation).
> Seeing as this structure is exposed to userspace for both read and
> write via various netlink paths (in cbq, htb, tbf, etc...) there
> doesn't seem to be a particularly clean way to increase the size of
> this field. While there is a __reserved field that could
> theoretically be repurposed as some sort of rate bit shift register, I
> don't think we can rely on __reserved having been set to zero by
> userspace (by older programs), and we will definitely see problems
> with output by programs (tc) that don't expect to have to parse this
> field to output an understandable rate limit...
>
> Anybody have any bright ideas?
Well, netlink is extensible, so we can easily add a new structure, with
64bit fields if necessary.
We did that for 64bit stats already.
^ permalink raw reply
* On Linux rate limiting and the magic value of 34.64 Gbps...
From: Maciej Żenczykowski @ 2011-03-25 7:14 UTC (permalink / raw)
To: Linux NetDev; +Cc: Eric Dumazet, David Miller
Hey,
The Linux rate limiting code relies on the rate field of struct tc_ratespec.
This field is a __u32 and measures rate in "bytes per second".
This basically means maximum representable rate is 4GB per second.
This is equivalent to 34.36 Gbps and I just ran across that limit with
40 Gbps (which behaves like 5.64 Gbps because of overflow/truncation).
Seeing as this structure is exposed to userspace for both read and
write via various netlink paths (in cbq, htb, tbf, etc...) there
doesn't seem to be a particularly clean way to increase the size of
this field. While there is a __reserved field that could
theoretically be repurposed as some sort of rate bit shift register, I
don't think we can rely on __reserved having been set to zero by
userspace (by older programs), and we will definitely see problems
with output by programs (tc) that don't expect to have to parse this
field to output an understandable rate limit...
Anybody have any bright ideas?
Thanks,
Maciej
^ permalink raw reply
* Re: [PATCH] Fix IP timestamp option (IPOPT_TS_PRESPEC) handling in ip_options_echo()
From: Jan Lübbe @ 2011-03-25 6:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110324.162005.71589730.davem@davemloft.net>
Hi!
On Thu, 2011-03-24 at 16:20 -0700, David Miller wrote:
> From: Jan Luebbe <jluebbe@debian.org>
> Date: Thu, 24 Mar 2011 18:44:22 +0100
>
> > - if (soffset + 8 <= optlen) {
> > + if (soffset + 7 <= optlen) {
>
> I don't see how you can legally reduce this check from 8 to 7 bytes.
>
> > + if (inet_addr_type(dev_net(skb_dst(skb)->dev), addr) != RTN_UNICAST) {
> > dopt->ts_needtime = 1;
> > soffset += 8;
> > }
>
> Yet keep this code which advances soffset by 8.
The encoding of soffset is a bit unusual, in that the option is 'full'
when soffset = optlen + 1. We need to keep advancing soffset by 8
because that's really the amount of data per entry.
In ip_options_compile, near 'case IPOPT_TS_PRESPEC:' (line 382 in
2.6.38), we have:
case IPOPT_TS_PRESPEC:
if (optptr[2]+7 > optptr[1]) {
pp_ptr = optptr + 2;
goto error;
}
opt->ts = optptr - iph;
{
__be32 addr;
memcpy(&addr, &optptr[optptr[2]-1], 4);
if (inet_addr_type(net, addr) == RTN_UNICAST)
break;
if (skb)
timeptr = (__be32*)&optptr[optptr[2]+3];
}
opt->ts_needtime = 1;
optptr[2] += 8;
break;
Here optptr[1] matches optlen from _echo and optptr[2] is soffset.
When we use soffset to index into the packet we substract 1. See the
memcpy in my patch which reads from the packet and also the memcpys in
_compile around line 390.
If we checked soffset + 8 <= optlen, we abort updating the timestamp
when exactly space for one entry remains.
Also, I don't think writing to unallocated memory is possible when the
IP header in the echoed packet are doesn't have enough space for the
length indicated by optlen, as we clear dopt with memset(dopt, 0,
sizeof(struct ip_options)) so it dopt must have the right size already.
Best regards,
Jan
^ permalink raw reply
* [PATCH] route: Take the right src and dst addresses in ip_route_newports
From: Steffen Klassert @ 2011-03-25 6:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110325064116.GE1290@secunet.com>
When we set up the flow informations in ip_route_newports(), we take the
address informations from the the rt_key_src and rt_key_dst fields of the
rtable. They appear to be empty. So take the address informations from
rt_src and rt_dst instead. This issue was introduced by
commit 5e2b61f78411be25f0b84f97d5b5d312f184dfd1
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
include/net/route.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/route.h b/include/net/route.h
index dc10244..f88429c 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -270,8 +270,8 @@ static inline struct rtable *ip_route_newports(struct rtable *rt,
struct flowi4 fl4 = {
.flowi4_oif = rt->rt_oif,
.flowi4_mark = rt->rt_mark,
- .daddr = rt->rt_key_dst,
- .saddr = rt->rt_key_src,
+ .daddr = rt->rt_dst,
+ .saddr = rt->rt_src,
.flowi4_tos = rt->rt_tos,
.flowi4_proto = protocol,
.fl4_sport = sport,
--
1.7.0.4
^ permalink raw reply related
* slow tcp connect when using IPsec
From: Steffen Klassert @ 2011-03-25 6:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev
I'm fighting with a strange behaviour since a some days.
When I try to send tcp data over an IPsec tunnel, the tcp connect hangs
for about 20 seconds before it finally sends out the SYN packet.
This happens just on tcp with IPsec. When I bind the connection to
a specific local port, everything works fine. After some time of
unsuccessful debugging, I bisected this issue down to
commit 5e2b61f78411be25f0b84f97d5b5d312f184dfd1
Author: David S. Miller <davem@davemloft.net>
Date: Fri Mar 4 21:47:09 2011 -0800
ipv4: Remove flowi from struct rtable.
Some time and a lot of trace_printks later I found that we set up
the flow informations without source _and_ destination address in
ip_route_newports(). That is because we take the address informations
from the the rt_key_src and rt_key_dst fields of the rtable here
and they appear to be empty. If I restore the behaviour before the bisected
commit by taking the address informations from rt_src and rt_dst the issue
is gone. So now I know why it did not behave as expected, but unfortunately
I still don't know why it magically started to work after 20 seconds...
I'll send the patch that fixed the issue in replay to this mail.
Steffen
^ permalink raw reply
* synchronous netlink
From: David Miller @ 2011-03-25 3:55 UTC (permalink / raw)
To: kaber; +Cc: netdev
Patrick, just a reminder that we have to deal with that
connector regression that exists ever since you added code
to netlink which assumes synchronous processing.
Thanks.
^ permalink raw reply
* Re: [PATCH] ipv4: fix fib metrics
From: Alessandro Suardi @ 2011-03-25 1:25 UTC (permalink / raw)
To: Kyle Moffett; +Cc: Eric Dumazet, David Miller, linux-kernel, netdev
In-Reply-To: <AANLkTi=hgOcwO8ojQBtmqbROS1qrhm5HCmq7tRxesc=6@mail.gmail.com>
On Fri, Mar 25, 2011 at 2:04 AM, Alessandro Suardi
<alessandro.suardi@gmail.com> wrote:
> On Fri, Mar 25, 2011 at 1:53 AM, Kyle Moffett <kyle@moffetthome.net> wrote:
>> On Thu, Mar 24, 2011 at 20:12, Alessandro Suardi
>> <alessandro.suardi@gmail.com> wrote:
>>> On Thu, Mar 24, 2011 at 11:44 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>> Le jeudi 24 mars 2011 à 15:36 -0700, David Miller a écrit :
>>>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>>>> Date: Thu, 24 Mar 2011 23:32:26 +0100
>>>>>
>>>>> > Then it doesnt work anymore because it parses an ipip field from
>>>>> > ip route get ...
>>>>> >
>>>>> > $ ip ro get 192.168.1.1
>>>>> > 192.168.1.1 dev wlan0 src 192.168.1.21
>>>>> > cache ipid 0x784c mtu 1500 advmss 1460 hoplimit 64
>>>>> >
>>>>> >
>>>>> > Maybe you upgraded iproute2
>>>>>
>>>>> I'm leaning towards app bug too.
>>>>>
>>>>> These default metrics wouldn't get printed before, but now because of
>>>>> how metrics are handled, they will.
>>>>>
>>>>> Userland needs to cope properly with this.
>>>>
>>>>
>>>> BTW, ipip is not always printed (even on old kernels) : One needs to
>>>> actually need ipip generation .
>>>>
>>>> edumazet@edumazet-laptop:~$ ping 4.4.4.4
>>>> PING 4.4.4.4 (4.4.4.4) 56(84) bytes of data.
>>>> ^C
>>>>
>>>> edumazet@edumazet-laptop:~$ ip ro get 4.4.4.4
>>>> 4.4.4.4 dev ppp0 src 10.150.51.210
>>>> cache mtu 1500 advmss 1460 hoplimit 64
>>>>
>>>> edumazet@edumazet-laptop:~$ ping -s 2000 4.4.4.4
>>>> PING 4.4.4.4 (4.4.4.4) 2000(2028) bytes of data.
>>>> ^C
>>>>
>>>> edumazet@edumazet-laptop:~$ ip ro get 4.4.4.4
>>>> 4.4.4.4 dev ppp0 src 10.150.51.210
>>>> cache ipid 0xf99a mtu 1500 advmss 1460 hoplimit 64
>>>>
>>>>
>>>> This on a 2.6.35 kernel
>>>>
>>>> I suspect Alessandro tool had a bug anyway.
>>>
>>> I still contend this is a kernel regression :)
>>>
>>>
>>> vpnc is a custom build from trunk as of June 2010, with openssl support
>>> to talk to my corporate VPN concentrator:
>>>
>> [...snip...]
>>>
>>> My iproute package, on this up-to-date Fedora 14 x86_64, has last been
>>> updated on 20 Nov 2010, and back then I was running 2.6.37-rc2-git4
>>> (I keep around my historical .config files, so I know for sure).
>>>
>>> [root@duff ~]# ip -V
>>> ip utility, iproute2-ss100804
>>> [root@duff ~]# rpm -qf /sbin/ip
>>> iproute-2.6.35-6.fc14.x86_64
>>>
>>> The behavior of this version of 'ip' as invoked by this version of 'vpnc'
>>> is something that has worked for the last 4 months, and isn't working
>>> right now. Furthermore, previous versions of 'ip' in Fedora 14 were
>>> also working with the same 'vpnc', which means it's actually 9 months
>>> minimum of working behavior.
>>>
>>> If some change in the kernel broke my userspace, this usually qualifies
>>> as a regression.
>>>
>>> That said, if you can point me to a working version of iproute with the
>>> current kernel, I have no problem in upgrading it :)
>>
>> Historically you could usually take the text output of "ip route get"
>> and feed it right back to "ip route add", and it would work, but this
>> was never guaranteed.
>>
>> Recently, the "ip route get" command started printing extra statistics
>> (like "ipid") after the other information, but obviously those
>> statistics are not valid for an "ip route add" command.
>>
>> The kernel bug was that the "ip" command was not always getting those
>> statistics from the kernel, so obviously they would not be printed.
>>
>> Unfortunately vpnc still tries to pass the entire output of "ip route
>> get" as arguments to "ip route add"; the latter command reports an
>> error when it gets the statistics from the former command as input.
>>
>> So this is certainly not a kernel bug. At *best* it's an iproute bug,
>> depending on whether or not this is considered valid:
>> RT="$(ip route get [...])"
>> ip route flush
>> ip route add ${RT}
>
> Fair enough, I get it.
>
> Looks like the fix_ip_get_output() function in /etc/vpnc/vpnc-script
> needs to be augmented from the current
>
> sed 's/cache//;s/metric \?[0-9]\+ [0-9]\+//g;s/hoplimit [0-9]\+//g'
>
> to something slightly more comprehensive.
>
>
> Thanks for the explanation - will keep around -git2 for my vpnc
> needs until I get this one sorted out :)
...which didn't take that long - one last bugging question and I'm happily
off to sleep; does ipid always come in the form of 0x followed by four
bytes representing hex values ? In a perhaps inelegant but working way
(I'm now writing through the VPN tunnel),
sed 's/cache//;s/metric \?[0-9]\+ [0-9]\+//g;s/hoplimit
[0-9]\+//g;s/ipid 0x....//g'
appears to be Work For Me (TM).
Thanks loads,
--alessandro
"There's always a siren singing you to shipwreck"
(Radiohead, "There There")
^ permalink raw reply
* [PATCH 2/2] ipv4: Fix nexthop caching wrt. scoping.
From: David Miller @ 2011-03-25 1:14 UTC (permalink / raw)
To: netdev; +Cc: ja
Move the scope value out of the fib alias entries and into fib_info,
so that we always use the correct scope when recomputing the nexthop
cached source address.
Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/ip_fib.h | 6 +++---
net/ipv4/fib_lookup.h | 3 +--
net/ipv4/fib_semantics.c | 15 ++++++++-------
net/ipv4/fib_trie.c | 12 ++++--------
4 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index cd92b92..e5d66ec 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -51,7 +51,6 @@ struct fib_nh {
struct fib_info *nh_parent;
unsigned nh_flags;
unsigned char nh_scope;
- unsigned char nh_cfg_scope;
#ifdef CONFIG_IP_ROUTE_MULTIPATH
int nh_weight;
int nh_power;
@@ -75,9 +74,10 @@ struct fib_info {
struct net *fib_net;
int fib_treeref;
atomic_t fib_clntref;
- int fib_dead;
unsigned fib_flags;
- int fib_protocol;
+ unsigned char fib_dead;
+ unsigned char fib_protocol;
+ unsigned char fib_scope;
__be32 fib_prefsrc;
u32 fib_priority;
u32 *fib_metrics;
diff --git a/net/ipv4/fib_lookup.h b/net/ipv4/fib_lookup.h
index 4ec3238..af0f14a 100644
--- a/net/ipv4/fib_lookup.h
+++ b/net/ipv4/fib_lookup.h
@@ -10,7 +10,6 @@ struct fib_alias {
struct fib_info *fa_info;
u8 fa_tos;
u8 fa_type;
- u8 fa_scope;
u8 fa_state;
struct rcu_head rcu;
};
@@ -29,7 +28,7 @@ extern void fib_release_info(struct fib_info *);
extern struct fib_info *fib_create_info(struct fib_config *cfg);
extern int fib_nh_match(struct fib_config *cfg, struct fib_info *fi);
extern int fib_dump_info(struct sk_buff *skb, u32 pid, u32 seq, int event,
- u32 tb_id, u8 type, u8 scope, __be32 dst,
+ u32 tb_id, u8 type, __be32 dst,
int dst_len, u8 tos, struct fib_info *fi,
unsigned int);
extern void rtmsg_fib(int event, __be32 key, struct fib_alias *fa,
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 2d4bebc..641a5a2 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -222,7 +222,7 @@ static inline unsigned int fib_info_hashfn(const struct fib_info *fi)
unsigned int mask = (fib_info_hash_size - 1);
unsigned int val = fi->fib_nhs;
- val ^= fi->fib_protocol;
+ val ^= (fi->fib_protocol << 8) | fi->fib_scope;
val ^= (__force u32)fi->fib_prefsrc;
val ^= fi->fib_priority;
for_nexthops(fi) {
@@ -248,6 +248,7 @@ static struct fib_info *fib_find_info(const struct fib_info *nfi)
if (fi->fib_nhs != nfi->fib_nhs)
continue;
if (nfi->fib_protocol == fi->fib_protocol &&
+ nfi->fib_scope == fi->fib_scope &&
nfi->fib_prefsrc == fi->fib_prefsrc &&
nfi->fib_priority == fi->fib_priority &&
memcmp(nfi->fib_metrics, fi->fib_metrics,
@@ -328,7 +329,7 @@ void rtmsg_fib(int event, __be32 key, struct fib_alias *fa,
goto errout;
err = fib_dump_info(skb, info->pid, seq, event, tb_id,
- fa->fa_type, fa->fa_scope, key, dst_len,
+ fa->fa_type, key, dst_len,
fa->fa_tos, fa->fa_info, nlm_flags);
if (err < 0) {
/* -EMSGSIZE implies BUG in fib_nlmsg_size() */
@@ -699,7 +700,7 @@ __be32 fib_info_update_nh_saddr(struct net *net, struct fib_nh *nh)
{
nh->nh_saddr = inet_select_addr(nh->nh_dev,
nh->nh_gw,
- nh->nh_cfg_scope);
+ nh->nh_parent->fib_scope);
nh->nh_saddr_genid = atomic_read(&net->ipv4.dev_addr_genid);
return nh->nh_saddr;
@@ -763,6 +764,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
fi->fib_net = hold_net(net);
fi->fib_protocol = cfg->fc_protocol;
+ fi->fib_scope = cfg->fc_scope;
fi->fib_flags = cfg->fc_flags;
fi->fib_priority = cfg->fc_priority;
fi->fib_prefsrc = cfg->fc_prefsrc;
@@ -864,7 +866,6 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
}
change_nexthops(fi) {
- nexthop_nh->nh_cfg_scope = cfg->fc_scope;
fib_info_update_nh_saddr(net, nexthop_nh);
} endfor_nexthops(fi)
@@ -914,7 +915,7 @@ failure:
}
int fib_dump_info(struct sk_buff *skb, u32 pid, u32 seq, int event,
- u32 tb_id, u8 type, u8 scope, __be32 dst, int dst_len, u8 tos,
+ u32 tb_id, u8 type, __be32 dst, int dst_len, u8 tos,
struct fib_info *fi, unsigned int flags)
{
struct nlmsghdr *nlh;
@@ -936,7 +937,7 @@ int fib_dump_info(struct sk_buff *skb, u32 pid, u32 seq, int event,
NLA_PUT_U32(skb, RTA_TABLE, tb_id);
rtm->rtm_type = type;
rtm->rtm_flags = fi->fib_flags;
- rtm->rtm_scope = scope;
+ rtm->rtm_scope = fi->fib_scope;
rtm->rtm_protocol = fi->fib_protocol;
if (rtm->rtm_dst_len)
@@ -1092,7 +1093,7 @@ void fib_select_default(struct fib_result *res)
list_for_each_entry_rcu(fa, fa_head, fa_list) {
struct fib_info *next_fi = fa->fa_info;
- if (fa->fa_scope != res->scope ||
+ if (next_fi->fib_scope != res->scope ||
fa->fa_type != RTN_UNICAST)
continue;
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index ac87a49..90a3ff6 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -1245,7 +1245,6 @@ int fib_table_insert(struct fib_table *tb, struct fib_config *cfg)
if (fa->fa_info->fib_priority != fi->fib_priority)
break;
if (fa->fa_type == cfg->fc_type &&
- fa->fa_scope == cfg->fc_scope &&
fa->fa_info == fi) {
fa_match = fa;
break;
@@ -1271,7 +1270,6 @@ int fib_table_insert(struct fib_table *tb, struct fib_config *cfg)
new_fa->fa_tos = fa->fa_tos;
new_fa->fa_info = fi;
new_fa->fa_type = cfg->fc_type;
- new_fa->fa_scope = cfg->fc_scope;
state = fa->fa_state;
new_fa->fa_state = state & ~FA_S_ACCESSED;
@@ -1308,7 +1306,6 @@ int fib_table_insert(struct fib_table *tb, struct fib_config *cfg)
new_fa->fa_info = fi;
new_fa->fa_tos = tos;
new_fa->fa_type = cfg->fc_type;
- new_fa->fa_scope = cfg->fc_scope;
new_fa->fa_state = 0;
/*
* Insert new entry to the list.
@@ -1362,7 +1359,7 @@ static int check_leaf(struct fib_table *tb, struct trie *t, struct leaf *l,
if (fa->fa_tos && fa->fa_tos != flp->flowi4_tos)
continue;
- if (fa->fa_scope < flp->flowi4_scope)
+ if (fa->fa_info->fib_scope < flp->flowi4_scope)
continue;
fib_alias_accessed(fa);
err = fib_props[fa->fa_type].error;
@@ -1388,7 +1385,7 @@ static int check_leaf(struct fib_table *tb, struct trie *t, struct leaf *l,
res->prefixlen = plen;
res->nh_sel = nhsel;
res->type = fa->fa_type;
- res->scope = fa->fa_scope;
+ res->scope = fa->fa_info->fib_scope;
res->fi = fi;
res->table = tb;
res->fa_head = &li->falh;
@@ -1664,7 +1661,7 @@ int fib_table_delete(struct fib_table *tb, struct fib_config *cfg)
if ((!cfg->fc_type || fa->fa_type == cfg->fc_type) &&
(cfg->fc_scope == RT_SCOPE_NOWHERE ||
- fa->fa_scope == cfg->fc_scope) &&
+ fa->fa_info->fib_scope == cfg->fc_scope) &&
(!cfg->fc_prefsrc ||
fi->fib_prefsrc == cfg->fc_prefsrc) &&
(!cfg->fc_protocol ||
@@ -1863,7 +1860,6 @@ static int fn_trie_dump_fa(t_key key, int plen, struct list_head *fah,
RTM_NEWROUTE,
tb->tb_id,
fa->fa_type,
- fa->fa_scope,
xkey,
plen,
fa->fa_tos,
@@ -2384,7 +2380,7 @@ static int fib_trie_seq_show(struct seq_file *seq, void *v)
seq_indent(seq, iter->depth+1);
seq_printf(seq, " /%d %s %s", li->plen,
rtn_scope(buf1, sizeof(buf1),
- fa->fa_scope),
+ fa->fa_info->fib_scope),
rtn_type(buf2, sizeof(buf2),
fa->fa_type));
if (fa->fa_tos)
--
1.7.4.1
^ permalink raw reply related
* [PATCH 1/2] ipv4: Invalidate nexthop cache nh_saddr more correctly.
From: David Miller @ 2011-03-25 1:14 UTC (permalink / raw)
To: netdev; +Cc: ja
Any operation that:
1) Brings up an interface
2) Adds an IP address to an interface
3) Deletes an IP address from an interface
can potentially invalidate the nh_saddr value, requiring
it to be recomputed.
Perform the recomputation lazily using a generation ID.
Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/ip_fib.h | 12 ++++++++++--
include/net/netns/ipv4.h | 1 +
net/ipv4/fib_frontend.c | 11 +++++++----
net/ipv4/fib_semantics.c | 32 +++++++++++---------------------
net/ipv4/route.c | 6 ++++--
5 files changed, 33 insertions(+), 29 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index a1a8580..cd92b92 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -62,6 +62,7 @@ struct fib_nh {
int nh_oif;
__be32 nh_gw;
__be32 nh_saddr;
+ int nh_saddr_genid;
};
/*
@@ -141,12 +142,19 @@ struct fib_result_nl {
#endif /* CONFIG_IP_ROUTE_MULTIPATH */
-#define FIB_RES_SADDR(res) (FIB_RES_NH(res).nh_saddr)
+extern __be32 fib_info_update_nh_saddr(struct net *net, struct fib_nh *nh);
+
+#define FIB_RES_SADDR(net, res) \
+ ((FIB_RES_NH(res).nh_saddr_genid == \
+ atomic_read(&(net)->ipv4.dev_addr_genid)) ? \
+ FIB_RES_NH(res).nh_saddr : \
+ fib_info_update_nh_saddr((net), &FIB_RES_NH(res)))
#define FIB_RES_GW(res) (FIB_RES_NH(res).nh_gw)
#define FIB_RES_DEV(res) (FIB_RES_NH(res).nh_dev)
#define FIB_RES_OIF(res) (FIB_RES_NH(res).nh_oif)
-#define FIB_RES_PREFSRC(res) ((res).fi->fib_prefsrc ? : FIB_RES_SADDR(res))
+#define FIB_RES_PREFSRC(net, res) ((res).fi->fib_prefsrc ? : \
+ FIB_RES_SADDR(net, res))
struct fib_table {
struct hlist_node tb_hlist;
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index e2e2ef5..542195d 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -55,6 +55,7 @@ struct netns_ipv4 {
int current_rt_cache_rebuild_count;
atomic_t rt_genid;
+ atomic_t dev_addr_genid;
#ifdef CONFIG_IP_MROUTE
#ifndef CONFIG_IP_MROUTE_MULTIPLE_TABLES
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 02c3ba6..f116ce8 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -228,7 +228,7 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
if (res.type != RTN_LOCAL || !accept_local)
goto e_inval;
}
- *spec_dst = FIB_RES_PREFSRC(res);
+ *spec_dst = FIB_RES_PREFSRC(net, res);
fib_combine_itag(itag, &res);
dev_match = false;
@@ -258,7 +258,7 @@ int fib_validate_source(__be32 src, __be32 dst, u8 tos, int oif,
ret = 0;
if (fib_lookup(net, &fl4, &res) == 0) {
if (res.type == RTN_UNICAST) {
- *spec_dst = FIB_RES_PREFSRC(res);
+ *spec_dst = FIB_RES_PREFSRC(net, res);
ret = FIB_RES_NH(res).nh_scope >= RT_SCOPE_HOST;
}
}
@@ -960,6 +960,7 @@ static int fib_inetaddr_event(struct notifier_block *this, unsigned long event,
{
struct in_ifaddr *ifa = (struct in_ifaddr *)ptr;
struct net_device *dev = ifa->ifa_dev->dev;
+ struct net *net = dev_net(dev);
switch (event) {
case NETDEV_UP:
@@ -967,12 +968,12 @@ static int fib_inetaddr_event(struct notifier_block *this, unsigned long event,
#ifdef CONFIG_IP_ROUTE_MULTIPATH
fib_sync_up(dev);
#endif
- fib_update_nh_saddrs(dev);
+ atomic_inc(&net->ipv4.dev_addr_genid);
rt_cache_flush(dev_net(dev), -1);
break;
case NETDEV_DOWN:
fib_del_ifaddr(ifa, NULL);
- fib_update_nh_saddrs(dev);
+ atomic_inc(&net->ipv4.dev_addr_genid);
if (ifa->ifa_dev->ifa_list == NULL) {
/* Last address was deleted from this interface.
* Disable IP.
@@ -990,6 +991,7 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
{
struct net_device *dev = ptr;
struct in_device *in_dev = __in_dev_get_rtnl(dev);
+ struct net *net = dev_net(dev);
if (event == NETDEV_UNREGISTER) {
fib_disable_ip(dev, 2, -1);
@@ -1007,6 +1009,7 @@ static int fib_netdev_event(struct notifier_block *this, unsigned long event, vo
#ifdef CONFIG_IP_ROUTE_MULTIPATH
fib_sync_up(dev);
#endif
+ atomic_inc(&net->ipv4.dev_addr_genid);
rt_cache_flush(dev_net(dev), -1);
break;
case NETDEV_DOWN:
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 75b9fb5..2d4bebc 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -695,6 +695,16 @@ static void fib_info_hash_move(struct hlist_head *new_info_hash,
fib_info_hash_free(old_laddrhash, bytes);
}
+__be32 fib_info_update_nh_saddr(struct net *net, struct fib_nh *nh)
+{
+ nh->nh_saddr = inet_select_addr(nh->nh_dev,
+ nh->nh_gw,
+ nh->nh_cfg_scope);
+ nh->nh_saddr_genid = atomic_read(&net->ipv4.dev_addr_genid);
+
+ return nh->nh_saddr;
+}
+
struct fib_info *fib_create_info(struct fib_config *cfg)
{
int err;
@@ -855,9 +865,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
change_nexthops(fi) {
nexthop_nh->nh_cfg_scope = cfg->fc_scope;
- nexthop_nh->nh_saddr = inet_select_addr(nexthop_nh->nh_dev,
- nexthop_nh->nh_gw,
- nexthop_nh->nh_cfg_scope);
+ fib_info_update_nh_saddr(net, nexthop_nh);
} endfor_nexthops(fi)
link_it:
@@ -1128,24 +1136,6 @@ out:
return;
}
-void fib_update_nh_saddrs(struct net_device *dev)
-{
- struct hlist_head *head;
- struct hlist_node *node;
- struct fib_nh *nh;
- unsigned int hash;
-
- hash = fib_devindex_hashfn(dev->ifindex);
- head = &fib_info_devhash[hash];
- hlist_for_each_entry(nh, node, head, nh_hash) {
- if (nh->nh_dev != dev)
- continue;
- nh->nh_saddr = inet_select_addr(nh->nh_dev,
- nh->nh_gw,
- nh->nh_cfg_scope);
- }
-}
-
#ifdef CONFIG_IP_ROUTE_MULTIPATH
/*
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 34921b0..4b0c811 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1718,7 +1718,7 @@ void ip_rt_get_source(u8 *addr, struct rtable *rt)
rcu_read_lock();
if (fib_lookup(dev_net(rt->dst.dev), &fl4, &res) == 0)
- src = FIB_RES_PREFSRC(res);
+ src = FIB_RES_PREFSRC(dev_net(rt->dst.dev), res);
else
src = inet_select_addr(rt->dst.dev, rt->rt_gateway,
RT_SCOPE_UNIVERSE);
@@ -2615,7 +2615,7 @@ static struct rtable *ip_route_output_slow(struct net *net,
fib_select_default(&res);
if (!fl4.saddr)
- fl4.saddr = FIB_RES_PREFSRC(res);
+ fl4.saddr = FIB_RES_PREFSRC(net, res);
dev_out = FIB_RES_DEV(res);
fl4.flowi4_oif = dev_out->ifindex;
@@ -3219,6 +3219,8 @@ static __net_init int rt_genid_init(struct net *net)
{
get_random_bytes(&net->ipv4.rt_genid,
sizeof(net->ipv4.rt_genid));
+ get_random_bytes(&net->ipv4.dev_addr_genid,
+ sizeof(net->ipv4.dev_addr_genid));
return 0;
}
--
1.7.4.1
^ permalink raw reply related
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