2015-02-13 05:04:55

by Hiroshi Shimamoto

[permalink] [raw]
Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

> > > -----Original Message-----
> > > From: Hiroshi Shimamoto [mailto:[email protected]]
> > > Sent: Monday, February 09, 2015 6:29 PM
> > > To: Kirsher, Jeffrey T
> > > Cc: Alexander Duyck; Skidmore, Donald C; Bjørn Mork; e1000-
> > > [email protected]; [email protected]; Choi, Sy Jong; linux-
> > > [email protected]; David Laight; Hayato Momma
> > > Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to
> > > enable MC promiscuous mode
> > >
> > > > > > > Can you please fix up your patches based on my tree:
> > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
> > > > > >
> > > > > > Yes. I haven't noticed your tree.
> > > > > > Will resend patches against it.
> > > > > >
> > > > >
> > > > > I encountered an issue with your tree, the commit id is below.
> > > > >
> > > > > $ git log | head
> > > > > commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
> > > > > Author: Rasmus Villemoes <[email protected]>
> > > > > Date: Fri Jan 23 20:43:14 2015 -0800
> > > > >
> > > > > ethernet: fm10k: Actually drop 4 bits
> > > > >
> > > > > The comment explains the intention, but vid has type u16. Before the
> > > > > inner shift, it is promoted to int, which has plenty of space for all
> > > > > vid's bits, so nothing is dropped. Use a simple mask instead.
> > > > >
> > > > >
> > > > > I use the kernel from your tree in both host and guest.
> > > > >
> > > > > Assign an IPv6 for VF in guest.
> > > > > # ip -6 addr add 2001:db8::18:1/64 dev ens0
> > > > >
> > > > > Send ping packet from other server to the VM.
> > > > > # ping6 2001:db8::18:1 -I eth0
> > > > >
> > > > > The following message was shown.
> > > > > ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
> > > > >
> > > > > If I did the same operation in the host, I saw the same error message in
> > > host too.
> > > > > ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
> > > > >
> > > > > Do you have any idea about that?
> > > >
> > > > Ah, sorry about that, try this tree again:
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
> > > >
> > > > That patch was dropped for favor of a patch that Matthew Vick put
> > > > together (and recently got pushed upstream). So my queue no longer
> > > > has that patch in the queue, since it got dropped.
> > >
> > > I still see the same error, the head id is the below
> > >
> > > $ git log | head
> > > commit a072afb0b45904022b76deef3b770ee9a93cb13a
> > > Author: Nicholas Krause <[email protected]>
> > > Date: Mon Feb 9 00:27:00 2015 -0800
> > >
> > > igb: Remove outdated fix me comment in the
> > > function,gb_acquire_swfw_sync_i210
> > >
> > >
> > > thanks,
> > > Hiroshi
> >
> > I'm having our validation see if they can recreate the same issue internally. When they get back to me I'll let you
> know
> > what we found.
>
> We did bisect, and the below looks the culprit;
>
> 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
> commit 32dce968dd987adfb0c00946d78dad9154f64759
> Author: Vlad Yasevich <[email protected]>
> Date: Sat Jan 31 10:40:18 2015 -0500
>
> ipv6: Allow for partial checksums on non-ufo packets
>
> Currntly, if we are not doing UFO on the packet, all UDP
> packets will start with CHECKSUM_NONE and thus perform full
> checksum computations in software even if device support
> IPv6 checksum offloading.
>
> Let's start start with CHECKSUM_PARTIAL if the device
> supports it and we are sending only a single packet at
> or below mtu size.
>
> Signed-off-by: Vladislav Yasevich <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
>
> :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d fade8da998d35c8da97a15f0556949ad371e5347 M net

When I reverted the commit, the issue was solved.

thanks,
Hiroshi

????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?


2015-02-13 17:26:13

by Skidmore, Donald C

[permalink] [raw]
Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode



