Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2018/11/10 9:42, Cong Wang wrote:
> On Fri, Nov 9, 2018 at 5:39 PM Yunsheng Lin <linyunsheng@huawei.com> 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 <xiyou.wangcong@gmail.com>
>>> ---
>>>  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 : "<unknown>");
>>> +             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.
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.

> 
> 
>>
>> Also, dev->features was dumped before this patch, why remove it?
> 
> Seriously? Where do I remove it? Please be specific. :)

Sorry, I missed that, I thought it was removed when adding the new log.

> 
> .
>