Re: [PATCH] net: tcp_drop adds `reason` and SNMP parameters for tracing v3

From: Eric Dumazet
Date: Thu Sep 02 2021 - 11:45:17 EST


On Thu, Sep 2, 2021 at 2:34 AM Zhongya Yan <yan2228598786@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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 ?