> -----Original Message-----
> From: Hiroshi Shimamoto [mailto:[email protected]]
> Sent: Thursday, February 12, 2015 8:45 PM
> To: Skidmore, Donald C; Kirsher, Jeffrey T
> Cc: Alexander Duyck; Bjørn Mork; [email protected];
> [email protected]; Choi, Sy Jong; [email protected]; David
> Laight; Hayato Momma
> Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to
> enable MC promiscuous mode
>
> > > > -----Original Message-----
> > > > From: Hiroshi Shimamoto [mailto:[email protected]]
> > > > Sent: Monday, February 09, 2015 6:29 PM
> > > > To: Kirsher, Jeffrey T
> > > > Cc: Alexander Duyck; Skidmore, Donald C; Bjørn Mork; e1000-
> > > > [email protected]; [email protected]; Choi, Sy
> > > > Jong; linux- [email protected]; David Laight; Hayato Momma
> > > > Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new
> > > > mbox API to enable MC promiscuous mode
> > > >
> > > > > > > > Can you please fix up your patches based on my tree:
> > > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/que
> > > > > > > > ue.git
> > > > > > >
> > > > > > > Yes. I haven't noticed your tree.
> > > > > > > Will resend patches against it.
> > > > > > >
> > > > > >
> > > > > > I encountered an issue with your tree, the commit id is below.
> > > > > >
> > > > > > $ git log | head
> > > > > > commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
> > > > > > Author: Rasmus Villemoes <[email protected]>
> > > > > > Date: Fri Jan 23 20:43:14 2015 -0800
> > > > > >
> > > > > > ethernet: fm10k: Actually drop 4 bits
> > > > > >
> > > > > > The comment explains the intention, but vid has type u16. Before
> the
> > > > > > inner shift, it is promoted to int, which has plenty of space for all
> > > > > > vid's bits, so nothing is dropped. Use a simple mask instead.
> > > > > >
> > > > > >
> > > > > > I use the kernel from your tree in both host and guest.
> > > > > >
> > > > > > Assign an IPv6 for VF in guest.
> > > > > > # ip -6 addr add 2001:db8::18:1/64 dev ens0
> > > > > >
> > > > > > Send ping packet from other server to the VM.
> > > > > > # ping6 2001:db8::18:1 -I eth0
> > > > > >
> > > > > > The following message was shown.
> > > > > > ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
> > > > > >
> > > > > > If I did the same operation in the host, I saw the same error
> > > > > > message in
> > > > host too.
> > > > > > ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
> > > > > >
> > > > > > Do you have any idea about that?
> > > > >
> > > > > Ah, sorry about that, try this tree again:
> > > > > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
> > > > >
> > > > > That patch was dropped for favor of a patch that Matthew Vick
> > > > > put together (and recently got pushed upstream). So my queue no
> > > > > longer has that patch in the queue, since it got dropped.
> > > >
> > > > I still see the same error, the head id is the below
> > > >
> > > > $ git log | head
> > > > commit a072afb0b45904022b76deef3b770ee9a93cb13a
> > > > Author: Nicholas Krause <[email protected]>
> > > > Date: Mon Feb 9 00:27:00 2015 -0800
> > > >
> > > > igb: Remove outdated fix me comment in the
> > > > function,gb_acquire_swfw_sync_i210
> > > >
> > > >
> > > > thanks,
> > > > Hiroshi
> > >
> > > I'm having our validation see if they can recreate the same issue
> > > internally. When they get back to me I'll let you
> > know
> > > what we found.
> >
> > We did bisect, and the below looks the culprit;
> >
> > 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
> > commit 32dce968dd987adfb0c00946d78dad9154f64759
> > Author: Vlad Yasevich <[email protected]>
> > Date: Sat Jan 31 10:40:18 2015 -0500
> >
> > ipv6: Allow for partial checksums on non-ufo packets
> >
> > Currntly, if we are not doing UFO on the packet, all UDP
> > packets will start with CHECKSUM_NONE and thus perform full
> > checksum computations in software even if device support
> > IPv6 checksum offloading.
> >
> > Let's start start with CHECKSUM_PARTIAL if the device
> > supports it and we are sending only a single packet at
> > or below mtu size.
> >
> > Signed-off-by: Vladislav Yasevich <[email protected]>
> > Signed-off-by: David S. Miller <[email protected]>
> >
> > :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d
> fade8da998d35c8da97a15f0556949ad371e5347 M net
>
> When I reverted the commit, the issue was solved.
>
> thanks,
> Hiroshi

