Date:   Sat, 5 Feb 2022 15:56:28 +0800
From:   Menglong Dong <menglong8.dong@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>, David Ahern <dsahern@...nel.org>,
        Ido Schimmel <idosch@...sch.org>
Cc:     Neil Horman <nhorman@...driver.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Menglong Dong <imagedong@...cent.com>
Subject: Re: [PATCH v5 net-next] net: drop_monitor: support drop reason

On Fri, Feb 4, 2022 at 10:08 PM <menglong8.dong@...il.com> wrote:
>
> From: Menglong Dong <imagedong@...cent.com>
>
> In the commit c504e5c2f964 ("net: skb: introduce kfree_skb_reason()")
> drop reason is introduced to the tracepoint of kfree_skb. Therefore,
> drop_monitor is able to report the drop reason to users by netlink.
>
> The drop reasons are reported as string to users, which is exactly
> the same as what we do when reporting it to ftrace.
>
> Signed-off-by: Menglong Dong <imagedong@...cent.com>
> ---
> v5:
> - check if drop reason larger than SKB_DROP_REASON_MAX
>
> v4:
> - report drop reasons as string
>
> v3:
> - referring to cb->reason and cb->pc directly in
>   net_dm_packet_report_fill()
>
> v2:
> - get a pointer to struct net_dm_skb_cb instead of local var for
>   each field
> ---
>  include/uapi/linux/net_dropmon.h |  1 +
>  net/core/drop_monitor.c          | 29 +++++++++++++++++++++++++----
>  2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/net_dropmon.h b/include/uapi/linux/net_dropmon.h
> index 66048cc5d7b3..1bbea8f0681e 100644
> --- a/include/uapi/linux/net_dropmon.h
> +++ b/include/uapi/linux/net_dropmon.h
> @@ -93,6 +93,7 @@ enum net_dm_attr {
>         NET_DM_ATTR_SW_DROPS,                   /* flag */
>         NET_DM_ATTR_HW_DROPS,                   /* flag */
>         NET_DM_ATTR_FLOW_ACTION_COOKIE,         /* binary */
> +       NET_DM_ATTR_REASON,                     /* string */
>
>         __NET_DM_ATTR_MAX,
>         NET_DM_ATTR_MAX = __NET_DM_ATTR_MAX - 1
> diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
> index 7b288a121a41..2d1c8e8dec83 100644
> --- a/net/core/drop_monitor.c
> +++ b/net/core/drop_monitor.c
> @@ -48,6 +48,16 @@
>  static int trace_state = TRACE_OFF;
>  static bool monitor_hw;
>
> +#undef EM
> +#undef EMe
> +
> +#define EM(a, b)       [a] = #b,
> +#define EMe(a, b)      [a] = #b
> +
> +static const char *drop_reasons[SKB_DROP_REASON_MAX + 1] = {
> +       TRACE_SKB_DROP_REASON
> +};
> +
>  /* net_dm_mutex
>   *
>   * An overall lock guarding every operation coming from userspace.
> @@ -126,6 +136,7 @@ struct net_dm_skb_cb {
>                 struct devlink_trap_metadata *hw_metadata;
>                 void *pc;
>         };
> +       enum skb_drop_reason reason;
>  };
>
>  #define NET_DM_SKB_CB(__skb) ((struct net_dm_skb_cb *)&((__skb)->cb[0]))
> @@ -498,6 +509,7 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
>  {
>         ktime_t tstamp = ktime_get_real();
>         struct per_cpu_dm_data *data;
> +       struct net_dm_skb_cb *cb;
>         struct sk_buff *nskb;
>         unsigned long flags;
>
> @@ -508,7 +520,9 @@ static void net_dm_packet_trace_kfree_skb_hit(void *ignore,
>         if (!nskb)
>                 return;
>
> -       NET_DM_SKB_CB(nskb)->pc = location;
> +       cb = NET_DM_SKB_CB(nskb);
> +       cb->reason = reason;
> +       cb->pc = location;
>         /* Override the timestamp because we care about the time when the
>          * packet was dropped.
>          */
> @@ -606,8 +620,9 @@ static int net_dm_packet_report_in_port_put(struct sk_buff *msg, int ifindex,
>  static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
>                                      size_t payload_len)
>  {
> -       u64 pc = (u64)(uintptr_t) NET_DM_SKB_CB(skb)->pc;
> +       struct net_dm_skb_cb *cb = NET_DM_SKB_CB(skb);
>         char buf[NET_DM_MAX_SYMBOL_LEN];
> +       enum skb_drop_reason reason;
>         struct nlattr *attr;
>         void *hdr;
>         int rc;
> @@ -620,10 +635,16 @@ static int net_dm_packet_report_fill(struct sk_buff *msg, struct sk_buff *skb,
>         if (nla_put_u16(msg, NET_DM_ATTR_ORIGIN, NET_DM_ORIGIN_SW))
>                 goto nla_put_failure;
>
> -       if (nla_put_u64_64bit(msg, NET_DM_ATTR_PC, pc, NET_DM_ATTR_PAD))
> +       if (nla_put_u64_64bit(msg, NET_DM_ATTR_PC, (u64)(uintptr_t)cb->pc,
> +                             NET_DM_ATTR_PAD))
> +               goto nla_put_failure;
> +
> +       reason = cb->reason;
> +       if (reason < SKB_DROP_REASON_MAX &&
> +           nla_put_string(msg, NET_DM_ATTR_REASON, drop_reasons[reason]))
>                 goto nla_put_failure;

I guess I made a mistake here: assuming that the enum is unsigned.
Please ignore this version, I'll make a new one.

Thanks!
Menglong Dong

>
> -       snprintf(buf, sizeof(buf), "%pS", NET_DM_SKB_CB(skb)->pc);
> +       snprintf(buf, sizeof(buf), "%pS", cb->pc);
>         if (nla_put_string(msg, NET_DM_ATTR_SYMBOL, buf))
>                 goto nla_put_failure;
>
> --
> 2.34.1
>

Powered by blists - more mailing lists