* [PATCH net,v2] net: sock: adapt SOCK_MIN_RCVBUF and SOCK_MIN_SNDBUF
@ 2013-06-19 10:51 Daniel Borkmann
2013-06-19 12:19 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Borkmann @ 2013-06-19 10:51 UTC (permalink / raw)
To: davem; +Cc: eric.dumazet, netdev
The current situation is that SOCK_MIN_RCVBUF is 2048 + sizeof(struct sk_buff))
while SOCK_MIN_SNDBUF is 2048. Since in both cases, skb->truesize is used for
sk_{r,w}mem_alloc accounting, we should have both sizes adjusted via defining a
TCP_SKB_MIN_TRUESIZE.
Further, as Eric Dumazet points out, the minimal skb truesize in transmit path is
SKB_TRUESIZE(2048) after commit f07d960df33c5 ("tcp: avoid frag allocation for
small frames"), and tcp_sendmsg() tries to limit skb size to half the congestion
window, meaning we try to build two skbs at minimum. Thus, having SOCK_MIN_SNDBUF
as 2048 can hit a small regression for some applications setting to low
SO_SNDBUF / SO_RCVBUF. Note that we define a TCP_SKB_MIN_TRUESIZE, because
SKB_TRUESIZE(2048) adds SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), but in
case of TCP skbs, the skb_shared_info is part of the 2048 bytes allocation for
skb->head.
The minor adaption in sk_stream_moderate_sndbuf() is to silence a warning by
using a typed max macro, as similarly done in SOCK_MIN_RCVBUF occurences, that
would appear otherwise.
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
v1 -> v2:
- Applied Eric's feedback, fixed up commit message
- Set subject to 'net' instead of 'net-next' due to the reported regression
include/net/sock.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index ac8e181..753e59f 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2045,18 +2045,21 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
sock_wake_async(sk->sk_socket, how, band);
}
-#define SOCK_MIN_SNDBUF 2048
-/*
- * Since sk_rmem_alloc sums skb->truesize, even a small frame might need
- * sizeof(sk_buff) + MTU + padding, unless net driver perform copybreak
+/* Since sk_{r,w}mem_alloc sums skb->truesize, even a small frame might
+ * need sizeof(sk_buff) + MTU + padding, unless net driver perform copybreak.
+ * Note: for send buffers, TCP works better if we can build two skbs at
+ * minimum.
*/
-#define SOCK_MIN_RCVBUF (2048 + sizeof(struct sk_buff))
+#define TCP_SKB_MIN_TRUESIZE (2048 + sizeof(struct sk_buff))
+
+#define SOCK_MIN_SNDBUF (TCP_SKB_MIN_TRUESIZE * 2)
+#define SOCK_MIN_RCVBUF TCP_SKB_MIN_TRUESIZE
static inline void sk_stream_moderate_sndbuf(struct sock *sk)
{
if (!(sk->sk_userlocks & SOCK_SNDBUF_LOCK)) {
sk->sk_sndbuf = min(sk->sk_sndbuf, sk->sk_wmem_queued >> 1);
- sk->sk_sndbuf = max(sk->sk_sndbuf, SOCK_MIN_SNDBUF);
+ sk->sk_sndbuf = max_t(u32, sk->sk_sndbuf, SOCK_MIN_SNDBUF);
}
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net,v2] net: sock: adapt SOCK_MIN_RCVBUF and SOCK_MIN_SNDBUF
2013-06-19 10:51 [PATCH net,v2] net: sock: adapt SOCK_MIN_RCVBUF and SOCK_MIN_SNDBUF Daniel Borkmann
@ 2013-06-19 12:19 ` Eric Dumazet
2013-06-20 4:17 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-06-19 12:19 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev
On Wed, 2013-06-19 at 12:51 +0200, Daniel Borkmann wrote:
> The current situation is that SOCK_MIN_RCVBUF is 2048 + sizeof(struct sk_buff))
> while SOCK_MIN_SNDBUF is 2048. Since in both cases, skb->truesize is used for
> sk_{r,w}mem_alloc accounting, we should have both sizes adjusted via defining a
> TCP_SKB_MIN_TRUESIZE.
>
> Further, as Eric Dumazet points out, the minimal skb truesize in transmit path is
> SKB_TRUESIZE(2048) after commit f07d960df33c5 ("tcp: avoid frag allocation for
> small frames"), and tcp_sendmsg() tries to limit skb size to half the congestion
> window, meaning we try to build two skbs at minimum. Thus, having SOCK_MIN_SNDBUF
> as 2048 can hit a small regression for some applications setting to low
> SO_SNDBUF / SO_RCVBUF. Note that we define a TCP_SKB_MIN_TRUESIZE, because
> SKB_TRUESIZE(2048) adds SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), but in
> case of TCP skbs, the skb_shared_info is part of the 2048 bytes allocation for
> skb->head.
>
> The minor adaption in sk_stream_moderate_sndbuf() is to silence a warning by
> using a typed max macro, as similarly done in SOCK_MIN_RCVBUF occurences, that
> would appear otherwise.
>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> v1 -> v2:
> - Applied Eric's feedback, fixed up commit message
> - Set subject to 'net' instead of 'net-next' due to the reported regression
I am fine with this patch (I already run it as a matter of fact), but
I think its net-next material :
Regression is not new, and concerns very pathological cases, where
applications relied on some non documented behavior of network stack.
Signed-off-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net,v2] net: sock: adapt SOCK_MIN_RCVBUF and SOCK_MIN_SNDBUF
2013-06-19 12:19 ` Eric Dumazet
@ 2013-06-20 4:17 ` David Miller
2013-07-03 12:02 ` [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-06-20 4:17 UTC (permalink / raw)
To: eric.dumazet; +Cc: dborkman, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 19 Jun 2013 05:19:15 -0700
> On Wed, 2013-06-19 at 12:51 +0200, Daniel Borkmann wrote:
>> The current situation is that SOCK_MIN_RCVBUF is 2048 + sizeof(struct sk_buff))
>> while SOCK_MIN_SNDBUF is 2048. Since in both cases, skb->truesize is used for
>> sk_{r,w}mem_alloc accounting, we should have both sizes adjusted via defining a
>> TCP_SKB_MIN_TRUESIZE.
>>
>> Further, as Eric Dumazet points out, the minimal skb truesize in transmit path is
>> SKB_TRUESIZE(2048) after commit f07d960df33c5 ("tcp: avoid frag allocation for
>> small frames"), and tcp_sendmsg() tries to limit skb size to half the congestion
>> window, meaning we try to build two skbs at minimum. Thus, having SOCK_MIN_SNDBUF
>> as 2048 can hit a small regression for some applications setting to low
>> SO_SNDBUF / SO_RCVBUF. Note that we define a TCP_SKB_MIN_TRUESIZE, because
>> SKB_TRUESIZE(2048) adds SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), but in
>> case of TCP skbs, the skb_shared_info is part of the 2048 bytes allocation for
>> skb->head.
>>
>> The minor adaption in sk_stream_moderate_sndbuf() is to silence a warning by
>> using a typed max macro, as similarly done in SOCK_MIN_RCVBUF occurences, that
>> would appear otherwise.
>>
>> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>> v1 -> v2:
>> - Applied Eric's feedback, fixed up commit message
>> - Set subject to 'net' instead of 'net-next' due to the reported regression
>
> I am fine with this patch (I already run it as a matter of fact), but
> I think its net-next material :
> Regression is not new, and concerns very pathological cases, where
> applications relied on some non documented behavior of network stack.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks guys.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE
2013-06-20 4:17 ` David Miller
@ 2013-07-03 12:02 ` Eric Dumazet
2013-07-03 15:31 ` Neal Cardwell
2013-07-03 23:52 ` David Miller
0 siblings, 2 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-07-03 12:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Daniel Borkmann, Neal Cardwell
From: Eric Dumazet <edumazet@google.com>
commit eea86af6b1e18d ("net: sock: adapt SOCK_MIN_RCVBUF and
SOCK_MIN_SNDBUF") forgot the sk_buff alignment taken into account
in __alloc_skb() : skb->truesize = SKB_TRUESIZE(size);
While above commit fixed the sender issue, the receiver is still
dropping the second packet (on loopback device), because the receiver
socket can not really hold two skbs :
First packet truesize already is above sk_rcvbuf, so even TCP coalescing
cannot help.
On a typical 64bit build, each tcp skb truesize is 2304, instead of 2272
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Daniel Borkmann <dborkman@redhat.com>
Cc: Neal Cardwell <ncardwell@google.com>
---
Google-Bug-Id: 8124810
include/net/sock.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index ea6206c..95a5a2c 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -2052,7 +2052,7 @@ static inline void sk_wake_async(struct sock *sk, int how, int band)
* Note: for send buffers, TCP works better if we can build two skbs at
* minimum.
*/
-#define TCP_SKB_MIN_TRUESIZE (2048 + sizeof(struct sk_buff))
+#define TCP_SKB_MIN_TRUESIZE (2048 + SKB_DATA_ALIGN(sizeof(struct sk_buff)))
#define SOCK_MIN_SNDBUF (TCP_SKB_MIN_TRUESIZE * 2)
#define SOCK_MIN_RCVBUF TCP_SKB_MIN_TRUESIZE
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE
2013-07-03 12:02 ` [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE Eric Dumazet
@ 2013-07-03 15:31 ` Neal Cardwell
2013-07-03 23:52 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Neal Cardwell @ 2013-07-03 15:31 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Netdev, Daniel Borkmann
On Wed, Jul 3, 2013 at 5:02 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> commit eea86af6b1e18d ("net: sock: adapt SOCK_MIN_RCVBUF and
> SOCK_MIN_SNDBUF") forgot the sk_buff alignment taken into account
> in __alloc_skb() : skb->truesize = SKB_TRUESIZE(size);
>
> While above commit fixed the sender issue, the receiver is still
> dropping the second packet (on loopback device), because the receiver
> socket can not really hold two skbs :
> First packet truesize already is above sk_rcvbuf, so even TCP coalescing
> cannot help.
>
> On a typical 64bit build, each tcp skb truesize is 2304, instead of 2272
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Daniel Borkmann <dborkman@redhat.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> ---
Acked-by: Neal Cardwell <ncardwell@google.com>
Tested-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE
2013-07-03 12:02 ` [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE Eric Dumazet
2013-07-03 15:31 ` Neal Cardwell
@ 2013-07-03 23:52 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-07-03 23:52 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, dborkman, ncardwell
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 03 Jul 2013 05:02:22 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> commit eea86af6b1e18d ("net: sock: adapt SOCK_MIN_RCVBUF and
> SOCK_MIN_SNDBUF") forgot the sk_buff alignment taken into account
> in __alloc_skb() : skb->truesize = SKB_TRUESIZE(size);
>
> While above commit fixed the sender issue, the receiver is still
> dropping the second packet (on loopback device), because the receiver
> socket can not really hold two skbs :
> First packet truesize already is above sk_rcvbuf, so even TCP coalescing
> cannot help.
>
> On a typical 64bit build, each tcp skb truesize is 2304, instead of 2272
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-03 23:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-19 10:51 [PATCH net,v2] net: sock: adapt SOCK_MIN_RCVBUF and SOCK_MIN_SNDBUF Daniel Borkmann
2013-06-19 12:19 ` Eric Dumazet
2013-06-20 4:17 ` David Miller
2013-07-03 12:02 ` [PATCH] net: sock: fix TCP_SKB_MIN_TRUESIZE Eric Dumazet
2013-07-03 15:31 ` Neal Cardwell
2013-07-03 23:52 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).