I believe the issue is that this patch (32dce968dd98 - ipv6: Allow for partial checksums on non-ufo packets) is that it now sets CHECKSUM_PARTIAL on all IPv6 packets including ICMPv6 ones. Our HW (82599) only supports checksum offload on TCP/UDP (NETIF_F_IPV6_CSUM) so we get hung up on the skb's protocol and the fact that it is CHECKSUM_PARTIAL.

Another thing that confuses me is the feature test in this patch. It checks (rt->dst.dev->features & NETIF_F_V6_CSUM) but NETIF_F_V6_CSUM is a two bit field?

#define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)

So the test would succeed if either bit was high, that doesn't seem right. I cc'd the author so maybe he could clue us in.

Thanks,
-Don Skidmore <[email protected]>


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-02-13 17:34:47

by Vlad Yasevich

[permalink] [raw]
Subject: Re: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

On 02/13/2015 12:26 PM, Skidmore, Donald C wrote:
>
>
>> -----Original Message-----
>> From: Hiroshi Shimamoto [mailto:[email protected]]
>> Sent: Thursday, February 12, 2015 8:45 PM
>> To: Skidmore, Donald C; Kirsher, Jeffrey T
>> Cc: Alexander Duyck; Bjørn Mork; [email protected];
>> [email protected]; Choi, Sy Jong; [email protected]; David
>> Laight; Hayato Momma
>> Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to
>> enable MC promiscuous mode
>>
>>>>> -----Original Message-----
>>>>> From: Hiroshi Shimamoto [mailto:[email protected]]
>>>>> Sent: Monday, February 09, 2015 6:29 PM
>>>>> To: Kirsher, Jeffrey T
>>>>> Cc: Alexander Duyck; Skidmore, Donald C; Bjørn Mork; e1000-
>>>>> [email protected]; [email protected]; Choi, Sy
>>>>> Jong; linux- [email protected]; David Laight; Hayato Momma
>>>>> Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new
>>>>> mbox API to enable MC promiscuous mode
>>>>>
>>>>>>>>> Can you please fix up your patches based on my tree:
>>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/que
>>>>>>>>> ue.git
>>>>>>>>
>>>>>>>> Yes. I haven't noticed your tree.
>>>>>>>> Will resend patches against it.
>>>>>>>>
>>>>>>>
>>>>>>> I encountered an issue with your tree, the commit id is below.
>>>>>>>
>>>>>>> $ git log | head
>>>>>>> commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
>>>>>>> Author: Rasmus Villemoes <[email protected]>
>>>>>>> Date: Fri Jan 23 20:43:14 2015 -0800
>>>>>>>
>>>>>>> ethernet: fm10k: Actually drop 4 bits
>>>>>>>
>>>>>>> The comment explains the intention, but vid has type u16. Before
>> the
>>>>>>> inner shift, it is promoted to int, which has plenty of space for all
>>>>>>> vid's bits, so nothing is dropped. Use a simple mask instead.
>>>>>>>
>>>>>>>
>>>>>>> I use the kernel from your tree in both host and guest.
>>>>>>>
>>>>>>> Assign an IPv6 for VF in guest.
>>>>>>> # ip -6 addr add 2001:db8::18:1/64 dev ens0
>>>>>>>
>>>>>>> Send ping packet from other server to the VM.
>>>>>>> # ping6 2001:db8::18:1 -I eth0
>>>>>>>
>>>>>>> The following message was shown.
>>>>>>> ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
>>>>>>>
>>>>>>> If I did the same operation in the host, I saw the same error
>>>>>>> message in
>>>>> host too.
>>>>>>> ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
>>>>>>>
>>>>>>> Do you have any idea about that?
>>>>>>
>>>>>> Ah, sorry about that, try this tree again:
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
>>>>>>
>>>>>> That patch was dropped for favor of a patch that Matthew Vick
>>>>>> put together (and recently got pushed upstream). So my queue no
>>>>>> longer has that patch in the queue, since it got dropped.
>>>>>
>>>>> I still see the same error, the head id is the below
>>>>>
>>>>> $ git log | head
>>>>> commit a072afb0b45904022b76deef3b770ee9a93cb13a
>>>>> Author: Nicholas Krause <[email protected]>
>>>>> Date: Mon Feb 9 00:27:00 2015 -0800
>>>>>
>>>>> igb: Remove outdated fix me comment in the
>>>>> function,gb_acquire_swfw_sync_i210
>>>>>
>>>>>
>>>>> thanks,
>>>>> Hiroshi
>>>>
>>>> I'm having our validation see if they can recreate the same issue
>>>> internally. When they get back to me I'll let you
>>> know
>>>> what we found.
>>>
>>> We did bisect, and the below looks the culprit;
>>>
>>> 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
>>> commit 32dce968dd987adfb0c00946d78dad9154f64759
>>> Author: Vlad Yasevich <[email protected]>
>>> Date: Sat Jan 31 10:40:18 2015 -0500
>>>
>>> ipv6: Allow for partial checksums on non-ufo packets
>>>
>>> Currntly, if we are not doing UFO on the packet, all UDP
>>> packets will start with CHECKSUM_NONE and thus perform full
>>> checksum computations in software even if device support
>>> IPv6 checksum offloading.
>>>
>>> Let's start start with CHECKSUM_PARTIAL if the device
>>> supports it and we are sending only a single packet at
>>> or below mtu size.
>>>
>>> Signed-off-by: Vladislav Yasevich <[email protected]>
>>> Signed-off-by: David S. Miller <[email protected]>
>>>
>>> :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d
>> fade8da998d35c8da97a15f0556949ad371e5347 M net
>>
>> When I reverted the commit, the issue was solved.
>>
>> thanks,
>> Hiroshi
>
> I believe the issue is that this patch (32dce968dd98 - ipv6: Allow for partial checksums on non-ufo packets) is that it now sets CHECKSUM_PARTIAL on all IPv6 packets including ICMPv6 ones. Our HW (82599) only supports checksum offload on TCP/UDP (NETIF_F_IPV6_CSUM) so we get hung up on the skb's protocol and the fact that it is CHECKSUM_PARTIAL.
>
> Another thing that confuses me is the feature test in this patch. It checks (rt->dst.dev->features & NETIF_F_V6_CSUM) but NETIF_F_V6_CSUM is a two bit field?
>
> #define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
>
> So the test would succeed if either bit was high, that doesn't seem right. I cc'd the author so maybe he could clue us in.

