net: more accurate skb truesize - regression on Microblaze

Hi Eric,

I have spent some time to investigate what it is causing regression on our network bechmark and I have identified that your patch "net: more accurate skb truesize" is causing that.

I see regression especially on incoming tcp (RX) connection which is 20%. On TX I see regression till 5%.

In general microblaze systems are pretty sensitive for memory usage and working with it. Increasing packet sizes has big impact on performance.
I was surprised that regression is so high.

From microblaze point of view is more important to remove that performance regression
than having more accurate memory accounting. 20% performance regression is simple so high for that.
Isn't there any other way to doing better memory accounting?
Or is there any workaround which would be possible to use?

Thanks,
Michal

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

Quoted 5 lines · Eric Dumazet

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

What is the driver you currently use on your platform ?

Quoted 8 lines · Eric Dumazet

Quoted 5 lines · Eric Dumazet

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

Quoted 1 line

What is the driver you currently use on your platform ?

Using Xilinx ll_temac(in mainline) and axi_emac.

Quoted 9 lines · Michal Simek

Quoted 8 lines · Eric Dumazet

Quoted 5 lines · Eric Dumazet

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

Quoted 4 lines

Quoted 1 line

What is the driver you currently use on your platform ?

Using Xilinx ll_temac(in mainline) and axi_emac.

OK thanks, could you please give us the content
of /proc/sys/net/ipv4/tcp_rmem ?

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Quoted 2 lines · Michal Simek

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Are they default values, or tuned by admin ?

130048 bytes isnt enough to let TCP open its rcv window.

Quoted 5 lines · Eric Dumazet

Quoted 2 lines · Michal Simek

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Quoted 3 lines

Are they default values, or tuned by admin ?

130048 bytes isnt enough to let TCP open its rcv window.

yep. Default value after powerup. What's wrong with that?

Michal

Quoted 6 lines · Michal Simek

Quoted 5 lines · Eric Dumazet

Quoted 2 lines · Michal Simek

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Quoted 8 lines

Quoted 3 lines

Are they default values, or tuned by admin ?

130048 bytes isnt enough to let TCP open its rcv window.

yep. Default value after powerup. What's wrong with that?

Michal

I guess your tcp performance is driven by these numbers mostly.

receive window wont grow above 64K in these case. a tcpdump could confirm the issue.

Do you have 130000 pages of memory ?

Quoted 6 lines · Michal Simek

Quoted 5 lines · Eric Dumazet

Quoted 2 lines · Michal Simek

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Quoted 8 lines

Quoted 3 lines

Are they default values, or tuned by admin ?

130048 bytes isnt enough to let TCP open its rcv window.

yep. Default value after powerup. What's wrong with that?

Michal

I guess your tcp performance is driven by these numbers mostly.

receive window wont grow above 64K in these case. a tcpdump could confirm the issue.

Do you have 130000 pages of memory ?

Seems the net/ipv4/tcp.c code is wrong (or the comment is wrong)

Since its not 1/128 but 1/1024 ....

        /* Set per-socket limits to no more than 1/128 the pressure threshold */
        limit = nr_free_buffer_pages() << (PAGE_SHIFT - 10);
        limit = max(limit, 128UL);
        max_share = min(4UL*1024*1024, limit);

        sysctl_tcp_wmem[0] = SK_MEM_QUANTUM;
        sysctl_tcp_wmem[1] = 16*1024;
        sysctl_tcp_wmem[2] = max(64*1024, max_share);

        sysctl_tcp_rmem[0] = SK_MEM_QUANTUM;
        sysctl_tcp_rmem[1] = 87380;
        sysctl_tcp_rmem[2] = max(87380, max_share);

Quoted 28 lines · Eric Dumazet

Quoted 6 lines · Michal Simek

Quoted 5 lines · Eric Dumazet

Quoted 2 lines · Michal Simek

~ # cat /proc/sys/net/ipv4/tcp_rmem
4096 87380 130048

Quoted 8 lines

Quoted 3 lines

Are they default values, or tuned by admin ?

130048 bytes isnt enough to let TCP open its rcv window.

yep. Default value after powerup. What's wrong with that?

Michal

I guess your tcp performance is driven by these numbers mostly.

receive window wont grow above 64K in these case. a tcpdump could confirm the issue.

Do you have 130000 pages of memory ?

Seems the net/ipv4/tcp.c code is wrong (or the comment is wrong)

Since its not 1/128 but 1/1024 ....

Quoted 13 lines

         /* Set per-socket limits to no more than 1/128 the pressure threshold */
         limit = nr_free_buffer_pages()<<  (PAGE_SHIFT - 10);
         limit = max(limit, 128UL);
         max_share = min(4UL*1024*1024, limit);

         sysctl_tcp_wmem[0] = SK_MEM_QUANTUM;
         sysctl_tcp_wmem[1] = 16*1024;
         sysctl_tcp_wmem[2] = max(64*1024, max_share);

         sysctl_tcp_rmem[0] = SK_MEM_QUANTUM;
         sysctl_tcp_rmem[1] = 87380;
         sysctl_tcp_rmem[2] = max(87380, max_share);

