[PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

[PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

[Elof Vrigborn]

CDC NCM driver clones SKBs for received data and update SKB
members of the clones according to received frames.

If SKB clone len member is updated but truesize member is not, it
could cause check #2 of the tcp_grow_window function to increase
the TCP window by the __tcp_grow_window function instead of the
expected increment by 2*advmss. This could to a great extent
limit the rcv_ssthres and by that the advertised TCP Window Size,
and in the end the data rate of a TCP connection, as according to
the TCP Sliding Window concept.

With this patch the truesize member of the SKB clones is updated
in similarity to the len member and by this the expected incremental
of the advertised TCP Window Size is seen and the TCP connection
data rate will not be unnecessarily limited.

Signed-off-by: Elof Vrigborn <elof.vrigborn@ericsson.com>
---
 drivers/net/usb/cdc_ncm.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index f33ca6a..09923cd 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1083,6 +1083,7 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 			if (!skb)
 				goto error;
 			skb->len = temp;
+			skb->truesize = temp + sizeof(struct sk_buff);
 			skb->data = ((u8 *)skb_in->data) + offset;
 			skb_set_tail_pointer(skb, temp);
 			usbnet_skb_return(dev, skb);
-- 
1.7.4.4

Re: [PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

[Eric Dumazet]

Le vendredi 10 juin 2011 à 15:39 +0200, Elof Vrigborn a écrit :
> CDC NCM driver clones SKBs for received data and update SKB
> members of the clones according to received frames.
> 
> If SKB clone len member is updated but truesize member is not, it
> could cause check #2 of the tcp_grow_window function to increase
> the TCP window by the __tcp_grow_window function instead of the
> expected increment by 2*advmss. This could to a great extent
> limit the rcv_ssthres and by that the advertised TCP Window Size,
> and in the end the data rate of a TCP connection, as according to
> the TCP Sliding Window concept.
> 
> With this patch the truesize member of the SKB clones is updated
> in similarity to the len member and by this the expected incremental
> of the advertised TCP Window Size is seen and the TCP connection
> data rate will not be unnecessarily limited.
> 
> Signed-off-by: Elof Vrigborn <elof.vrigborn@ericsson.com>
> ---
>  drivers/net/usb/cdc_ncm.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index f33ca6a..09923cd 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1083,6 +1083,7 @@ static int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
>  			if (!skb)
>  				goto error;
>  			skb->len = temp;
> +			skb->truesize = temp + sizeof(struct sk_buff);
>  			skb->data = ((u8 *)skb_in->data) + offset;
>  			skb_set_tail_pointer(skb, temp);
>  			usbnet_skb_return(dev, skb);

I find this patch dubious.

skb truesize is not meant to be skb->len + sizeof(struct sk_buff);

truesize is really accounting for the truesize of memory blocks, not the
used one.

Many drivers allocate a full 2Kbyte block, even if only 100 bytes are
used in it. If we want to reduce "truesize", then we perform a
"copybreak", to allocate a right sized skb. Some NICS do that for small
frames.

If you have TCP performance problems, this might be because of another
high level problem. This truesize underestimation is only hiding the
problem, but makes the whole machine more subject to OOM bugs.

Could you provide more information ?

RE: [PATCH 1/1] cdc_ncm: Fix TCP Window Size issue by CDC NCM driver

[Elof Vrigborn]

> 
> I find this patch dubious.
> 
> skb truesize is not meant to be skb->len + sizeof(struct sk_buff);
> 
> truesize is really accounting for the truesize of memory 
> blocks, not the used one.
> 
> Many drivers allocate a full 2Kbyte block, even if only 100 
> bytes are used in it. If we want to reduce "truesize", then 
> we perform a "copybreak", to allocate a right sized skb. Some 
> NICS do that for small frames.
> 
> If you have TCP performance problems, this might be because 
> of another high level problem. This truesize underestimation 
> is only hiding the problem, but makes the whole machine more 
> subject to OOM bugs.
> 
> Could you provide more information ?
> 
> 
> 
I'll explain further: The skb_in input of cdc_ncm_rx_fixup function holds the data of several ethernet packets received, as collected in one NCM packet. In CDC NCM driver cdc_ncm_rx_fixup function the skb_in is traversed and a new skb clone is created for each of the contained ethernet packets. Each of these clones are then modified by the len and data members to present only one ethernet packet each, but the truesize still is indicating the truesize of the parent skb which were holding the data of all clones. Each of the clones, but not the parent, will end up in the tcp_grow_window function now indicating the len of a single ethernet packet but a truesize of the collected ethernet packets. To the tcp_grow_window function of the TCP stack this seems as there is a huge overhead for each sk
 b, as it analyzes the skb clones but not the parent skb when comparing the len to the truesize when determining how to increase rcv_ssthres. By limiting rcv_ssthres, also the the advertised TCP window size will be limited, leading to the TCP throughput performance being low.

The original reason for simply cloning the skb holding the several packets, instead of any "copybreak" would probably be to limit the need for memory operations on the received data. I can not tell if this was a good or not so good choise.

Drivers that allocate 2Kbyte blocks and then use only a portion of that does in fact introduce the indicated overhead. But the situation with the CDC NCM driver is that the skb clones are only seemingly introducing an overhead. In reality, as indicated by the further non-used skb parent, there is in fact no such overhead.

Yes, we do have TCP throughput performance problems. By comparing the handling of data received through other interfaces we could conclude that the performance issue was due to that tcp_grow_window function of the TCP stack did limit the rcv_ssthres in Check #2 because of skb->truesize was much larger than the skb->len, for the abocve described clones.