tcp: allow drivers to tweak TSQ logic

PATCH Superseded Eric Dumazet 6 participants 12 messages Nov 10, 2017 – Dec 13, 2017

Subsystems (4)

Networking Network drivers TCP Sockets

lore

Share & tools

lore

Eric Dumazet

author

2017-11-10 02:41 UTC #0 lore raw

PATCH 1/1 tcp: allow drivers to tweak TSQ logic

From: Eric Dumazet <[email protected]>

I had many reports that TSQ logic breaks wifi aggregation.

Current logic is to allow up to 1 ms of bytes to be queued into qdisc and drivers queues.

But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.

This patch adds an extra socket field, allowing wifi drivers to select another log scale to derive TCP Small Queue credit from current pacing rate.

Initial value is 10, meaning that this patch does not change current behavior.

We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)

They would have to use following template :

if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)

     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Ref: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1670041

Signed-off-by Eric Dumazet <[email protected]>

Cc Johannes Berg <[email protected]>

Cc Toke Høiland-Jørgensen <[email protected]>

Cc Kir Kolyshkin <[email protected]>

---
 include/net/sock.h    |    1 +
 net/core/sock.c       |    1 +
 net/ipv4/tcp_output.c |    4 ++--
 3 files changed, 4 insertions(+), 2 deletions(-)

include/net/sock.h +1 -0

index 688a823dccc306bd21f47da167c6922161af5a6a..fb0e5194a3bce61fac00fc234d2a5d1bb3c60f35 100644
@@ -451,6 +451,7 @@ struct sock {
 	kmemcheck_bitfield_end(flags);
 
 	u16			sk_gso_max_segs;
+	u8			sk_pacing_shift;
 	unsigned long	        sk_lingertime;
 	struct proto		*sk_prot_creator;
 	rwlock_t		sk_callback_lock;

net/core/sock.c +1 -0

index c59bcf90d90536fedc7809e397f6bd414781b529..2811ff8322d4a5f68e3e745cf585564e1ec5d809 100644
@@ -2746,6 +2746,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 
 	sk->sk_max_pacing_rate = ~0U;
 	sk->sk_pacing_rate = ~0U;
+	sk->sk_pacing_shift = 10;
 	sk->sk_incoming_cpu = -1;
 	/*
 	 * Before updating sk_refcnt, we must commit prior changes to memory

net/ipv4/tcp_output.c +2 -2

index 9b98d35aa0d8d0a829e4a41985d805d4e2895a8e..fa5e7b81b5ec12039b1347474f5183b1d9c87887 100644
@@ -1737,7 +1737,7 @@ u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
 {
 	u32 bytes, segs;
 
-	bytes = min(sk->sk_pacing_rate >> 10,
+	bytes = min(sk->sk_pacing_rate >> sk->sk_pacing_shift,
 		    sk->sk_gso_max_size - 1 - MAX_TCP_HEADER);
 
 	/* Goal is to send at least one packet per ms,

@@ -2215,7 +2215,7 @@ static bool tcp_small_queue_check(struct sock *sk, const struct sk_buff *skb,
 {
 	unsigned int limit;
 
-	limit = max(2 * skb->truesize, sk->sk_pacing_rate >> 10);
+	limit = max(2 * skb->truesize, sk->sk_pacing_rate >> sk->sk_pacing_shift);
 	limit = min_t(u32, limit,
 		      sock_net(sk)->ipv4.sysctl_tcp_limit_output_bytes);
 	limit <<= factor;

Johannes Berg

2017-11-11 14:27 UTC #1 lore raw

Thanks Eric!

Quoted 2 lines

We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)

I suppose we should test each driver or so.

Quoted 4 lines

They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)
     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Hm. I wish we wouldn't have to do this on every skb, but perhaps it doesn't matter that much.

include/net/sock.h struct sock { · line 454

Quoted 2 lines

include/net/sock.h +1 -0

 	u16			sk_gso_max_segs;
+	u8			sk_pacing_shift;

I guess you tried to fill a hole, but weren't we saying that it would be better in the same cacheline? Then again, perhaps both cachelines are resident anyway, haven't looked at this now.

Unrelated to that, I think this is missing a documentation update since the struct has kernel-doc comments.

johannes

Eric Dumazet

author

2017-11-11 23:38 UTC #2 lore raw

Quoted 7 lines · Johannes Berg

Thanks Eric!
Quoted 2 lines
We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)
I suppose we should test each driver or so.

Quoted 7 lines

Quoted 4 lines
They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)
     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;
Hm. I wish we wouldn't have to do this on every skb, but perhaps it doesn't matter that much.

Yes, it does not matter, even at 40Gbit ;)

Quoted 8 lines

include/net/sock.h struct sock { · line 454
Quoted 4 lines
include/net/sock.h +1 -0
 	u16			sk_gso_max_segs;
+	u8			sk_pacing_shift;
I guess you tried to fill a hole, but weren't we saying that it would be better in the same cacheline? Then again, perhaps both cachelines are resident anyway, haven't looked at this now.

Same cache line already ;)

	u32                        sk_pacing_rate;       /* 0x1c0   0x4 */
	u32                        sk_max_pacing_rate;   /* 0x1c4   0x4 */
	struct page_frag           sk_frag;              /* 0x1c8  0x10 */
	netdev_features_t          sk_route_caps;        /* 0x1d8   0x8 */
	netdev_features_t          sk_route_nocaps;      /* 0x1e0   0x8 */
	int                        sk_gso_type;          /* 0x1e8   0x4 */
	unsigned int               sk_gso_max_size;      /* 0x1ec   0x4 */
	gfp_t                      sk_allocation;        /* 0x1f0   0x4 */
	__u32                      sk_txhash;            /* 0x1f4   0x4 */
	unsigned int               __sk_flags_offset[0]; /* 0x1f8     0 */
	unsigned int               sk_padding:1;         /* 0x1f8:0x1f 0x4 */
	unsigned int               sk_kern_sock:1;       /* 0x1f8:0x1e 0x4 */
	unsigned int               sk_no_check_tx:1;     /* 0x1f8:0x1d 0x4 */
	unsigned int               sk_no_check_rx:1;     /* 0x1f8:0x1c 0x4 */
	unsigned int               sk_userlocks:4;       /* 0x1f8:0x18 0x4 */
	unsigned int               sk_protocol:8;        /* 0x1f8:0x10 0x4 */
	unsigned int               sk_type:16;           /* 0x1f8: 0 0x4 */
	u16                        sk_gso_max_segs;      /* 0x1fc   0x2 */
	u8			   sk_pacing_shift;	 /* 0x1fe   0x1 */

Quoted 3 lines

Unrelated to that, I think this is missing a documentation update since the struct has kernel-doc comments.

Yeah, I believe these kernel-doc on gigantic struct sock are useless and we should remove them, they have zero useful info.

Johannes Berg

2017-11-13 09:21 UTC #3 lore raw

Quoted 4 lines · Eric Dumazet

Quoted 2 lines
Hm. I wish we wouldn't have to do this on every skb, but perhaps it doesn't matter that much.
Yes, it does not matter, even at 40Gbit ;)

