From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yunsheng Lin Subject: Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault() Date: Sat, 10 Nov 2018 10:44:24 +0800 Message-ID: <50c8053c-f987-60fb-6f92-c31d9c54c9a3@huawei.com> 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" Content-Transfer-Encoding: 7bit Cc: Linux Kernel Network Developers To: Cong Wang Return-path: Received: from szxga04-in.huawei.com ([45.249.212.190]:15080 "EHLO huawei.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728130AbeKJM1w (ORCPT ); Sat, 10 Nov 2018 07:27:52 -0500 Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.60]) by Forcepoint Email with ESMTP id 8B93A487BD9CC for ; Sat, 10 Nov 2018 10:44:27 +0800 (CST) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 2018/11/10 10:09, Cong Wang wrote: > 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. Ok. :) > > Thanks. > > . >