This has been addressed by:
commit bf250a1fa769f2eb8fc7a4e28b3b523e9cb67eef
Author: Vlad Yasevich <[email protected]>
Date: Tue Feb 10 11:37:29 2015 -0500

ipv6: Partial checksum only UDP packets


As far the 2 bit issue, GEN_CSUM (HW_SUM) and IPV6_CSUM can not coexist at the same time.
See netdev_fix_features().

-vlad

>
> Thanks,
> -Don Skidmore <[email protected]>
>
>

2015-02-16 05:48:35

by Hiroshi Shimamoto

[permalink] [raw]
Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

> >>>>>>>>> Can you please fix up your patches based on my tree:
> >>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/que
> >>>>>>>>> ue.git
> >>>>>>>>
> >>>>>>>> Yes. I haven't noticed your tree.
> >>>>>>>> Will resend patches against it.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I encountered an issue with your tree, the commit id is below.
> >>>>>>>
> >>>>>>> $ git log | head
> >>>>>>> commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
> >>>>>>> Author: Rasmus Villemoes <[email protected]>
> >>>>>>> Date: Fri Jan 23 20:43:14 2015 -0800
> >>>>>>>
> >>>>>>> ethernet: fm10k: Actually drop 4 bits
> >>>>>>>
> >>>>>>> The comment explains the intention, but vid has type u16. Before
> >> the
> >>>>>>> inner shift, it is promoted to int, which has plenty of space for all
> >>>>>>> vid's bits, so nothing is dropped. Use a simple mask instead.
> >>>>>>>
> >>>>>>>
> >>>>>>> I use the kernel from your tree in both host and guest.
> >>>>>>>
> >>>>>>> Assign an IPv6 for VF in guest.
> >>>>>>> # ip -6 addr add 2001:db8::18:1/64 dev ens0
> >>>>>>>
> >>>>>>> Send ping packet from other server to the VM.
> >>>>>>> # ping6 2001:db8::18:1 -I eth0
> >>>>>>>
> >>>>>>> The following message was shown.
> >>>>>>> ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
> >>>>>>>
> >>>>>>> If I did the same operation in the host, I saw the same error
> >>>>>>> message in
> >>>>> host too.
> >>>>>>> ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
> >>>>>>>
> >>>>>>> Do you have any idea about that?
> >>>>>>
> >>>>>> Ah, sorry about that, try this tree again:
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
> >>>>>>
> >>>>>> That patch was dropped for favor of a patch that Matthew Vick
> >>>>>> put together (and recently got pushed upstream). So my queue no
> >>>>>> longer has that patch in the queue, since it got dropped.
> >>>>>
> >>>>> I still see the same error, the head id is the below
> >>>>>
> >>>>> $ git log | head
> >>>>> commit a072afb0b45904022b76deef3b770ee9a93cb13a
> >>>>> Author: Nicholas Krause <[email protected]>
> >>>>> Date: Mon Feb 9 00:27:00 2015 -0800
> >>>>>
> >>>>> igb: Remove outdated fix me comment in the
> >>>>> function,gb_acquire_swfw_sync_i210
> >>>>>
> >>>>>
> >>>>> thanks,
> >>>>> Hiroshi
> >>>>
> >>>> I'm having our validation see if they can recreate the same issue
> >>>> internally. When they get back to me I'll let you
> >>> know
> >>>> what we found.
> >>>
> >>> We did bisect, and the below looks the culprit;
> >>>
> >>> 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
> >>> commit 32dce968dd987adfb0c00946d78dad9154f64759
> >>> Author: Vlad Yasevich <[email protected]>
> >>> Date: Sat Jan 31 10:40:18 2015 -0500
> >>>
> >>> ipv6: Allow for partial checksums on non-ufo packets
> >>>
> >>> Currntly, if we are not doing UFO on the packet, all UDP
> >>> packets will start with CHECKSUM_NONE and thus perform full
> >>> checksum computations in software even if device support
> >>> IPv6 checksum offloading.
> >>>
> >>> Let's start start with CHECKSUM_PARTIAL if the device
> >>> supports it and we are sending only a single packet at
> >>> or below mtu size.
> >>>
> >>> Signed-off-by: Vladislav Yasevich <[email protected]>
> >>> Signed-off-by: David S. Miller <[email protected]>
> >>>
> >>> :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d
> >> fade8da998d35c8da97a15f0556949ad371e5347 M net
> >>
> >> When I reverted the commit, the issue was solved.
> >>
> >> thanks,
> >> Hiroshi
> >
> > I believe the issue is that this patch (32dce968dd98 - ipv6: Allow for partial checksums on non-ufo packets) is that
> it now sets CHECKSUM_PARTIAL on all IPv6 packets including ICMPv6 ones. Our HW (82599) only supports checksum offload
> on TCP/UDP (NETIF_F_IPV6_CSUM) so we get hung up on the skb's protocol and the fact that it is CHECKSUM_PARTIAL.
> >
> > Another thing that confuses me is the feature test in this patch. It checks (rt->dst.dev->features & NETIF_F_V6_CSUM)
> but NETIF_F_V6_CSUM is a two bit field?
> >
> > #define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> >
> > So the test would succeed if either bit was high, that doesn't seem right. I cc'd the author so maybe he could clue
> us in.
>
> This has been addressed by:
> commit bf250a1fa769f2eb8fc7a4e28b3b523e9cb67eef
> Author: Vlad Yasevich <[email protected]>
> Date: Tue Feb 10 11:37:29 2015 -0500
>
> ipv6: Partial checksum only UDP packets
>
>
> As far the 2 bit issue, GEN_CSUM (HW_SUM) and IPV6_CSUM can not coexist at the same time.
> See netdev_fix_features().
>

