| Message ID | 20201005144838.851988-1-vladimir.oltean@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: always dump full packets with skb_dump | expand |
On 10/5/2020 7:48 AM, Vladimir Oltean wrote: > Currently skb_dump has a restriction to only dump full packet for the > first 5 socket buffers, then only headers will be printed. Remove this > arbitrary and confusing restriction, which is only documented vaguely > ("up to") in the comments above the prototype. > So, this limitation appeared very clearly in the original commit, 6413139dfc64 ("skbuff: increase verbosity when dumping skb data").. Searching the netdev list, that patch links back to this one as the original idea: https://patchwork.ozlabs.org/project/netdev/patch/20181121021309.6595-2-xiyou.wangcong@gmail.com/ I can't find any further justification on that limit. I suppose the primary reasoning being if you somehow call this function in a loop this would avoid dumping the entire packet over and over? Thanks, Jake > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > net/core/skbuff.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index e0774471f56d..720076a6e2b1 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -712,11 +712,10 @@ EXPORT_SYMBOL(kfree_skb_list); > * > * Must only be called from net_ratelimit()-ed paths. > * > - * Dumps up to can_dump_full whole packets if full_pkt, headers otherwise. > + * Dumps whole packets if full_pkt, only headers otherwise. > */ > void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt) > { > - static atomic_t can_dump_full = ATOMIC_INIT(5); > struct skb_shared_info *sh = skb_shinfo(skb); > struct net_device *dev = skb->dev; > struct sock *sk = skb->sk; > @@ -725,9 +724,6 @@ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt) > int headroom, tailroom; > int i, len, seg_len; > > - if (full_pkt) > - full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0; > - > if (full_pkt) > len = skb->len; > else >
On Mon, Oct 5, 2020 at 8:25 PM Jacob Keller <jacob.e.keller@intel.com> wrote: > > > > On 10/5/2020 7:48 AM, Vladimir Oltean wrote: > > Currently skb_dump has a restriction to only dump full packet for the > > first 5 socket buffers, then only headers will be printed. Remove this > > arbitrary and confusing restriction, which is only documented vaguely > > ("up to") in the comments above the prototype. > > > > So, this limitation appeared very clearly in the original commit, > 6413139dfc64 ("skbuff: increase verbosity when dumping skb data").. > > Searching the netdev list, that patch links back to this one as the > original idea: > > https://patchwork.ozlabs.org/project/netdev/patch/20181121021309.6595-2-xiyou.wangcong@gmail.com/ > > I can't find any further justification on that limit. I suppose the > primary reasoning being if you somehow call this function in a loop this > would avoid dumping the entire packet over and over? Not in a loop per se, but indeed to avoid unbounded writing to the kernel log. skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault. Previously when these were triggered, a few example bad packets were sufficient to debug the issue. A full dump can add a lot of data to the kernel log, so I limited to what is strictly needed.
On Tue, Oct 06, 2020 at 07:30:13AM -0400, Willem de Bruijn wrote: > skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault. > Previously when these were triggered, a few example bad packets were > sufficient to debug the issue. Yes, and it's only netdev_rx_csum_fault that matters, because skb_warn_bad_offload calls with full_pkt=false anyway. During the times when I had netdev_rx_csum_fault triggered, it was pretty bad anyway. I don't think that full_pkt getting unset after 5 skbs made too big of a difference. > A full dump can add a lot of data to the kernel log, so I limited to > what is strictly needed. Yes, well my expectation is that other people are using skb_dump for debugging, even beyond those 2 callers in the mainline kernel. And when they want to dump with full_pkt=true, they really want to dump with full_pkt=true. Thanks, -Vladimir
On Tue, Oct 6, 2020 at 7:43 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Tue, Oct 06, 2020 at 07:30:13AM -0400, Willem de Bruijn wrote: > > skb_dump is called from skb_warn_bad_offload and netdev_rx_csum_fault. > > Previously when these were triggered, a few example bad packets were > > sufficient to debug the issue. > > Yes, and it's only netdev_rx_csum_fault that matters, because > skb_warn_bad_offload calls with full_pkt=false anyway. > > During the times when I had netdev_rx_csum_fault triggered, it was > pretty bad anyway. I don't think that full_pkt getting unset after 5 > skbs made too big of a difference. > > > A full dump can add a lot of data to the kernel log, so I limited to > > what is strictly needed. > > Yes, well my expectation is that other people are using skb_dump for > debugging, even beyond those 2 callers in the mainline kernel. And when > they want to dump with full_pkt=true, they really want to dump with > full_pkt=true. Sure, that makes sense.
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Mon, 5 Oct 2020 17:48:38 +0300 > Currently skb_dump has a restriction to only dump full packet for the > first 5 socket buffers, then only headers will be printed. Remove this > arbitrary and confusing restriction, which is only documented vaguely > ("up to") in the comments above the prototype. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Applied, thank you.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index e0774471f56d..720076a6e2b1 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -712,11 +712,10 @@ EXPORT_SYMBOL(kfree_skb_list); * * Must only be called from net_ratelimit()-ed paths. * - * Dumps up to can_dump_full whole packets if full_pkt, headers otherwise. + * Dumps whole packets if full_pkt, only headers otherwise. */ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt) { - static atomic_t can_dump_full = ATOMIC_INIT(5); struct skb_shared_info *sh = skb_shinfo(skb); struct net_device *dev = skb->dev; struct sock *sk = skb->sk; @@ -725,9 +724,6 @@ void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt) int headroom, tailroom; int i, len, seg_len; - if (full_pkt) - full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0; - if (full_pkt) len = skb->len; else
Currently skb_dump has a restriction to only dump full packet for the first 5 socket buffers, then only headers will be printed. Remove this arbitrary and confusing restriction, which is only documented vaguely ("up to") in the comments above the prototype. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- net/core/skbuff.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)