Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210824125140.190253-1-yan2228598786@gmail.com>
In-Reply-To: <20210824125140.190253-1-yan2228598786@gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Tue, 24 Aug 2021 08:22:11 -0700
Message-ID: <CANn89i+-EnK-zZ_kXsVAW_Or+8w3V3F4orbt40GFP2zQrr6gvw@mail.gmail.com>
Subject: Re: [PATCH] net: tcp_drop adds `reason` parameter for tracing
To:     Zhongya Yan <yan2228598786@gmail.com>
Cc:     Jakub Kicinski <kuba@kernel.org>, netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ingo Molnar <mingo@redhat.com>,
        David Miller <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>, hengqi.chen@gmail.com,
        Yonghong Song <yhs@fb.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 24, 2021 at 5:52 AM Zhongya Yan <yan2228598786@gmail.com> wrote:
>
> When using `tcp_drop(struct sock *sk, struct sk_buff *skb)` we can
> not tell why we need to delete `skb`. To solve this problem I updated the
> method `tcp_drop(struct sock *sk, struct sk_buff *skb, enum tcp_drop_reason reason)`
> to include the source of the deletion when it is done, so you can
> get an idea of the reason for the deletion based on the source.
>
> The current purpose is mainly derived from the suggestions
> of `Yonghong Song` and `brendangregg`:
>
> https://github.com/iovisor/bcc/issues/3533.
>
> "It is worthwhile to mention the context/why we want to this
> tracepoint with bcc issue https://github.com/iovisor/bcc/issues/3533.
> Mainly two reasons: (1). tcp_drop is a tiny function which
> may easily get inlined, a tracepoint is more stable, and (2).
> tcp_drop does not provide enough information on why it is dropped.
> " by Yonghong Song
>
> Signed-off-by: Zhongya Yan <yan2228598786@gmail.com>
> ---

That is a good start, but really if people want to use this
tracepoint, they will hit a wall soon.


>         return true;
>
>  discard:

There are many " goto discards;" in this function, so using a common
value ("TCP_VALIDATE_INCOMING" ) is not helpful.


> -       tcp_drop(sk, skb);
> +       tcp_drop(sk, skb, TCP_VALIDATE_INCOMING);
>         return false;
>  }
>
> @@ -5905,7 +5915,7 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
>         TCP_INC_STATS(sock_net(sk), TCP_MIB_INERRS);
>
>  discard:
> -       tcp_drop(sk, skb);

Same here.

> +       tcp_drop(sk, skb, TCP_RCV_ESTABLISHED);
>  }
>  EXPORT_SYMBOL(tcp_rcv_established);
>
> @@ -6196,7 +6206,7 @@ static int tcp_rcv_synsent_state_process(struct sock *sk, struct sk_buff *skb,
>                                                   TCP_DELACK_MAX, TCP_RTO_MAX);
>
>  discard:
> -                       tcp_drop(sk, skb);
> +                       tcp_drop(sk, skb, TCP_RCV_SYNSENT_STATE_PROCESS);
>                         return 0;
>                 } else {
>                         tcp_send_ack(sk);
> @@ -6568,7 +6578,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
>
>         if (!queued) {
>  discard:

same here.

> -               tcp_drop(sk, skb);
> +               tcp_drop(sk, skb, TCP_RCV_STATE_PROCESS);
>         }
>         return 0;
>  }
> --
> 2.25.1
>