thanks for pointing it. I will test with that commit.

Jeff's tree hasn't included that commit yet, right?
Which branch has the commit?

thanks,
Hiroshi
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2015-02-17 15:03:26

by Vlad Yasevich

[permalink] [raw]
Subject: Re: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

On 02/15/2015 11:54 PM, Hiroshi Shimamoto wrote:
>>>>>>>>>>> Can you please fix up your patches based on my tree:
>>>>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/que
>>>>>>>>>>> ue.git
>>>>>>>>>>
>>>>>>>>>> Yes. I haven't noticed your tree.
>>>>>>>>>> Will resend patches against it.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I encountered an issue with your tree, the commit id is below.
>>>>>>>>>
>>>>>>>>> $ git log | head
>>>>>>>>> commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
>>>>>>>>> Author: Rasmus Villemoes <[email protected]>
>>>>>>>>> Date: Fri Jan 23 20:43:14 2015 -0800
>>>>>>>>>
>>>>>>>>> ethernet: fm10k: Actually drop 4 bits
>>>>>>>>>
>>>>>>>>> The comment explains the intention, but vid has type u16. Before
>>>> the
>>>>>>>>> inner shift, it is promoted to int, which has plenty of space for all
>>>>>>>>> vid's bits, so nothing is dropped. Use a simple mask instead.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I use the kernel from your tree in both host and guest.
>>>>>>>>>
>>>>>>>>> Assign an IPv6 for VF in guest.
>>>>>>>>> # ip -6 addr add 2001:db8::18:1/64 dev ens0
>>>>>>>>>
>>>>>>>>> Send ping packet from other server to the VM.
>>>>>>>>> # ping6 2001:db8::18:1 -I eth0
>>>>>>>>>
>>>>>>>>> The following message was shown.
>>>>>>>>> ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
>>>>>>>>>
>>>>>>>>> If I did the same operation in the host, I saw the same error
>>>>>>>>> message in
>>>>>>> host too.
>>>>>>>>> ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
>>>>>>>>>
>>>>>>>>> Do you have any idea about that?
>>>>>>>>
>>>>>>>> Ah, sorry about that, try this tree again:
>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.git
>>>>>>>>
>>>>>>>> That patch was dropped for favor of a patch that Matthew Vick
>>>>>>>> put together (and recently got pushed upstream). So my queue no
>>>>>>>> longer has that patch in the queue, since it got dropped.
>>>>>>>
>>>>>>> I still see the same error, the head id is the below
>>>>>>>
>>>>>>> $ git log | head
>>>>>>> commit a072afb0b45904022b76deef3b770ee9a93cb13a
>>>>>>> Author: Nicholas Krause <[email protected]>
>>>>>>> Date: Mon Feb 9 00:27:00 2015 -0800
>>>>>>>
>>>>>>> igb: Remove outdated fix me comment in the
>>>>>>> function,gb_acquire_swfw_sync_i210
>>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> Hiroshi
>>>>>>
>>>>>> I'm having our validation see if they can recreate the same issue
>>>>>> internally. When they get back to me I'll let you
>>>>> know
>>>>>> what we found.
>>>>>
>>>>> We did bisect, and the below looks the culprit;
>>>>>
>>>>> 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
>>>>> commit 32dce968dd987adfb0c00946d78dad9154f64759
>>>>> Author: Vlad Yasevich <[email protected]>
>>>>> Date: Sat Jan 31 10:40:18 2015 -0500
>>>>>
>>>>> ipv6: Allow for partial checksums on non-ufo packets
>>>>>
>>>>> Currntly, if we are not doing UFO on the packet, all UDP
>>>>> packets will start with CHECKSUM_NONE and thus perform full
>>>>> checksum computations in software even if device support
>>>>> IPv6 checksum offloading.
>>>>>
>>>>> Let's start start with CHECKSUM_PARTIAL if the device
>>>>> supports it and we are sending only a single packet at
>>>>> or below mtu size.
>>>>>
>>>>> Signed-off-by: Vladislav Yasevich <[email protected]>
>>>>> Signed-off-by: David S. Miller <[email protected]>
>>>>>
>>>>> :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d
>>>> fade8da998d35c8da97a15f0556949ad371e5347 M net
>>>>
>>>> When I reverted the commit, the issue was solved.
>>>>
>>>> thanks,
>>>> Hiroshi
>>>
>>> I believe the issue is that this patch (32dce968dd98 - ipv6: Allow for partial checksums on non-ufo packets) is that
>> it now sets CHECKSUM_PARTIAL on all IPv6 packets including ICMPv6 ones. Our HW (82599) only supports checksum offload
>> on TCP/UDP (NETIF_F_IPV6_CSUM) so we get hung up on the skb's protocol and the fact that it is CHECKSUM_PARTIAL.
>>>
>>> Another thing that confuses me is the feature test in this patch. It checks (rt->dst.dev->features & NETIF_F_V6_CSUM)
>> but NETIF_F_V6_CSUM is a two bit field?
>>>
>>> #define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
>>>
>>> So the test would succeed if either bit was high, that doesn't seem right. I cc'd the author so maybe he could clue
>> us in.
>>
>> This has been addressed by:
>> commit bf250a1fa769f2eb8fc7a4e28b3b523e9cb67eef
>> Author: Vlad Yasevich <[email protected]>
>> Date: Tue Feb 10 11:37:29 2015 -0500
>>
>> ipv6: Partial checksum only UDP packets
>>
>>
>> As far the 2 bit issue, GEN_CSUM (HW_SUM) and IPV6_CSUM can not coexist at the same time.
>> See netdev_fix_features().
>>
>
> thanks for pointing it. I will test with that commit.
>
> Jeff's tree hasn't included that commit yet, right?
> Which branch has the commit?

