* IPsec+SCTP+IPv6 bug (was: Re: https://bugzilla.kernel.org/show_bug.cgi?id=24412)
From: Alexey Dobriyan @ 2013-09-06 16:56 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev
In-Reply-To: <5229B37A.2070900@redhat.com>
[add netdev@ to CC]
My notes and recollections are below.
Bug reporter says traffic is unecnrypted which is technically a different thing,
but when I tried to reproduce absense of encryption, I couldn't even get past
established TCP connection.
IPv6 case works (worked) without IPsec (setkey -F; setkey -FP).
IPv4 case worked with IPsec.
IPv6 didn't work with IPsec.
setkey(8) setup:
#!/usr/sbin/setkey -f
flush;
spdflush;
add A B ah 0x42 -A hmac-sha256 0xKEY1;
add B A ah 0x43 -A hmac-sha256 0xKEY2;
add A B esp 0x44 -E blowfish-cbc 0xKEY3;
add B A esp 0x45 -E blowfish-cbc 0xKEY4;
spdadd A B any -P in ipsec esp/transport//require ah/transport//require;
spdadd B A any -P in ipsec esp/transport//require ah/transport//require;
A, B -- IPv4 or IPv6 client/server addresses.
Client and server copy of the file should have matching keys and SPI numbers,
but "opposite" addresses (A <=> B).
Keys are written in hex.
IIRC setkey is picky about key lengths (they have to match exactly those of crypto algorithms).
--------------------------------------------------------------------
IPv4 reproducer:
socat sctp-listen:3333 -
echo plaintext | socat - sctp-connect:IP4IP4IP4IP4:3333
IPv6 reproducer:
socat sctp6-listen:3333 -
#include <sys/socket.h>
#include <arpa/inet.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <unistd.h>
#include <string.h>
int main(void)
{
struct sockaddr_in6 sa6 = {};
int fd;
fd = socket(PF_INET6, SOCK_STREAM, 0x84);
sa6.sin6_family = AF_INET6;
sa6.sin6_port = htons(3333);
inet_pton(AF_INET6, "IP6IP6IP6IP6IP6IP6", &sa6.sin6_addr);
sa6.sin6_scope_id = 2; /* it depends */
connect(fd, (struct sockaddr *)&sa6, sizeof(struct sockaddr_in6));
write(fd, "plaintext\n", strlen("plaintext\n"));
close(fd);
return 0;
}
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Eric Dumazet @ 2013-09-06 16:56 UTC (permalink / raw)
To: Zoltan Kiss; +Cc: Wei Liu, Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <522A049A.7000105@citrix.com>
On Fri, 2013-09-06 at 17:36 +0100, Zoltan Kiss wrote:
> On 06/09/13 13:57, Eric Dumazet wrote:
> > Well, I have no problem to get line rate on 20Gb with a single flow, so
> > other drivers have no problem.
> I've made some tests on bare metal:
> Dell PE R815, Intel 82599EB 10Gb, 3.11-rc4 32 bit kernel with 3.17.3
> ixgbe (TSO, GSO on), iperf 2.0.5
> Transmitting packets toward the remote end (so running iperf -c on this
> host) can make 8.3 Gbps with the default 128k tcp_limit_output_bytes.
> When I increased this to 131.506 (128k + 434 bytes) suddenly it jumped
> to 9.4 Gbps. Iperf CPU usage also jumped a few percent from ~36 to ~40%
> (softint percentage in top also increased from ~3 to ~5%)
Typical tradeoff between latency and throughput
If you favor throughput, then you can increase tcp_limit_output_bytes
The default is quite reasonable IMHO.
> So I guess it would be good to revisit the default value of this
> setting. What hw you used Eric for your 20Gb results?
Mellanox CX-3
Make sure your NIC doesn't hold TX packets in TX ring too long before
signaling an interrupt for TX completion.
For example I had to patch mellanox :
commit ecfd2ce1a9d5e6376ff5c00b366345160abdbbb7
Author: Eric Dumazet <edumazet@google.com>
Date: Mon Nov 5 16:20:42 2012 +0000
mlx4: change TX coalescing defaults
mlx4 currently uses a too high tx coalescing setting, deferring
TX completion interrupts by up to 128 us.
With the recent skb_orphan() removal in commit 8112ec3b872,
performance of a single TCP flow is capped to ~4 Gbps, unless
we increase tcp_limit_output_bytes.
I suggest using 16 us instead of 128 us, allowing a finer control.
Performance of a single TCP flow is restored to previous levels,
while keeping TCP small queues fully enabled with default sysctl.
This patch is also a BQL prereq.
Reported-by: Vimalkumar <j.vimal@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yevgeny Petrilin <yevgenyp@mellanox.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Acked-by: Amir Vadai <amirv@mellanox.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: IPsec+SCTP+IPv6 bug
From: Daniel Borkmann @ 2013-09-06 16:59 UTC (permalink / raw)
To: Alexey Dobriyan; +Cc: netdev
In-Reply-To: <20130906165609.GA2795@p183.telecom.by>
Thanks Alexey, I'll have a look.
^ permalink raw reply
* Re: TSQ accounting skb->truesize degrades throughput for large packets
From: Eric Dumazet @ 2013-09-06 17:00 UTC (permalink / raw)
To: Zoltan Kiss; +Cc: Wei Liu, Jonathan Davies, Ian Campbell, netdev, xen-devel
In-Reply-To: <522A049A.7000105@citrix.com>
On Fri, 2013-09-06 at 17:36 +0100, Zoltan Kiss wrote:
> So I guess it would be good to revisit the default value of this
> setting.
If ixgbe requires 3 TSO packets in TX ring to get line rate, you also
can tweak dev->gso_max_size from 65535 to 64000.
^ permalink raw reply
* RE: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel proper
From: Williams, Mitch A @ 2013-09-06 17:28 UTC (permalink / raw)
To: Joe Perches, Brandeburg, Jesse
Cc: Stephen Hemminger, Kirsher, Jeffrey T, davem@davemloft.net,
e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <1378450038.19204.6.camel@joe-AO722>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> Behalf Of Joe Perches
> Sent: Thursday, September 05, 2013 11:47 PM
> To: Brandeburg, Jesse
> Cc: Stephen Hemminger; Kirsher, Jeffrey T; davem@davemloft.net; e1000-
> devel@lists.sourceforge.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel
> proper
>
> On Fri, 2013-09-06 at 06:28 +0000, Brandeburg, Jesse wrote:
> > On Sep 5, 2013, at 11:15 PM, "Stephen Hemminger"
> <stephen@networkplumber.org<mailto:stephen@networkplumber.org>> wrote:
> >
> > Dumb question why is this named Kbuild instead of Makefile like almost
> > every other network driver?
> >
> > All the new kids are doing it, we'll at least that is what I thought when
> I made it.
> >
> > Re-reading https://www.kernel.org/doc/Documentation/kbuild/makefiles.txt
> >
> > I see that Makefile is preferred but Kbuild overrides Makefile.
> >
> > Either way works, but I would prefer not to rename it at this point but
> would gladly do so in a follow up patch
>
> Please do.
>
> I suppose it makes _some_ sense to use Kbuild in the
> include paths but I think Makefile in places where
> compilations are done is better.
>
That was me. I did it to make things less confusing for our customers and for us as engineers.
My goal was to make our out-of-tree driver as close as possible - including the makefiles - to the upstream driver. Doing this makes it simpler for us to backport and forward-port patches. It makes it less confusing for us when we're moving from one environment to the other.
The Kbuild file submitted here is exactly the same file that will be included in our out-of-tree tarball. To avoid confusion, I named it Kbuild, so nobody would just run make in the source directory. The toplevel makefile in our tarball is named Makefile, and I wanted that to be the only Makefile around so nobody would be confused (including me).
The kernel documentation specifically states that this is acceptable (Documentation/kbuild/modules.txt). Furthermore, Documentation/kbuild/makefiles.txt states that using Kbuild as a filename is acceptable.
Personally, I'd prefer to keep these named Kbuild. Doing so makes it simpler to keep our in-tree and out-of-tree drivers in sync. But if you're really passionate about having them named Makefile, we can deal with it. It's just one more detail for us to keep track of (and potentially screw up).
If there is a hard requirement that these files be named Makefile, then I would request that the kernel documentation be updated to reflect this.
-Mitch
^ permalink raw reply
* [PATCH] tcp: properly increase rcv_ssthresh for ofo packets
From: Eric Dumazet @ 2013-09-06 17:35 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Neal Cardwell
From: Eric Dumazet <edumazet@google.com>
TCP receive window handling is multi staged.
A socket has a memory budget, static or dynamic, in sk_rcvbuf.
Because we do not really know how this memory budget translates to
a TCP window (payload), TCP announces a small initial window
(about 20 MSS).
When a packet is received, we increase TCP rcv_win depending
on the payload/truesize ratio of this packet. Good citizen
packets give a hint that it's reasonable to have rcv_win = sk_rcvbuf/2
This heuristic takes place in tcp_grow_window()
Problem is : We currently call tcp_grow_window() only for in-order
packets.
This means that reorders or packet losses stop proper grow of
rcv_win, and senders are unable to benefit from fast recovery,
or proper reordering level detection.
Really, a packet being stored in OFO queue is not a bad citizen.
It should be part of the game as in-order packets.
In our traces, we very often see sender is limited by linux small
receive windows, even if linux hosts use autotuning (DRS) and should
allow rcv_win to grow to ~3MB.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
---
net/ipv4/tcp_input.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 1969e16..28708d3 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4141,6 +4141,7 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
if (!tcp_try_coalesce(sk, skb1, skb, &fragstolen)) {
__skb_queue_after(&tp->out_of_order_queue, skb1, skb);
} else {
+ tcp_grow_window(sk, skb);
kfree_skb_partial(skb, fragstolen);
skb = NULL;
}
@@ -4216,8 +4217,10 @@ add_sack:
if (tcp_is_sack(tp))
tcp_sack_new_ofo_skb(sk, seq, end_seq);
end:
- if (skb)
+ if (skb) {
+ tcp_grow_window(sk, skb);
skb_set_owner_r(skb, sk);
+ }
}
static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int hdrlen,
^ permalink raw reply related
* Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel proper
From: Stephen Hemminger @ 2013-09-06 17:39 UTC (permalink / raw)
To: Williams, Mitch A
Cc: Joe Perches, Brandeburg, Jesse, Kirsher, Jeffrey T,
davem@davemloft.net, e1000-devel@lists.sourceforge.net,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <AAEA33E297BCAC4B9BB20A7C2DF0AB8D5C3B0A3D@FMSMSX113.amr.corp.intel.com>
On Fri, 6 Sep 2013 17:28:09 +0000
"Williams, Mitch A" <mitch.a.williams@intel.com> wrote:
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On
> > Behalf Of Joe Perches
> > Sent: Thursday, September 05, 2013 11:47 PM
> > To: Brandeburg, Jesse
> > Cc: Stephen Hemminger; Kirsher, Jeffrey T; davem@davemloft.net; e1000-
> > devel@lists.sourceforge.net; netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com
> > Subject: Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel
> > proper
> >
> > On Fri, 2013-09-06 at 06:28 +0000, Brandeburg, Jesse wrote:
> > > On Sep 5, 2013, at 11:15 PM, "Stephen Hemminger"
> > <stephen@networkplumber.org<mailto:stephen@networkplumber.org>> wrote:
> > >
> > > Dumb question why is this named Kbuild instead of Makefile like almost
> > > every other network driver?
> > >
> > > All the new kids are doing it, we'll at least that is what I thought when
> > I made it.
> > >
> > > Re-reading https://www.kernel.org/doc/Documentation/kbuild/makefiles.txt
> > >
> > > I see that Makefile is preferred but Kbuild overrides Makefile.
> > >
> > > Either way works, but I would prefer not to rename it at this point but
> > would gladly do so in a follow up patch
> >
> > Please do.
> >
> > I suppose it makes _some_ sense to use Kbuild in the
> > include paths but I think Makefile in places where
> > compilations are done is better.
> >
>
> That was me. I did it to make things less confusing for our customers and for us as engineers.
>
> My goal was to make our out-of-tree driver as close as possible - including the makefiles - to the upstream driver. Doing this makes it simpler for us to backport and forward-port patches. It makes it less confusing for us when we're moving from one environment to the other.
>
> The Kbuild file submitted here is exactly the same file that will be included in our out-of-tree tarball. To avoid confusion, I named it Kbuild, so nobody would just run make in the source directory. The toplevel makefile in our tarball is named Makefile, and I wanted that to be the only Makefile around so nobody would be confused (including me).
>
> The kernel documentation specifically states that this is acceptable (Documentation/kbuild/modules.txt). Furthermore, Documentation/kbuild/makefiles.txt states that using Kbuild as a filename is acceptable.
>
> Personally, I'd prefer to keep these named Kbuild. Doing so makes it simpler to keep our in-tree and out-of-tree drivers in sync. But if you're really passionate about having them named Makefile, we can deal with it. It's just one more detail for us to keep track of (and potentially screw up).
>
> If there is a hard requirement that these files be named Makefile, then I would request that the kernel documentation be updated to reflect this.
>
> -Mitch
First, upstream kernel does not care how or why you do something for out-of-tree driver.
As far as network developers are concerned, it is your own process (mistake) and you can
live with the consequences of that decision.
Second, why introduce something different to upstream with no apparent benefit
$ find drivers/net -name Kbuild | wc -l
0
^ permalink raw reply
* [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: Marcelo Ricardo Leitner @ 2013-09-06 17:41 UTC (permalink / raw)
To: netdev; +Cc: naleksan, fubar, andy
Hi Dave,
Please queue this one for -stable trees too.
Thanks.
---8<---
As bond_store_arp_validation and bond_store_arp_interval doesn't
deal with each other, we can't chain both at bond_open, otherwise
we are open to this de-sync state, steps in sequence:
- bond is down
- arp_interval = 0
- arp_validate = 1 or 2
- bond is up
- arp_interval = 1
This would lead to bond issuing ARP requests but never listening
to the replies, because the tap wasn't attached. After reaching
this state, while bond is up, setting arp_validate won't help
as it only acts if needed.
---
drivers/net/bonding/bond_main.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 39e5b1c..805d098 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
if (bond->params.miimon) /* link check interval, in milliseconds. */
queue_delayed_work(bond->wq, &bond->mii_work, 0);
- if (bond->params.arp_interval) { /* arp interval, in milliseconds. */
+ if (bond->params.arp_interval) /* arp interval, in milliseconds. */
queue_delayed_work(bond->wq, &bond->arp_work, 0);
- if (bond->params.arp_validate)
- bond->recv_probe = bond_arp_rcv;
- }
+ if (bond->params.arp_validate)
+ bond->recv_probe = bond_arp_rcv;
if (bond->params.mode == BOND_MODE_8023AD) {
queue_delayed_work(bond->wq, &bond->ad_work, 0);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: Nikolay Aleksandrov @ 2013-09-06 17:49 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev, naleksan, fubar, andy
In-Reply-To: <994d2dc994a801fc3aae7aa912266b09ea45d95c.1378489255.git.mleitner@redhat.com>
On 09/06/2013 07:41 PM, Marcelo Ricardo Leitner wrote:
> Hi Dave,
>
> Please queue this one for -stable trees too.
>
> Thanks.
>
> ---8<---
>
> As bond_store_arp_validation and bond_store_arp_interval doesn't
> deal with each other, we can't chain both at bond_open, otherwise
> we are open to this de-sync state, steps in sequence:
> - bond is down
> - arp_interval = 0
> - arp_validate = 1 or 2
> - bond is up
> - arp_interval = 1
>
> This would lead to bond issuing ARP requests but never listening
> to the replies, because the tap wasn't attached. After reaching
> this state, while bond is up, setting arp_validate won't help
> as it only acts if needed.
> ---
> drivers/net/bonding/bond_main.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 39e5b1c..805d098 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
> if (bond->params.miimon) /* link check interval, in milliseconds. */
> queue_delayed_work(bond->wq, &bond->mii_work, 0);
>
> - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */
> + if (bond->params.arp_interval) /* arp interval, in milliseconds. */
> queue_delayed_work(bond->wq, &bond->arp_work, 0);
> - if (bond->params.arp_validate)
> - bond->recv_probe = bond_arp_rcv;
> - }
> + if (bond->params.arp_validate)
> + bond->recv_probe = bond_arp_rcv;
>
> if (bond->params.mode == BOND_MODE_8023AD) {
> queue_delayed_work(bond->wq, &bond->ad_work, 0);
>
Thanks for fixing this.
Acked-by: Nikolay Aleksandrov <nikolay@redhat.com>
^ permalink raw reply
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: Jay Vosburgh @ 2013-09-06 17:56 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev, naleksan, andy
In-Reply-To: <994d2dc994a801fc3aae7aa912266b09ea45d95c.1378489255.git.mleitner@redhat.com>
Marcelo Ricardo Leitner <mleitner@redhat.com> wrote:
>Hi Dave,
>
>Please queue this one for -stable trees too.
>
>Thanks.
>
>---8<---
>
>As bond_store_arp_validation and bond_store_arp_interval doesn't
>deal with each other, we can't chain both at bond_open, otherwise
>we are open to this de-sync state, steps in sequence:
>- bond is down
>- arp_interval = 0
>- arp_validate = 1 or 2
>- bond is up
>- arp_interval = 1
>
>This would lead to bond issuing ARP requests but never listening
>to the replies, because the tap wasn't attached. After reaching
>this state, while bond is up, setting arp_validate won't help
>as it only acts if needed.
You need to sign off you patch.
> drivers/net/bonding/bond_main.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 39e5b1c..805d098 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
> if (bond->params.miimon) /* link check interval, in milliseconds. */
> queue_delayed_work(bond->wq, &bond->mii_work, 0);
>
>- if (bond->params.arp_interval) { /* arp interval, in milliseconds. */
>+ if (bond->params.arp_interval) /* arp interval, in milliseconds. */
> queue_delayed_work(bond->wq, &bond->arp_work, 0);
>- if (bond->params.arp_validate)
>- bond->recv_probe = bond_arp_rcv;
>- }
>+ if (bond->params.arp_validate)
>+ bond->recv_probe = bond_arp_rcv;
Won't this set the recv_probe when arp_interval is 0 (disabled)?
That's going to make bonding look at a bunch of traffic it's not going
to do anything with.
Why is this better than having bonding_store_arp_interval set
recv_probe if arp_validate is set when it queues the arp_work? And,
presumably, clear the recv_probe if arp_interval is being cleared.
-J
> if (bond->params.mode == BOND_MODE_8023AD) {
> queue_delayed_work(bond->wq, &bond->ad_work, 0);
>--
>1.8.3.1
>
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: David Miller @ 2013-09-06 17:58 UTC (permalink / raw)
To: mleitner; +Cc: netdev, naleksan, fubar, andy
In-Reply-To: <994d2dc994a801fc3aae7aa912266b09ea45d95c.1378489255.git.mleitner@redhat.com>
From: Marcelo Ricardo Leitner <mleitner@redhat.com>
Date: Fri, 6 Sep 2013 14:41:53 -0300
> Please queue this one for -stable trees too.
This is very much not the correct way to submit a patch.
First of all, you haven't signed off on the patch.
Second of all, any supplemental information that doesn't belong in the
commit message needs to go after a "---" dileanator line.
^ permalink raw reply
* Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel proper
From: David Miller @ 2013-09-06 18:01 UTC (permalink / raw)
To: jesse.brandeburg
Cc: stephen, jeffrey.t.kirsher, e1000-devel, netdev, gospo, sassmann
In-Reply-To: <C0F8C5F1-0D98-4297-ACC9-55BB9C889FEB@intel.com>
I'm kind of getting sick of the "we'll fix it in a follow up patch"
talk.
Please rename this Kbuild file to the normal Makefile instead of
trying to be different from every single other driver in the
networking for the sake of an issue that is your, and your problem
alone.
You guys should really be grateful that anyone at all not being paid
to do so is reviewing such a huge body of code for you, rather than
complaining that all the issues weren't discovered the first time
this series was posted.
Please start being more reasonable about this situation.
Thank you.
^ permalink raw reply
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: Marcelo Ricardo Leitner @ 2013-09-06 18:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, naleksan, fubar, andy
In-Reply-To: <20130906.135836.365469022075105934.davem@davemloft.net>
Em 06-09-2013 14:58, David Miller escreveu:
> From: Marcelo Ricardo Leitner <mleitner@redhat.com>
> Date: Fri, 6 Sep 2013 14:41:53 -0300
>
>> Please queue this one for -stable trees too.
>
> This is very much not the correct way to submit a patch.
>
> First of all, you haven't signed off on the patch.
>
> Second of all, any supplemental information that doesn't belong in the
> commit message needs to go after a "---" dileanator line.
Okay, sorry for that. I'll resubmit as soon as I work on Jay's comments.
Thanks
Marcelo
^ permalink raw reply
* Re: [PATCH net] bonding: Avoid possible de-sync with arp_validate
From: Marcelo Ricardo Leitner @ 2013-09-06 18:13 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, naleksan, andy
In-Reply-To: <7249.1378490193@death.nxdomain>
Em 06-09-2013 14:56, Jay Vosburgh escreveu:
> Marcelo Ricardo Leitner <mleitner@redhat.com> wrote:
>
>> Hi Dave,
>>
>> Please queue this one for -stable trees too.
>>
>> Thanks.
>>
>> ---8<---
>>
>> As bond_store_arp_validation and bond_store_arp_interval doesn't
>> deal with each other, we can't chain both at bond_open, otherwise
>> we are open to this de-sync state, steps in sequence:
>> - bond is down
>> - arp_interval = 0
>> - arp_validate = 1 or 2
>> - bond is up
>> - arp_interval = 1
>>
>> This would lead to bond issuing ARP requests but never listening
>> to the replies, because the tap wasn't attached. After reaching
>> this state, while bond is up, setting arp_validate won't help
>> as it only acts if needed.
>
> You need to sign off you patch.
My bad, sorry
>> drivers/net/bonding/bond_main.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 39e5b1c..805d098 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3180,11 +3180,10 @@ static int bond_open(struct net_device *bond_dev)
>> if (bond->params.miimon) /* link check interval, in milliseconds. */
>> queue_delayed_work(bond->wq, &bond->mii_work, 0);
>>
>> - if (bond->params.arp_interval) { /* arp interval, in milliseconds. */
>> + if (bond->params.arp_interval) /* arp interval, in milliseconds. */
>> queue_delayed_work(bond->wq, &bond->arp_work, 0);
>> - if (bond->params.arp_validate)
>> - bond->recv_probe = bond_arp_rcv;
>> - }
>> + if (bond->params.arp_validate)
>> + bond->recv_probe = bond_arp_rcv;
>
> Won't this set the recv_probe when arp_interval is 0 (disabled)?
> That's going to make bonding look at a bunch of traffic it's not going
> to do anything with.
Yes.
> Why is this better than having bonding_store_arp_interval set
> recv_probe if arp_validate is set when it queues the arp_work? And,
> presumably, clear the recv_probe if arp_interval is being cleared.
>
> -J
Yeah, no good. You're right.
Self-NACK here. Nikolay will submit a better patch later.
Thanks,
Marcelo
>> if (bond->params.mode == BOND_MODE_8023AD) {
>> queue_delayed_work(bond->wq, &bond->ad_work, 0);
>> --
>> 1.8.3.1
>>
>
> ---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>
>
>
^ permalink raw reply
* Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel proper
From: Joe Perches @ 2013-09-06 18:20 UTC (permalink / raw)
To: Williams, Mitch A
Cc: Brandeburg, Jesse, Stephen Hemminger, Kirsher, Jeffrey T,
davem@davemloft.net, e1000-devel@lists.sourceforge.net,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <AAEA33E297BCAC4B9BB20A7C2DF0AB8D5C3B0A3D@FMSMSX113.amr.corp.intel.com>
On Fri, 2013-09-06 at 17:28 +0000, Williams, Mitch A wrote:
> My goal was to make our out-of-tree driver as close as possible -
> including the makefiles - to the upstream driver. Doing this makes it
> simpler for us to backport and forward-port patches. It makes it less
> confusing for us when we're moving from one environment to the other.
I think using things like
#define i40e_memset memset
is just odd.
^ permalink raw reply
* Re: [E1000-devel] [net-next v5 8/8] i40e: include i40e in kernel proper
From: David Miller @ 2013-09-06 18:23 UTC (permalink / raw)
To: joe
Cc: mitch.a.williams, jesse.brandeburg, stephen, jeffrey.t.kirsher,
e1000-devel, netdev, gospo, sassmann
In-Reply-To: <1378491615.19204.10.camel@joe-AO722>
From: Joe Perches <joe@perches.com>
Date: Fri, 06 Sep 2013 11:20:15 -0700
> On Fri, 2013-09-06 at 17:28 +0000, Williams, Mitch A wrote:
>> My goal was to make our out-of-tree driver as close as possible -
>> including the makefiles - to the upstream driver. Doing this makes it
>> simpler for us to backport and forward-port patches. It makes it less
>> confusing for us when we're moving from one environment to the other.
>
> I think using things like
>
> #define i40e_memset memset
>
> is just odd.
Agreed, this is rediculous.
^ permalink raw reply
* Re: [PATCH] bnx2x: avoid atomic allocations during initialization
From: David Miller @ 2013-09-06 18:27 UTC (permalink / raw)
To: mschmidt; +Cc: netdev, ariele, eilong
In-Reply-To: <1378411989-19775-1-git-send-email-mschmidt@redhat.com>
From: Michal Schmidt <mschmidt@redhat.com>
Date: Thu, 5 Sep 2013 22:13:09 +0200
> During initialization bnx2x allocates significant amounts of memory
> (for rx data, rx SGEs, TPA pool) using atomic allocations.
>
> I received a report where bnx2x failed to allocate SGEs and it had
> to fall back to TPA-less operation.
>
> Let's use GFP_KERNEL allocations during initialization, which runs
> in process context. Add gfp_t parameters to functions that are used
> both in initialization and in the receive path.
>
> Use an unlikely branch in bnx2x_frag_alloc() to avoid atomic allocation
> by netdev_alloc_frag(). The branch is taken several thousands of times
> during initialization, but then never more. Note that fp->rx_frag_size
> is never greater than PAGE_SIZE, so __get_free_page() can be used here.
>
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
This change looks good to me, if some Broadcom folks could take a look
and ACK/NACK that would be great.
Thanks.
^ permalink raw reply
* Re: [net-next v5 8/8] i40e: include i40e in kernel proper
From: Jesse Brandeburg @ 2013-09-06 18:30 UTC (permalink / raw)
To: David Miller; +Cc: e1000-devel, netdev, gospo, sassmann
In-Reply-To: <20130906.140141.2245821068783126791.davem@davemloft.net>
On Fri, 6 Sep 2013 14:01:41 -0400
David Miller <davem@davemloft.net> wrote:
> Please rename this Kbuild file to the normal Makefile instead of
> trying to be different from every single other driver in the
> networking for the sake of an issue that is your, and your problem
> alone.
Thanks Dave, will do, I'm preparing the patch now.
> You guys should really be grateful that anyone at all not being paid
> to do so is reviewing such a huge body of code for you, rather than
> complaining that all the issues weren't discovered the first time
> this series was posted.
We *are* really grateful for all the effort of any/all reviewers. I
would like to personally thank you Dave, Joe Perches, Ben Hutchings,
and Stephen Hemminger for the non-trival amount of time spent on
reviewing this patch set.
------------------------------------------------------------------------------
Learn the latest--Visual Studio 2012, SharePoint 2013, SQL 2012, more!
Discover the easy way to master current and previous Microsoft technologies
and advance your career. Get an incredible 1,500+ hours of step-by-step
tutorial videos with LearnDevNow. Subscribe today and save!
http://pubads.g.doubleclick.net/gampad/clk?id=58041391&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH net] net: netlink: filter particular protocols from analyzers
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: dborkman; +Cc: netdev, stephen
In-Reply-To: <5228E2A0.4010607@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 05 Sep 2013 21:59:28 +0200
> On 09/05/2013 09:54 PM, David Miller wrote:
>> From: Daniel Borkmann <dborkman@redhat.com>
>> Date: Thu, 05 Sep 2013 21:48:00 +0200
>>
>>> With socket(PF_PACKET, ..., htons(ETH_P_ALL)) you will already get
>>> all users from the suggested white-list of the patch, which is the
>>> majority of netlink users I believe. Hence, you do not need to have
>>> one socket per protocol. skbs from there should get dragged into
>>> pf_packet via dev_queue_xmit_nit() which works on ptype_all list.
>>
>> What about user level netlink protocols?
>
> If you are referring to NETLINK_USERSOCK, then we let this pass here,
> so nothing changes.
Ok, I've applied this, thanks Daniel.
^ permalink raw reply
* Re: [PATCH net-next] tcp: fix no cwnd growth after timeout
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: ncardwell; +Cc: ycheng, edumazet, netdev
In-Reply-To: <CADVnQynvfPtsJN-XqpTkbu4Y+=uzYcmv22GpaV=tRYL8w90TUw@mail.gmail.com>
From: Neal Cardwell <ncardwell@google.com>
Date: Thu, 5 Sep 2013 19:56:59 -0400
> On Thu, Sep 5, 2013 at 6:36 PM, Yuchung Cheng <ycheng@google.com> wrote:
>> In commit 0f7cc9a3 "tcp: increase throughput when reordering is high",
>> it only allows cwnd to increase in Open state. This mistakenly disables
>> slow start after timeout (CA_Loss). Moreover cwnd won't grow if the
>> state moves from Disorder to Open later in tcp_fastretrans_alert().
>>
>> Therefore the correct logic should be to allow cwnd to grow as long
>> as the data is received in order in Open, Loss, or even Disorder state.
>>
>> Signed-off-by: Yuchung Cheng <ycheng@google.com>
>
> Acked-by: Neal Cardwell <ncardwell@google.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net] bnx2x: fix broken compilation with CONFIG_BNX2X_SRIOV is not set
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: dmitry; +Cc: netdev, eric.dumazet, ariele
In-Reply-To: <1378452928-6072-1-git-send-email-dmitry@broadcom.com>
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Fri, 6 Sep 2013 10:35:28 +0300
> Since commit 60cad4e67bd6ff400e7ea61fe762b3042b12ae9d
> "bnx2x: VF RSS support - VF side" fails to compile w/o
> CONFIG_BNX2X_SRIOV option.
>
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Ariel Elior <ariele@broadcom.com>
> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> ---
> Done with same formatting used in the file.
Applied.
^ permalink raw reply
* Re: [PATCH net] bnx2x: Restore a call to config_init
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: eilong; +Cc: netdev, davej
In-Reply-To: <1378461302.15758.2.camel@lb-tlvb-eilong.il.broadcom.com>
From: "Eilon Greenstein" <eilong@broadcom.com>
Date: Fri, 6 Sep 2013 12:55:02 +0300
> Commit c0a77ec74f295013d7ba3204dd3ed25fccf83cb4 'bnx2x: Add missing braces in
> bnx2x:bnx2x_link_initialize' identified indentation problem, but resolved it
> by adding braces instead of fixing the indentation. The braces now prevents a
> config_init call in some cases, though it should be called regardless of that
> condition. This patch removes the braces and fix the confusing indentation
> that caused this mess.
>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Applied.
^ permalink raw reply
* Re: [PATCH] mlx5: remove unused MLX5_DEBUG param in Kconfig
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: michael.opdenacker-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
Cc: eli-VPRAkNaXOzVWk0Htik3J/w, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1378468162-3109-1-git-send-email-michael.opdenacker-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
From: Michael Opdenacker <michael.opdenacker-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Date: Fri, 6 Sep 2013 13:49:22 +0200
> This patch proposes to remove the MLX5_DEBUG kernel configuration
> parameter defined in drivers/net/ethernet/mellanox/mlx5/core/Kconfig,
> but used nowhere in the makefiles and source code.
>
> This could also be fixed by using this parameter,
> but this may be a leftover from driver development...
>
> Signed-off-by: Michael Opdenacker <michael.opdenacker-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next v2] net: add documentation for BQL helpers
From: David Miller @ 2013-09-06 18:47 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, eric.dumazet
In-Reply-To: <1378483080-31632-1-git-send-email-f.fainelli@gmail.com>
From: "Florian Fainelli" <f.fainelli@gmail.com>
Date: Fri, 6 Sep 2013 16:58:00 +0100
> Provide a kernel-doc comment documentation for the BQL helpers:
> - netdev_sent_queue
> - netdev_completed_queue
> - netdev_reset_queue
>
> Similarly to how it is done for the other functions, the documentation
> only covers the function operating on struct net_device and not struct
> netdev_queue.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH] tcp: properly increase rcv_ssthresh for ofo packets
From: David Miller @ 2013-09-06 18:48 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, ncardwell
In-Reply-To: <1378488958.31445.47.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 06 Sep 2013 10:35:58 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> TCP receive window handling is multi staged.
>
> A socket has a memory budget, static or dynamic, in sk_rcvbuf.
>
> Because we do not really know how this memory budget translates to
> a TCP window (payload), TCP announces a small initial window
> (about 20 MSS).
>
> When a packet is received, we increase TCP rcv_win depending
> on the payload/truesize ratio of this packet. Good citizen
> packets give a hint that it's reasonable to have rcv_win = sk_rcvbuf/2
>
> This heuristic takes place in tcp_grow_window()
>
> Problem is : We currently call tcp_grow_window() only for in-order
> packets.
>
> This means that reorders or packet losses stop proper grow of
> rcv_win, and senders are unable to benefit from fast recovery,
> or proper reordering level detection.
>
> Really, a packet being stored in OFO queue is not a bad citizen.
> It should be part of the game as in-order packets.
>
> In our traces, we very often see sender is limited by linux small
> receive windows, even if linux hosts use autotuning (DRS) and should
> allow rcv_win to grow to ~3MB.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Neal Cardwell <ncardwell@google.com>
Applied.
^ 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