Re: [PATCH net-next 2/2] tcp: propagate GSO to the new skb built in tcp collapse

[Yafang Shao]

On Sat, Jul 28, 2018 at 11:13 AM, Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> On Fri, Jul 27, 2018 at 8:02 PM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>>
>> Currently the collapsed SKB doesn't propagate the GSO information to the
>> new SKB.
>> The GSO should be propagated for better tracking, i.e. when this SKB is
>> dropped we could know how many network segments are dropped.
>
> What is "the GSO"  ?
>

I mean gso_segs, gso_type and gso_size, which are all set in GRO.

>>
>> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
>> ---
>>  net/ipv4/tcp_input.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index 90f83eb..af52e4e 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -4893,6 +4893,8 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>>                 if (!nskb)
>>                         break;
>>
>> +               skb_shinfo(nskb)->gso_size = skb_shinfo(skb)->gso_size;
>> +               skb_shinfo(nskb)->gso_type = skb_shinfo(skb)->gso_type;
>
> Why gso_size and gso_type are important ?
>
> Where later in the stack these values are used ?
>

I'm not sure it is important or not.
I just worry it may be used latter.

>>                 memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
>>  #ifdef CONFIG_TLS_DEVICE
>>                 nskb->decrypted = skb->decrypted;
>> @@ -4906,18 +4908,24 @@ void tcp_rbtree_insert(struct rb_root *root, struct sk_buff *skb)
>>
>>                 /* Copy data, releasing collapsed skbs. */
>>                 while (copy > 0) {
>> -                       int offset = start - TCP_SKB_CB(skb)->seq;
>>                         int size = TCP_SKB_CB(skb)->end_seq - start;
>> +                       int offset = start - TCP_SKB_CB(skb)->seq;
>
>>
>>                         BUG_ON(offset < 0);
>>                         if (size > 0) {
>> -                               size = min(copy, size);
>> +                               if (copy >= size)
>> +                                       skb_shinfo(nskb)->gso_segs +=
>> +                                               max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>> +                               else
>> +                                       size = copy;
>> +
>
> So... what happens if copy was partial ?
>

In the current patch, if copy was parial, the gso_segs are in the
orignal SKB as it will not be freed now.
If that is not ok, what about the bellow change ?

else {
    size = copy;
    skb_shinfo(nskb)->gso_segs +=  DIV_ROUND_UP(size,
skb_shinfo(nskb)->gso_size);
}

> Your patch does not really fix the uncertainty, it merely shifts it a bit.