Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <a78450d6-698a-e819-89f4-ebbe03fdf971@gmail.com>
References: <1532524010-11855-1-git-send-email-laoar.shao@gmail.com>
 <047bc12f-9eb5-e512-10c5-6334c494c77e@gmail.com> <CALOAHbCC=oWgzcA=VhkXq0rVLBa5_TvtbKtTqESGrgxvYTtH6w@mail.gmail.com>
 <096a8a2f-ea32-2107-b2af-c246450cc3b9@gmail.com> <CALOAHbASvcEdudqpit-c=RRZ8xVD+G+0jU-ynp9PsH4_mU7xjw@mail.gmail.com>
 <CALOAHbAHgEF_=w_hbd_03NOH3h9m-4W=UJ0HCYGK0BtEyq=5Tw@mail.gmail.com>
 <70428ad9-5d59-faed-4384-36190939a19d@gmail.com> <CALOAHbBKSPQdo9bA5VFLvR448Ag30nE2Td1f6204MPZ7Nfy7ag@mail.gmail.com>
 <a78450d6-698a-e819-89f4-ebbe03fdf971@gmail.com>
From:   Yafang Shao <laoar.shao@gmail.com>
Date:   Thu, 26 Jul 2018 12:42:25 +0800
Message-ID: <CALOAHbCLN=mZGOjDg5ML60YtUZWmTTWOHuTwj5tAxyFoabKLUw@mail.gmail.com>
Subject: Re: [PATCH net-next] tcp: add SNMP counter for the number of packets
 pruned from ofo queue
To:     Eric Dumazet <eric.dumazet@gmail.com>
Cc:     David Miller <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 26, 2018 at 12:36 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 07/25/2018 09:31 PM, Yafang Shao wrote:
>> On Thu, Jul 26, 2018 at 12:06 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>>
>>>
>>> On 07/25/2018 06:42 PM, Yafang Shao wrote:
>>>
>>>>
>>>> Hi Eric,
>>>>
>>>> LINUX_MIB_TCPOFOQUEUE, LINUX_MIB_TCPOFODROP and LINUX_MIB_TCPOFOMERGE
>>>> are all for the number of SKBs, but only  LINUX_MIB_OFOPRUNED is for
>>>> the event, that could lead misunderstading.
>>>> So I think introducing a counter for the number of SKB pruned could be
>>>> better, that could help us to track the whole behavior of ofo queue.
>>>> That is why I submit this patch.
>>>
>>> Sure, and I said your patch had issues.
>>> You mixed 'packets' and 'skbs' but apparently you do not get my point.
>>>
>>
>> I had noticed that I made this mistake.
>>
>>> I would rather not add another SNMP counter, and refine the current one,
>>> trying to track something more meaningful.
>>>
>>> The notion of 'skb' is internal to the kernel, and can not be mapped easily
>>> to 'number of network segments' which probably is more useful for the user.
>>>
>>> I will send this instead :
>>>
>>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>>> index d51fa358b2b196d0f9c258b24354813f2128a675..141a062abd0660c8f6d049de1dc7c7ecf7a7230d 100644
>>> --- a/net/ipv4/tcp_input.c
>>> +++ b/net/ipv4/tcp_input.c
>>> @@ -5001,18 +5001,19 @@ static bool tcp_prune_ofo_queue(struct sock *sk)
>>>  {
>>>         struct tcp_sock *tp = tcp_sk(sk);
>>>         struct rb_node *node, *prev;
>>> +       unsigned int segs = 0;
>>>         int goal;
>>>
>>>         if (RB_EMPTY_ROOT(&tp->out_of_order_queue))
>>>                 return false;
>>>
>>> -       NET_INC_STATS(sock_net(sk), LINUX_MIB_OFOPRUNED);
>>>         goal = sk->sk_rcvbuf >> 3;
>>>         node = &tp->ooo_last_skb->rbnode;
>>>         do {
>>>                 prev = rb_prev(node);
>>>                 rb_erase(node, &tp->out_of_order_queue);
>>>                 goal -= rb_to_skb(node)->truesize;
>>> +               segs += max_t(u16, 1, skb_shinfo(rb_to_skb(node))->gso_segs);
>>>                 tcp_drop(sk, rb_to_skb(node));
>>>                 if (!prev || goal <= 0) {
>>>                         sk_mem_reclaim(sk);
>>> @@ -5023,6 +5024,7 @@ static bool tcp_prune_ofo_queue(struct sock *sk)
>>>                 }
>>>                 node = prev;
>>>         } while (node);
>>> +       NET_ADD_STATS(sock_net(sk), LINUX_MIB_OFOPRUNED, segs);
>>>         tp->ooo_last_skb = rb_to_skb(prev);
>>>
>>>         /* Reset SACK state.  A conforming SACK implementation will
>>>
>>
>> You patch will make it more meaningful.
>> How about the other ones?
>
> Same thing really.
>
> Note that tcp_collapse() does not currently propagate skb_shinfo(skb)->gso_segs
> when rebuilding the skbs, so this will need a fix.
>

Got it.
Thank you very much for your comment.

Thanks
Yafang