* [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
@ 2018-11-09 19:43 Cong Wang
2018-11-10 1:39 ` Yunsheng Lin
2018-11-10 4:16 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Cong Wang @ 2018-11-09 19:43 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang
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);
dump_stack();
}
}
@@ -5779,7 +5785,7 @@ __sum16 __skb_gro_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);
}
NAPI_GRO_CB(skb)->csum = wsum;
diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
index 9062967575c4..7e55cfc69697 100644
--- a/net/sunrpc/socklib.c
+++ b/net/sunrpc/socklib.c
@@ -175,7 +175,7 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr, struct sk_buff *skb)
return -1;
if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
!skb->csum_complete_sw)
- netdev_rx_csum_fault(skb->dev);
+ netdev_rx_csum_fault(skb->dev, skb);
return 0;
no_checksum:
if (xdr_partial_copy_from_skb(xdr, 0, &desc, xdr_skb_read_bits) < 0)
--
2.19.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-09 19:43 [Patch net-next] net: dump more useful information in netdev_rx_csum_fault() Cong Wang
@ 2018-11-10 1:39 ` Yunsheng Lin
2018-11-10 1:42 ` Cong Wang
2018-11-10 4:16 ` David Miller
1 sibling, 1 reply; 8+ messages in thread
From: Yunsheng Lin @ 2018-11-10 1:39 UTC (permalink / raw)
To: Cong Wang, netdev
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?
Also, dev->features was dumped before this patch, why remove it?
> dump_stack();
> }
> }
> @@ -5779,7 +5785,7 @@ __sum16 __skb_gro_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);
> }
>
> NAPI_GRO_CB(skb)->csum = wsum;
> diff --git a/net/sunrpc/socklib.c b/net/sunrpc/socklib.c
> index 9062967575c4..7e55cfc69697 100644
> --- a/net/sunrpc/socklib.c
> +++ b/net/sunrpc/socklib.c
> @@ -175,7 +175,7 @@ int csum_partial_copy_to_xdr(struct xdr_buf *xdr, struct sk_buff *skb)
> return -1;
> if (unlikely(skb->ip_summed == CHECKSUM_COMPLETE) &&
> !skb->csum_complete_sw)
> - netdev_rx_csum_fault(skb->dev);
> + netdev_rx_csum_fault(skb->dev, skb);
> return 0;
> no_checksum:
> if (xdr_partial_copy_from_skb(xdr, 0, &desc, xdr_skb_read_bits) < 0)
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-10 1:39 ` Yunsheng Lin
@ 2018-11-10 1:42 ` Cong Wang
2018-11-10 2:02 ` Yunsheng Lin
0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2018-11-10 1:42 UTC (permalink / raw)
To: linyunsheng; +Cc: Linux Kernel Network Developers
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.
>
> Also, dev->features was dumped before this patch, why remove it?
Seriously? Where do I remove it? Please be specific. :)
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-10 1:42 ` Cong Wang
@ 2018-11-10 2:02 ` Yunsheng Lin
2018-11-10 2:09 ` Cong Wang
0 siblings, 1 reply; 8+ messages in thread
From: Yunsheng Lin @ 2018-11-10 2:02 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
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.
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-10 2:02 ` Yunsheng Lin
@ 2018-11-10 2:09 ` Cong Wang
2018-11-10 2:44 ` Yunsheng Lin
0 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2018-11-10 2:09 UTC (permalink / raw)
To: linyunsheng; +Cc: Linux Kernel Network Developers
On Fri, Nov 9, 2018 at 6:02 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> 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.
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-10 2:09 ` Cong Wang
@ 2018-11-10 2:44 ` Yunsheng Lin
0 siblings, 0 replies; 8+ messages in thread
From: Yunsheng Lin @ 2018-11-10 2:44 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers
On 2018/11/10 10:09, Cong Wang wrote:
> On Fri, Nov 9, 2018 at 6:02 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> 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.
>
>
> 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.
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Patch net-next] net: dump more useful information in netdev_rx_csum_fault()
2018-11-09 19:43 [Patch net-next] net: dump more useful information in netdev_rx_csum_fault() Cong Wang
2018-11-10 1:39 ` Yunsheng Lin
@ 2018-11-10 4:16 ` David Miller
2018-11-12 22:41 ` Cong Wang
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2018-11-10 4:16 UTC (permalink / raw)
To: xiyou.wangcong; +Cc: netdev
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 9 Nov 2018 11:43:33 -0800
> 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;
Didn't you move this function into net/core/skbuff.c? :-)
Please respin.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-11-13 8:37 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-09 19:43 [Patch net-next] net: dump more useful information in netdev_rx_csum_fault() Cong Wang
2018-11-10 1:39 ` Yunsheng Lin
2018-11-10 1:42 ` Cong Wang
2018-11-10 2:02 ` Yunsheng Lin
2018-11-10 2:09 ` Cong Wang
2018-11-10 2:44 ` Yunsheng Lin
2018-11-10 4:16 ` David Miller
2018-11-12 22:41 ` Cong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).