This is in DaveM's net and net-next trees.

-vlad

>
> thanks,
> Hiroshi
>

2015-02-17 23:31:33

by Skidmore, Donald C

[permalink] [raw]
Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode



-----Original Message-----
From: Hiroshi Shimamoto [mailto:[email protected]]
Sent: Sunday, February 15, 2015 8:54 PM
To: [email protected]; Skidmore, Donald C; Kirsher, Jeffrey T
Cc: Alexander Duyck; Bjørn Mork; [email protected]; [email protected]; Choi, Sy Jong; [email protected]; David Laight; Hayato Momma
Subject: RE: [E1000-devel] [PATCH 1/3] ixgbe, ixgbevf: Add new mbox API to enable MC promiscuous mode

> >>>>>>>>> Can you please fix up your patches based on my tree:
> >>>>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/que
> >>>>>>>>> ue.git
> >>>>>>>>
> >>>>>>>> Yes. I haven't noticed your tree.
> >>>>>>>> Will resend patches against it.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I encountered an issue with your tree, the commit id is below.
> >>>>>>>
> >>>>>>> $ git log | head
> >>>>>>> commit e6f1649780f8f5a87299bf6af04453f93d1e3d5e
> >>>>>>> Author: Rasmus Villemoes <[email protected]>
> >>>>>>> Date: Fri Jan 23 20:43:14 2015 -0800
> >>>>>>>
> >>>>>>> ethernet: fm10k: Actually drop 4 bits
> >>>>>>>
> >>>>>>> The comment explains the intention, but vid has type u16.
> >>>>>>> Before
> >> the
> >>>>>>> inner shift, it is promoted to int, which has plenty of space for all
> >>>>>>> vid's bits, so nothing is dropped. Use a simple mask instead.
> >>>>>>>
> >>>>>>>
> >>>>>>> I use the kernel from your tree in both host and guest.
> >>>>>>>
> >>>>>>> Assign an IPv6 for VF in guest.
> >>>>>>> # ip -6 addr add 2001:db8::18:1/64 dev ens0
> >>>>>>>
> >>>>>>> Send ping packet from other server to the VM.
> >>>>>>> # ping6 2001:db8::18:1 -I eth0
> >>>>>>>
> >>>>>>> The following message was shown.
> >>>>>>> ixgbevf 0000:00:08.0: partial checksum but l4 proto=3a!
> >>>>>>>
> >>>>>>> If I did the same operation in the host, I saw the same error
> >>>>>>> message in
> >>>>> host too.
> >>>>>>> ixgbe 0000:2d:00.0: partial checksum but l4 proto=3a!
> >>>>>>>
> >>>>>>> Do you have any idea about that?
> >>>>>>
> >>>>>> Ah, sorry about that, try this tree again:
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/queue.gi
> >>>>>> t
> >>>>>>
> >>>>>> That patch was dropped for favor of a patch that Matthew Vick
> >>>>>> put together (and recently got pushed upstream). So my queue
> >>>>>> no longer has that patch in the queue, since it got dropped.
> >>>>>
> >>>>> I still see the same error, the head id is the below
> >>>>>
> >>>>> $ git log | head
> >>>>> commit a072afb0b45904022b76deef3b770ee9a93cb13a
> >>>>> Author: Nicholas Krause <[email protected]>
> >>>>> Date: Mon Feb 9 00:27:00 2015 -0800
> >>>>>
> >>>>> igb: Remove outdated fix me comment in the
> >>>>> function,gb_acquire_swfw_sync_i210
> >>>>>
> >>>>>
> >>>>> thanks,
> >>>>> Hiroshi
> >>>>
> >>>> I'm having our validation see if they can recreate the same issue
> >>>> internally. When they get back to me I'll let you
> >>> know
> >>>> what we found.
> >>>
> >>> We did bisect, and the below looks the culprit;
> >>>
> >>> 32dce968dd987adfb0c00946d78dad9154f64759 is the first bad commit
> >>> commit 32dce968dd987adfb0c00946d78dad9154f64759
> >>> Author: Vlad Yasevich <[email protected]>
> >>> Date: Sat Jan 31 10:40:18 2015 -0500
> >>>
> >>> ipv6: Allow for partial checksums on non-ufo packets
> >>>
> >>> Currntly, if we are not doing UFO on the packet, all UDP
> >>> packets will start with CHECKSUM_NONE and thus perform full
> >>> checksum computations in software even if device support
> >>> IPv6 checksum offloading.
> >>>
> >>> Let's start start with CHECKSUM_PARTIAL if the device
> >>> supports it and we are sending only a single packet at
> >>> or below mtu size.
> >>>
> >>> Signed-off-by: Vladislav Yasevich <[email protected]>
> >>> Signed-off-by: David S. Miller <[email protected]>
> >>>
> >>> :040000 040000 4437eaf7e944f5a6136ebf668a256fee688fda3d
> >> fade8da998d35c8da97a15f0556949ad371e5347 M net
> >>
> >> When I reverted the commit, the issue was solved.
> >>
> >> thanks,
> >> Hiroshi
> >
> > I believe the issue is that this patch (32dce968dd98 - ipv6: Allow
> > for partial checksums on non-ufo packets) is that
> it now sets CHECKSUM_PARTIAL on all IPv6 packets including ICMPv6
> ones. Our HW (82599) only supports checksum offload on TCP/UDP (NETIF_F_IPV6_CSUM) so we get hung up on the skb's protocol and the fact that it is CHECKSUM_PARTIAL.
> >
> > Another thing that confuses me is the feature test in this patch.
> > It checks (rt->dst.dev->features & NETIF_F_V6_CSUM)
> but NETIF_F_V6_CSUM is a two bit field?
> >
> > #define NETIF_F_V6_CSUM (NETIF_F_GEN_CSUM | NETIF_F_IPV6_CSUM)
> >
> > So the test would succeed if either bit was high, that doesn't seem
> > right. I cc'd the author so maybe he could clue
> us in.
>
> This has been addressed by:
> commit bf250a1fa769f2eb8fc7a4e28b3b523e9cb67eef
> Author: Vlad Yasevich <[email protected]>
> Date: Tue Feb 10 11:37:29 2015 -0500
>
> ipv6: Partial checksum only UDP packets
>
>
> As far the 2 bit issue, GEN_CSUM (HW_SUM) and IPV6_CSUM can not coexist at the same time.
> See netdev_fix_features().
>

thanks for pointing it. I will test with that commit.

Jeff's tree hasn't included that commit yet, right?
Which branch has the commit?

thanks,
Hiroshi

Jeff should have his tree updated pretty soon after the patch is merged into net-next. It would probably be best to wait till that happens and use Jeff's tree.

Thanks,
-Don Skidmore <[email protected]>


????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?