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: <20210902093358.28345-1-yan2228598786@gmail.com>
In-Reply-To: <20210902093358.28345-1-yan2228598786@gmail.com>
From:   Eric Dumazet <edumazet@google.com>
Date:   Thu, 2 Sep 2021 08:44:57 -0700
Message-ID: <CANn89iLHg8cjLFKVzO+CkewLs_NkjEjQGetwARVnkuKRS9iUfQ@mail.gmail.com>
Subject: Re: [PATCH] net: tcp_drop adds `reason` and SNMP parameters for
 tracing v3
To:     Zhongya Yan <yan2228598786@gmail.com>
Cc:     netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jakub Kicinski <kuba@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>,
        Brendan Gregg <brendan.d.gregg@gmail.com>, 2228598786@qq.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Sep 2, 2021 at 2:34 AM Zhongya Yan <yan2228598786@gmail.com> wrote:
>
> I used the suggestion from `Brendan Gregg`. In addition to the
> `reason` parameter there is also the `field` parameter pointing
> to `SNMP` to distinguish the `tcp_drop` cause. I know what I
> submitted is not accurate, so I am submitting the current
> patch to get comments and criticism from everyone so that I
> can submit better code and solutions.And of course to make me
> more familiar and understand the `linux` kernel network code.
> Thank you everyone!
>
> Signed-off-by: Zhongya Yan <yan2228598786@gmail.com>
> ---
>  include/trace/events/tcp.h |  39 +++---------
>  net/ipv4/tcp_input.c       | 126 ++++++++++++++-----------------------
>  2 files changed, 57 insertions(+), 108 deletions(-)
>
> diff --git a/include/trace/events/tcp.h b/include/trace/events/tcp.h
> index 699539702ea9..80892660458e 100644
> --- a/include/trace/events/tcp.h
> +++ b/include/trace/events/tcp.h
> @@ -371,28 +371,10 @@ DEFINE_EVENT(tcp_event_skb, tcp_bad_csum,
>         TP_ARGS(skb)
>  );
>
> -// from author @{Steven Rostedt}
> -#define TCP_DROP_REASON                                                        \
> -       REASON_STRING(TCP_OFO_QUEUE,            ofo_queue)                      \
> -       REASON_STRING(TCP_DATA_QUEUE_OFO,               data_queue_ofo)                 \
> -       REASON_STRING(TCP_DATA_QUEUE,           data_queue)                     \
> -       REASON_STRING(TCP_PRUNE_OFO_QUEUE,              prune_ofo_queue)                \
> -       REASON_STRING(TCP_VALIDATE_INCOMING,    validate_incoming)              \
> -       REASON_STRING(TCP_RCV_ESTABLISHED,              rcv_established)                \
> -       REASON_STRING(TCP_RCV_SYNSENT_STATE_PROCESS, rcv_synsent_state_process) \
> -       REASON_STRINGe(TCP_RCV_STATE_PROCESS,   rcv_state_proces)

??? On which tree / branch this patch is based on ?

