From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cong Wang Subject: Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault() Date: Fri, 9 Nov 2018 18:09:55 -0800 Message-ID: References: <20181109194333.19949-1-xiyou.wangcong@gmail.com> <2fe5fbef-ece4-1c0a-189b-514074a7f46b@huawei.com> <17be1af6-9aaf-593c-a68f-932d63aefbd5@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Linux Kernel Network Developers To: linyunsheng@huawei.com Return-path: Received: from mail-pl1-f193.google.com ([209.85.214.193]:42115 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728032AbeKJLxZ (ORCPT ); Sat, 10 Nov 2018 06:53:25 -0500 Received: by mail-pl1-f193.google.com with SMTP id t6-v6so1719041plo.9 for ; Fri, 09 Nov 2018 18:10:07 -0800 (PST) In-Reply-To: <17be1af6-9aaf-593c-a68f-932d63aefbd5@huawei.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Nov 9, 2018 at 6:02 PM Yunsheng Lin wrote: > > On 2018/11/10 9:42, Cong Wang wrote: > > On Fri, Nov 9, 2018 at 5:39 PM Yunsheng Lin wrote: > >> > >> On 2018/11/10 3:43, Cong Wang wrote: > >>> Currently netdev_rx_csum_fault() only shows a device name, > >>> we need more information about the skb for debugging. > >>> > >>> Sample output: > >>> > >>> ens3: hw csum failure > >>> dev features: 0x0000000000014b89 > >>> skb len=84 data_len=0 gso_size=0 gso_type=0 ip_summed=0 csum=0, csum_complete_sw=0, csum_valid=0 > >>> > >>> Signed-off-by: Cong Wang > >>> --- > >>> include/linux/netdevice.h | 5 +++-- > >>> net/core/datagram.c | 6 +++--- > >>> net/core/dev.c | 10 ++++++++-- > >>> net/sunrpc/socklib.c | 2 +- > >>> 4 files changed, 15 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > >>> index 857f8abf7b91..fabcd9fa6cf7 100644 > >>> --- a/include/linux/netdevice.h > >>> +++ b/include/linux/netdevice.h > >>> @@ -4332,9 +4332,10 @@ static inline bool can_checksum_protocol(netdev_features_t features, > >>> } > >>> > >>> #ifdef CONFIG_BUG > >>> -void netdev_rx_csum_fault(struct net_device *dev); > >>> +void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb); > >>> #else > >>> -static inline void netdev_rx_csum_fault(struct net_device *dev) > >>> +static inline void netdev_rx_csum_fault(struct net_device *dev, > >>> + struct sk_buff *skb) > >>> { > >>> } > >>> #endif > >>> diff --git a/net/core/datagram.c b/net/core/datagram.c > >>> index 57f3a6fcfc1e..d8f4d55cd6c5 100644 > >>> --- a/net/core/datagram.c > >>> +++ b/net/core/datagram.c > >>> @@ -736,7 +736,7 @@ __sum16 __skb_checksum_complete_head(struct sk_buff *skb, int len) > >>> if (likely(!sum)) { > >>> if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) && > >>> !skb->csum_complete_sw) > >>> - netdev_rx_csum_fault(skb->dev); > >>> + netdev_rx_csum_fault(skb->dev, skb); > >>> } > >>> if (!skb_shared(skb)) > >>> skb->csum_valid = !sum; > >>> @@ -756,7 +756,7 @@ __sum16 __skb_checksum_complete(struct sk_buff *skb) > >>> if (likely(!sum)) { > >>> if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) && > >>> !skb->csum_complete_sw) > >>> - netdev_rx_csum_fault(skb->dev); > >>> + netdev_rx_csum_fault(skb->dev, skb); > >>> } > >>> > >>> if (!skb_shared(skb)) { > >>> @@ -810,7 +810,7 @@ int skb_copy_and_csum_datagram_msg(struct sk_buff *skb, > >>> > >>> if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) && > >>> !skb->csum_complete_sw) > >>> - netdev_rx_csum_fault(NULL); > >>> + netdev_rx_csum_fault(NULL, skb); > >>> } > >>> return 0; > >>> fault: > >>> diff --git a/net/core/dev.c b/net/core/dev.c > >>> index 0ffcbdd55fa9..2b337df26117 100644 > >>> --- a/net/core/dev.c > >>> +++ b/net/core/dev.c > >>> @@ -3091,10 +3091,16 @@ EXPORT_SYMBOL(__skb_gso_segment); > >>> > >>> /* Take action when hardware reception checksum errors are detected. */ > >>> #ifdef CONFIG_BUG > >>> -void netdev_rx_csum_fault(struct net_device *dev) > >>> +void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb) > >>> { > >>> if (net_ratelimit()) { > >>> pr_err("%s: hw csum failure\n", dev ? dev->name : ""); > >>> + if (dev) > >>> + pr_err("dev features: %pNF\n", &dev->features); > >>> + pr_err("skb len=%d data_len=%d gso_size=%d gso_type=%d ip_summed=%d csum=%x, csum_complete_sw=%d, csum_valid=%d\n", > >>> + skb->len, skb->data_len, skb_shinfo(skb)->gso_size, > >>> + skb_shinfo(skb)->gso_type, skb->ip_summed, skb->csum, > >>> + skb->csum_complete_sw, skb->csum_valid); > >> > >> > >> This function also have the netdev available, use netdev_err to log the error? > > > > It is apparently not me who picked pr_err() from the beginning, > > I just follow that pr_err(). If you are not happy with it, please send > > a followup. > > Yes, but perhaps it is something to improve. Sure, no one stops you from improving it in a followup patch. :) > When using the netdev, then maybe it does not have to check if dev is null, because > netdev_err has handled the netdev being NULL case. > Maybe I missed something that netdev can not be used here? > If not, maybe I can send a followup. > Maybe. Again, my patch intends to add a few debugging logs, not to convert pr_err() to whatever else, they are totally different goals. I choose pr_err() only because I follow the existing one, not to say which one is better than the other. Thanks.