Quoted 1 line

Same cache line already ;)

Hah. It seemed so far away, but actually looking at the layout helps :)

Quoted 5 lines

Quoted 2 lines
Unrelated to that, I think this is missing a documentation update since the struct has kernel-doc comments.
Yeah, I believe these kernel-doc on gigantic struct sock are useless and we should remove them, they have zero useful info.

No argument from me, but while it's there ... anyway I see you resent this, thanks for doing this.

I'll try to get people to run some tests for our driver.

johannes

Toke Høiland-Jørgensen

2017-11-12 14:41 UTC #4 lore raw

In reply to Johannes Berg

Quoted 6 lines · Johannes Berg

Thanks Eric!
Quoted 2 lines
We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)
I suppose we should test each driver or so.

I think that for TXQ-enabled drivers we could probably set this in tx_dequeue()?

Will test it when I get back to my testbed...

-Toke

Eric Dumazet

author

2017-11-11 23:54 UTC #5 lore raw

PATCH v2 1/1 tcp: allow drivers to tweak TSQ logic

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>

I had many reports that TSQ logic breaks wifi aggregation.

Current logic is to allow up to 1 ms of bytes to be queued into qdisc and drivers queues.

But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.

This patch adds an extra socket field, allowing wifi drivers to select another log scale to derive TCP Small Queue credit from current pacing rate.

Initial value is 10, meaning that this patch does not change current behavior.

We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)

They would have to use following template :

if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)

     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Ref: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1670041

Signed-off-by Eric Dumazet <[email protected]>

Cc Johannes Berg <[email protected]>

Cc Toke Høiland-Jørgensen <[email protected]>

Cc Kir Kolyshkin <[email protected]>

---
v2: added kernel-doc comment, based on Johannes feedback.

 include/net/sock.h    |    2 ++
 net/core/sock.c       |    1 +
 net/ipv4/tcp_output.c |    4 ++--
 3 files changed, 5 insertions(+), 2 deletions(-)

include/net/sock.h +2 -0