OK there is a bug introduced in commit
c43b874d5d714f271b80d4c3f49e05d0cbf51ed2
(tcp: properly initialize tcp memory limits)

I'll send a patch to change :

limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 10);

back to

limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 7);

You could try this :)

Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) added a regression on machines with low amount of memory, since sockets cant use 1/128 of memory but 1/1024

Fix this to match comment and previous behavior.

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

Cc Jason Wang<[email protected]>

Cc Glauber Costa<[email protected]>

---
  net/ipv4/tcp.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

net/ipv4/tcp.c +0 -0

index 5d54ed3..67d726e 100644

Quoted 23 lines · Eric Dumazet

Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) added a regression on machines with low amount of memory, since sockets cant use 1/128 of memory but 1/1024

Fix this to match comment and previous behavior.

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

Cc Jason Wang<[email protected]>

Cc Glauber Costa<[email protected]>

---
  net/ipv4/tcp.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

net/ipv4/tcp.c +0 -0

index 5d54ed3..67d726e 100644
@@ -3302,7 +3302,7 @@ void __init tcp_init(void)
 
      tcp_init_mem(&init_net);
      /* Set per-socket limits to no more than 1/128 the pressure 

threshold */

net/ipv4/tcp.c void __init tcp_init(void) · line 3307

Quoted 4 lines

net/ipv4/tcp.c +1 -1

-    limit = nr_free_buffer_pages()<<  (PAGE_SHIFT - 10);
+    limit = nr_free_buffer_pages()<<  (PAGE_SHIFT - 7);
      limit = max(limit, 128UL);
      max_share = min(4UL*1024*1024, limit);

hw design with csum is also much better.

Tested-by Michal Simek <[email protected]>

Thanks for help,
Michal

Quoted 29 lines · Michal Simek

Quoted 18 lines · Eric Dumazet

Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) added a regression on machines with low amount of memory, since sockets cant use 1/128 of memory but 1/1024

Fix this to match comment and previous behavior.

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

Cc Jason Wang<[email protected]>

Cc Glauber Costa<[email protected]>

---
net/ipv4/tcp.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

net/ipv4/tcp.c +0 -0

index 5d54ed3..67d726e 100644

net/ipv4/tcp.c void __init tcp_init(void) · line 3308

Quoted 8 lines

net/ipv4/tcp.c +1 -1

@@ -3302,7 +3302,7 @@ void __init tcp_init(void)
 
 tcp_init_mem(&init_net);
 /* Set per-socket limits to no more than 1/128 the pressure threshold */
- limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 10);
+ limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 7);
 limit = max(limit, 128UL);
 max_share = min(4UL*1024*1024, limit);
 

hw design with csum is also much better.

Quoted 9 lines

Tested-by Michal Simek <[email protected]>

Thanks for help,
Michal

Hi Michal and Eric:

Which version of kernel did you test, did you try the newest kernel? The reason I use (PAGE_SHIFT - 10) is in the commit before 3dc43e3, the limit were calculated with:

Quoted 30 lines · Michal Simek

Quoted 23 lines · Eric Dumazet

Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) added a regression on machines with low amount of memory, since sockets cant use 1/128 of memory but 1/1024

Fix this to match comment and previous behavior.

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

Cc Jason Wang<[email protected]>

Cc Glauber Costa<[email protected]>

---
  net/ipv4/tcp.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

net/ipv4/tcp.c +0 -0

index 5d54ed3..67d726e 100644
@@ -3302,7 +3302,7 @@ void __init tcp_init(void)
 
      tcp_init_mem(&init_net);
      /* Set per-socket limits to no more than 1/128 the pressure 

threshold */

net/ipv4/tcp.c void __init tcp_init(void) · line 3307

Quoted 4 lines

net/ipv4/tcp.c +1 -1

-    limit = nr_free_buffer_pages()<<  (PAGE_SHIFT - 10);
+    limit = nr_free_buffer_pages()<<  (PAGE_SHIFT - 7);
      limit = max(limit, 128UL);
      max_share = min(4UL*1024*1024, limit);

hw design with csum is also much better.

Quoted 9 lines

Tested-by Michal Simek <[email protected]>

Thanks for help,
Michal

Hi Michal and Eric:

Which version of kernel did you test, did you try the newest kernel? The reason I use (PAGE_SHIFT - 10) is in the commit before 3dc43e3, the limit were calculated with:

     limit = nr_free_buffer_pages() / 8;
     limit = max(limit, 128UL);
...
     limit = ((unsigned long)sysctl_tcp_mem[1]) << (PAGE_SHIFT - 7);

