On Wed, Aug 21, 2024 at 03:37:11PM +0200, Przemek Kitszel wrote:
> From: Mateusz Polchlopek <[email protected]>
>
> Add devlink_fmsg_dump_skb() function that adds some diagnostic
> information about skb (like length, pkt type, MAC, etc) to devlink
> fmsg mechanism using bunch of devlink_fmsg_put() function calls.
>
> Signed-off-by: Mateusz Polchlopek <[email protected]>
> Signed-off-by: Przemek Kitszel <[email protected]>
...
> diff --git a/net/devlink/health.c b/net/devlink/health.c
> index acb8c0e174bb..b98ca650284c 100644
> --- a/net/devlink/health.c
> +++ b/net/devlink/health.c
> @@ -1241,3 +1241,70 @@ int devlink_nl_health_reporter_test_doit(struct
> sk_buff *skb,
>
> return reporter->ops->test(reporter, info->extack);
> }
> +
> +/**
> + * devlink_fmsg_dump_skb - Dump sk_buffer structure
> + * @fmsg: devlink formatted message pointer
> + * @skb: pointer to skb
> + *
> + * Dump diagnostic information about sk_buff structure, like headroom,
> length,
> + * tailroom, MAC, etc.
> + */
> +void devlink_fmsg_dump_skb(struct devlink_fmsg *fmsg, const struct sk_buff
> *skb)
> +{
> + struct skb_shared_info *sh = skb_shinfo(skb);
> + struct sock *sk = skb->sk;
> + bool has_mac, has_trans;
> +
> + has_mac = skb_mac_header_was_set(skb);
> + has_trans = skb_transport_header_was_set(skb);
> +
> + devlink_fmsg_pair_nest_start(fmsg, "skb");
> + devlink_fmsg_obj_nest_start(fmsg);
> + devlink_fmsg_put(fmsg, "actual len", skb->len);
> + devlink_fmsg_put(fmsg, "head len", skb_headlen(skb));
> + devlink_fmsg_put(fmsg, "data len", skb->data_len);
> + devlink_fmsg_put(fmsg, "tail len", skb_tailroom(skb));
> + devlink_fmsg_put(fmsg, "MAC", has_mac ? skb->mac_header : -1);
> + devlink_fmsg_put(fmsg, "MAC len",
> + has_mac ? skb_mac_header_len(skb) : -1);
> + devlink_fmsg_put(fmsg, "network hdr", skb->network_header);
> + devlink_fmsg_put(fmsg, "network hdr len",
> + has_trans ? skb_network_header_len(skb) : -1);
> + devlink_fmsg_put(fmsg, "transport hdr",
> + has_trans ? skb->transport_header : -1);
> + devlink_fmsg_put(fmsg, "csum", skb->csum);
Hi,
One minor nit here, which I don't think needs to stop progress of this
patchset. Sparse warns that:
error: no generic selection for 'restricted __wsum const [usertype] csum'
I believe this can be addressed by casting: (__force __u32) skb->csum,
perhaps incorporated into devlink_fmsg_put(). Which seems fine enough for
this case.
However, my observation is that there are a lot of sparse warnings
present in the tree due to similar issues around the use of __wsum.
And IMHO naked casts are error prone and not obviously correct to the
reader (me). So I wonder if there is some value in introducing some
helpers. E.g.
wsum_to_cpu()
cpu_to_wsum()
To my mind, that would clearly be out of scope for this patchset.
But It seems appropriate to raise this as it's been on my mind for a while.
...