index 688a823dccc306bd21f47da167c6922161af5a6a..f8715c5af37d4e598770dbe5c5f83246241f18d5 100644
@@ -267,6 +267,7 @@ struct sock_common {
   *	@sk_gso_type: GSO type (e.g. %SKB_GSO_TCPV4)
   *	@sk_gso_max_size: Maximum GSO segment size to build
   *	@sk_gso_max_segs: Maximum number of GSO segments
+  *	@sk_pacing_shift: scaling factor for TCP Small Queues
   *	@sk_lingertime: %SO_LINGER l_linger setting
   *	@sk_backlog: always used with the per-socket spinlock held
   *	@sk_callback_lock: used with the callbacks in the end of this struct

@@ -451,6 +452,7 @@ struct sock {
 	kmemcheck_bitfield_end(flags);
 
 	u16			sk_gso_max_segs;
+	u8			sk_pacing_shift;
 	unsigned long	        sk_lingertime;
 	struct proto		*sk_prot_creator;
 	rwlock_t		sk_callback_lock;

net/core/sock.c +1 -0

index 57bbd6040eb6a3c072ce4e024687786079552ddf..13719af7b4e35d2050ccba51d44c7f691a889b37 100644
@@ -2746,6 +2746,7 @@ void sock_init_data(struct socket *sock, struct sock *sk)
 
 	sk->sk_max_pacing_rate = ~0U;
 	sk->sk_pacing_rate = ~0U;
+	sk->sk_pacing_shift = 10;
 	sk->sk_incoming_cpu = -1;
 	/*
 	 * Before updating sk_refcnt, we must commit prior changes to memory

net/ipv4/tcp_output.c +2 -2

index 0256f7a410417d93c9edab9d25a3ce5a81c2b296..76dbe884f2469660028684a46fc19afa000a1353 100644
@@ -1720,7 +1720,7 @@ u32 tcp_tso_autosize(const struct sock *sk, unsigned int mss_now,
 {
 	u32 bytes, segs;
 
-	bytes = min(sk->sk_pacing_rate >> 10,
+	bytes = min(sk->sk_pacing_rate >> sk->sk_pacing_shift,
 		    sk->sk_gso_max_size - 1 - MAX_TCP_HEADER);
 
 	/* Goal is to send at least one packet per ms,

@@ -2198,7 +2198,7 @@ static bool tcp_small_queue_check(struct sock *sk, const struct sk_buff *skb,
 {
 	unsigned int limit;
 
-	limit = max(2 * skb->truesize, sk->sk_pacing_rate >> 10);
+	limit = max(2 * skb->truesize, sk->sk_pacing_rate >> sk->sk_pacing_shift);
 	limit = min_t(u32, limit,
 		      sock_net(sk)->ipv4.sysctl_tcp_limit_output_bytes);
 	limit <<= factor;

Neal Cardwell

maintainer Acked-by on v2 1/1

2017-11-12 13:40 UTC #6 lore raw

Quoted 27 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>
I had many reports that TSQ logic breaks wifi aggregation.
Current logic is to allow up to 1 ms of bytes to be queued into qdisc and drivers queues.
But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.
This patch adds an extra socket field, allowing wifi drivers to select another log scale to derive TCP Small Queue credit from current pacing rate.
Initial value is 10, meaning that this patch does not change current behavior.
We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)
They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)
     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Quoted 6 lines

Ref: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1670041
Signed-off-by Eric Dumazet <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Toke Høiland-Jørgensen <[email protected]>
Cc Kir Kolyshkin <[email protected]>
---

Nice. Thanks, Eric!

Acked-by Neal Cardwell <[email protected]>

neal

David Miller

maintainer Applied

2017-11-14 07:18 UTC #7 lore raw

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Sat, 11 Nov 2017 15:54:12 -0800

Quoted 26 lines

From: Eric Dumazet <[email protected]>
I had many reports that TSQ logic breaks wifi aggregation.
Current logic is to allow up to 1 ms of bytes to be queued into qdisc and drivers queues.
But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.
This patch adds an extra socket field, allowing wifi drivers to select another log scale to derive TCP Small Queue credit from current pacing rate.
Initial value is 10, meaning that this patch does not change current behavior.
We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)
They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)
     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Quoted 7 lines

Ref: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1670041
Signed-off-by Eric Dumazet <[email protected]>
Cc Johannes Berg <[email protected]>
Cc Toke Høiland-Jørgensen <[email protected]>
Cc Kir Kolyshkin <[email protected]>
---
v2: added kernel-doc comment, based on Johannes feedback.

Applied, thanks Eric.

Felix Fietkau

2017-11-28 13:10 UTC #8 lore raw

In reply to Eric Dumazet

Quoted 17 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>
I had many reports that TSQ logic breaks wifi aggregation.
Current logic is to allow up to 1 ms of bytes to be queued into qdisc and drivers queues.
But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.
This patch adds an extra socket field, allowing wifi drivers to select another log scale to derive TCP Small Queue credit from current pacing rate.
Initial value is 10, meaning that this patch does not change current behavior.

Quoted 7 lines

We expect wifi drivers to set this field to smaller values (tests have been done with values from 6 to 9)
They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT)
     skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

I did some experiments with this approach (with your patch backported to a 4.9 kernel), and I got some crashes.
After looking at the crashes and code some more, it seems that this would need some extra checks to ensure that skb->sk is a full struct sock, instead of just a struct request_sock.
Should this be done by checking for skb->sk->sk_state == TCP_ESTABLISHED? It seems to me that this might introduce some extra overhead.

- Felix

Eric Dumazet

author

2017-11-28 17:33 UTC #9 lore raw

Quoted 31 lines · Felix Fietkau

Quoted 21 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
I had many reports that TSQ logic breaks wifi aggregation.
Current logic is to allow up to 1 ms of bytes to be queued into qdisc
and drivers queues.
But Wifi aggregation needs a bigger budget to allow bigger rates to be discovered by various TCP Congestion Controls algorithms.
This patch adds an extra socket field, allowing wifi drivers to select
another log scale to derive TCP Small Queue credit from current pacing
rate.
Initial value is 10, meaning that this patch does not change current
behavior.
Quoted 8 lines
We expect wifi drivers to set this field to smaller values (tests have
been done with values from 6 to 9)
They would have to use following template :
if (skb->sk && skb->sk->sk_pacing_shift != MY_PACING_SHIFT) skb->sk->sk_pacing_shift = MY_PACING_SHIFT;

Quoted 10 lines

I did some experiments with this approach (with your patch backported to
a 4.9 kernel), and I got some crashes.
After looking at the crashes and code some more, it seems that this would need some extra checks to ensure that skb->sk is a full struct sock, instead of just a struct request_sock.
Should this be done by checking for skb->sk->sk_state == TCP_ESTABLISHED? It seems to me that this might introduce some extra overhead.

Hi Felix.

Answer is in the question, the pseudo code in the changelog was not 100% correct.

I will add following helper to net-next I guess :

void sk_pacing_shift_update(struct sock *sk, int val)

{
	if (!sk || !sk_fullsock(sk) || sk->sk_pacing_shift == val)
		return;
	sk->sk_pacing_shift = val;
}

Then you might use it like that :

	sk_pacing_shift_update(skb->sk, 7);

Thanks.

Eric Dumazet

author

2017-12-12 14:34 UTC #10 lore raw

PATCH 1/1 net: sk_pacing_shift_update() helper

From: Eric Dumazet <[email protected]>

In commit 3a9b76fd0db9 ("tcp: allow drivers to tweak TSQ logic") I gave a code sample to set sk->sk_pacing_shift that was not complete.

Better add a helper that can be used by drivers without worries, and maybe amended in the future.

A wifi driver might use it from its ndo_start_xmit()

Following call would setup TCP to allow up to ~8ms of queued data per flow.

sk_pacing_shift_update(skb->sk, 7);

Signed-off-by Eric Dumazet <[email protected]>

---
include/net/sock.h | 11 +++++++++++

 1 file changed, 11 insertions(+)

include/net/sock.h +11 -0

index 9155da42269208b358df8535b14dfd3dba509365..9a9047268d375496bccc954d03ec20baf4177fd3 100644
@@ -2407,4 +2407,15 @@ static inline int sk_get_rmem0(const struct sock *sk, const struct proto *proto)
 	return *proto->sysctl_rmem;
 }
 
+/* Default TCP Small queue budget is ~1 ms of data (1sec >> 10)
+ * Some wifi drivers need to tweak it to get more chunks.
+ * They can use this helper from their ndo_start_xmit()
+ */
+static inline void sk_pacing_shift_update(struct sock *sk, int val)
+{
+	if (!sk || !sk_fullsock(sk) || sk->sk_pacing_shift == val)
+		return;
+	sk->sk_pacing_shift = val;
+}
+
 #endif	/* _SOCK_H */

David Miller

maintainer Applied

2017-12-13 20:11 UTC #11 lore raw

Re: [PATCH net-next] net: sk_pacing_shift_update() helper

From: Eric Dumazet <[email protected]>
Date: Tue, 12 Dec 2017 06:34:19 -0800

Quoted 8 lines

From: Eric Dumazet <[email protected]>
In commit 3a9b76fd0db9 ("tcp: allow drivers to tweak TSQ logic") I gave a code sample to set sk->sk_pacing_shift that was not complete.
Better add a helper that can be used by drivers without worries, and maybe amended in the future.

Quoted 8 lines

A wifi driver might use it from its ndo_start_xmit()
Following call would setup TCP to allow up to ~8ms of queued data per flow.
sk_pacing_shift_update(skb->sk, 7);
Signed-off-by Eric Dumazet <[email protected]>

Applied, thanks Eric.