So the rmem should be ok. But there's a defect (which I think does affect the regression) of my patch would could cause limit that we should shift after comparing with 128UL like:

     limit = nr_free_buffer_pages() / 8;
     limit = max(limit, 128UL) << (PAGE_SHIFT - 7);

Is anything I miss?

Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) tried to fix a regression added in commits 4acb4190 & 3dc43e3,
but still get it wrong.

Result is machines with low amount of memory have too small tcp_rmem[2] value and slow tcp receives : Per socket limit being 1/1024 of memory instead of 1/128 in old kernels, so rcv window is capped to small values.

Fix this to match comment and previous behavior.

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

Cc Jason Wang <[email protected]>

Cc Glauber Costa <[email protected]>

Quoted 8 lines · Eric Dumazet

Quoted 5 lines · Eric Dumazet

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

Quoted 1 line

What is the driver you currently use on your platform ?

Using Xilinx ll_temac(in mainline) and axi_emac.

Quoted 9 lines · Michal Simek

Quoted 8 lines · Eric Dumazet

Quoted 5 lines · Eric Dumazet

BTW, some NIC drivers are known to provide fat skb in their rx path, and need to be fixed as well. (Some others just lie about skb->truesize to avoid the tcp slowdown, see my previous iwlwifi patch)

Quoted 4 lines

Quoted 1 line

What is the driver you currently use on your platform ?

Using Xilinx ll_temac(in mainline) and axi_emac.

ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?

Trying to allocate smaller packets would definitely help your tcp receiver performance and overall reliability (order-0 allocations instead of order-2)

Quoted 2 lines · Eric Dumazet

ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?

yes.

Quoted 4 lines

Trying to allocate smaller packets would definitely help your tcp receiver performance and overall reliability (order-0 allocations instead of order-2)

There is long history for ll_temac and axi emac drivers. and if you compare them they are very similar with similar bugs.
In our distribution we are using older ll_temac driver where this is fixed. Performance is much higher if you allocate buffers for actual mtu.

Thanks,
Michal

P.S.: Our plan is to start to use mainline drivers and fixing them to get the same performance as we have with old one. It is one my todo list.

Quoted 16 lines · Michal Simek

Quoted 2 lines · Eric Dumazet

ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?

yes.

Quoted 4 lines

Trying to allocate smaller packets would definitely help your tcp receiver performance and overall reliability (order-0 allocations instead of order-2)

There is long history for ll_temac and axi emac drivers. and if you compare them they are very similar with similar bugs.
In our distribution we are using older ll_temac driver where this is fixed. Performance is much higher if you allocate buffers for actual mtu.

Quoted 5 lines

Thanks,
Michal

P.S.: Our plan is to start to use mainline drivers and fixing them to get the same performance as we have with old one. It is one my todo list.

Ah good, because this 9000+pad versus 1500+pad allocation is way more problematic than the (small) change on skb->truesize you bisected ;)

There is long history for ll_temac and axi emac drivers. and if you compare
them they are very similar with similar bugs.
In our distribution we are using older ll_temac driver where this is fixed.
Performance is much higher if you allocate buffers for actual mtu.

I reported problem with performance regression on the same drvier between one kernel version and the newer version. Even with pointer to specific patch which I have identified as source of the problem. It has nothing to do with performance of driver itself because identified patch directly points to generic network code not to mainline driver and performance problem in it.

I didn't report problem on any private held driver. I think you have misunderstand our later discussion about ll_temac driver where are some problems which at least

From: Michal Simek<[email protected]>
Date: Tue, 10 Apr 2012 16:04:18 +0200

Quoted 3 lines

I didn't report problem on any private held driver. I think you have misunderstand our later discussion about ll_temac driver where are some problems which at least

Thanks for the clarification.

Quoted 11 lines · Michal Simek

Hi Eric,

I have spent some time to investigate what it is causing regression on our network bechmark and I have identified that your patch "net: more accurate skb truesize" is causing that.

I see regression especially on incoming tcp (RX) connection which is 20%. On TX I see regression till 5%.

In general microblaze systems are pretty sensitive for memory usage and working with it. Increasing packet sizes has big impact on performance.
I was surprised that regression is so high.

Quoted 9 lines

From microblaze point of view is more important to remove that performance regression
than having more accurate memory accounting. 20% performance regression is simple so high for that.
Isn't there any other way to doing better memory accounting?
Or is there any workaround which would be possible to use?

Thanks,
Michal

Hi Michal

We are currently working on the issue.

TCP stack was a bit optimistic in the ideal skb->len / skb->truesize ratio that happened to 'mostly work' before my patch, but not anymore after it.

memory accounting was wrong, we really wanted to be accurate, or risk OOM and crashes.

Now we must fix tcp.

In the meantime you could try :

echo 1>/proc/sys/net/ipv4/tcp_adv_win_scale