> -
> -#undef REASON_STRING
> -#undef REASON_STRINGe
> -
> -#define REASON_STRING(code, name) {code , #name},
> -#define REASON_STRINGe(code, name) {code, #name}
> -
> -
>  TRACE_EVENT(tcp_drop,
> -               TP_PROTO(struct sock *sk, struct sk_buff *skb, __u32 reason),
> +               TP_PROTO(struct sock *sk, struct sk_buff *skb, int field, const char *reason),
>
> -               TP_ARGS(sk, skb, reason),
> +               TP_ARGS(sk, skb, field, reason),
>
>                 TP_STRUCT__entry(
>                         __array(__u8, saddr, sizeof(struct sockaddr_in6))
> @@ -409,9 +391,8 @@ TRACE_EVENT(tcp_drop,
>                         __field(__u32, srtt)
>                         __field(__u32, rcv_wnd)
>                         __field(__u64, sock_cookie)
> -                       __field(__u32, reason)
> -                       __field(__u32, reason_code)
> -                       __field(__u32, reason_line)
> +                       __field(int, field)
> +                       __string(reason, reason)
>                         ),
>
>                 TP_fast_assign(
> @@ -437,21 +418,19 @@ TRACE_EVENT(tcp_drop,
>                                 __entry->ssthresh = tcp_current_ssthresh(sk);
>                                 __entry->srtt = tp->srtt_us >> 3;
>                                 __entry->sock_cookie = sock_gen_cookie(sk);
> -                               __entry->reason = reason;
> -                               __entry->reason_code = TCP_DROP_CODE(reason);
> -                               __entry->reason_line = TCP_DROP_LINE(reason);
> +                               __entry->field = field;
> +
> +                               __assign_str(reason, reason);
>                 ),
>
>                 TP_printk("src=%pISpc dest=%pISpc mark=%#x data_len=%d snd_nxt=%#x snd_una=%#x \
>                                 snd_cwnd=%u ssthresh=%u snd_wnd=%u srtt=%u rcv_wnd=%u \
> -                               sock_cookie=%llx reason=%d reason_type=%s reason_line=%d",
> +                               sock_cookie=%llx field=%d reason=%s",
>                                 __entry->saddr, __entry->daddr, __entry->mark,
>                                 __entry->data_len, __entry->snd_nxt, __entry->snd_una,
>                                 __entry->snd_cwnd, __entry->ssthresh, __entry->snd_wnd,
>                                 __entry->srtt, __entry->rcv_wnd, __entry->sock_cookie,
> -                               __entry->reason,
> -                               __print_symbolic(__entry->reason_code, TCP_DROP_REASON),
> -                               __entry->reason_line)
> +                               field, __get_str(reason))
>  );
>
>  #endif /* _TRACE_TCP_H */
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index b2bc49f1f0de..bd33fd466e1e 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -100,7 +100,6 @@ int sysctl_tcp_max_orphans __read_mostly = NR_FILE;
>  #define FLAG_UPDATE_TS_RECENT  0x4000 /* tcp_replace_ts_recent() */
>  #define FLAG_NO_CHALLENGE_ACK  0x8000 /* do not call tcp_send_challenge_ack()  */
>  #define FLAG_ACK_MAYBE_DELAYED 0x10000 /* Likely a delayed ACK */
> -#define FLAG_DSACK_TLP         0x20000 /* DSACK for tail loss probe */
>
>  #define FLAG_ACKED             (FLAG_DATA_ACKED|FLAG_SYN_ACKED)
>  #define FLAG_NOT_DUP           (FLAG_DATA|FLAG_WIN_UPDATE|FLAG_ACKED)
> @@ -455,12 +454,11 @@ static void tcp_sndbuf_expand(struct sock *sk)
>   */
>
>  /* Slow part of check#2. */
> -static int __tcp_grow_window(const struct sock *sk, const struct sk_buff *skb,
> -                            unsigned int skbtruesize)
> +static int __tcp_grow_window(const struct sock *sk, const struct sk_buff *skb)

???

>  {
>         struct tcp_sock *tp = tcp_sk(sk);
>         /* Optimize this! */
> -       int truesize = tcp_win_from_space(sk, skbtruesize) >> 1;
> +       int truesize = tcp_win_from_space(sk, skb->truesize) >> 1;

???

>         int window = tcp_win_from_space(sk, sock_net(sk)->ipv4.sysctl_tcp_rmem[2]) >> 1;
>
>         while (tp->rcv_ssthresh <= window) {
> @@ -473,27 +471,7 @@ static int __tcp_grow_window(const struct sock *sk, const struct sk_buff *skb,
>         return 0;
>  }
>
> -/* Even if skb appears to have a bad len/truesize ratio, TCP coalescing
> - * can play nice with us, as sk_buff and skb->head might be either
> - * freed or shared with up to MAX_SKB_FRAGS segments.
> - * Only give a boost to drivers using page frag(s) to hold the frame(s),
> - * and if no payload was pulled in skb->head before reaching us.
> - */
> -static u32 truesize_adjust(bool adjust, const struct sk_buff *skb)
> -{
> -       u32 truesize = skb->truesize;
> -
> -       if (adjust && !skb_headlen(skb)) {
> -               truesize -= SKB_TRUESIZE(skb_end_offset(skb));
> -               /* paranoid check, some drivers might be buggy */
> -               if (unlikely((int)truesize < (int)skb->len))
> -                       truesize = skb->truesize;
> -       }
> -       return truesize;
> -}

It seems clear you are doing wrong things.

Have you silently reverted a prior patch ?