| Message ID | 20220323104329.279559-1-abhiramrn@gmail.com |
|---|---|
| State | Accepted |
| Commit | 7539b4e45a9932844ceb111724b9042b4eaef4f4 |
| Headers | show |
| Series | [ovs-dev,v2] netdev-vport: Register IFINDEX for ERSPAN device | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 23 Mar 2022, at 11:43, Abhiram R N wrote: > When enabling offload for ERSPAN we are seeing one error as below > > netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: > Operation not supported > netdev_offload|INFO|erspan0: No suitable flow API found. > > Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this > error The change looks good to me, but there is no real unit test to verify this, and I have no real setup to test this. So based on a pure visible and compile only check: Acked-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Abhiram R N <abhiramrn@gmail.com> > --- PS: Next time you send out an updated revision, please add some comments after the --- above. For example something like: v2: - Removed terminating comma from final assignments. > lib/netdev-vport.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 64331f74b..3b3927865 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -1245,7 +1245,8 @@ netdev_vport_tunnel_register(void) > .type = "erspan", > .build_header = netdev_erspan_build_header, > .push_header = netdev_erspan_push_header, > - .pop_header = netdev_erspan_pop_header > + .pop_header = netdev_erspan_pop_header, > + .get_ifindex = NETDEV_VPORT_GET_IFINDEX > }, > {{NULL, NULL, 0, 0}} > }, > @@ -1255,7 +1256,8 @@ netdev_vport_tunnel_register(void) > .type = "ip6erspan", > .build_header = netdev_erspan_build_header, > .push_header = netdev_erspan_push_header, > - .pop_header = netdev_erspan_pop_header > + .pop_header = netdev_erspan_pop_header, > + .get_ifindex = NETDEV_VPORT_GET_IFINDEX > }, > {{NULL, NULL, 0, 0}} > }, > -- > 2.27.0
On 3/23/22 14:01, Eelco Chaudron wrote: > > > On 23 Mar 2022, at 11:43, Abhiram R N wrote: > >> When enabling offload for ERSPAN we are seeing one error as below >> >> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: >> Operation not supported >> netdev_offload|INFO|erspan0: No suitable flow API found. >> >> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this >> error > > The change looks good to me, but there is no real unit test to verify this, and I have no real setup to test this. > > So based on a pure visible and compile only check: > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> Signed-off-by: Abhiram R N <abhiramrn@gmail.com> >> --- I don't see why this can cause any problems or why this callback wasn't set from the beginning, so applied. Thanks! Best regards, Ilya Maximets.
On 5 Apr 2022, at 0:48, Ilya Maximets wrote: > On 3/23/22 14:01, Eelco Chaudron wrote: >> >> >> On 23 Mar 2022, at 11:43, Abhiram R N wrote: >> >>> When enabling offload for ERSPAN we are seeing one error as below >>> >>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: >>> Operation not supported >>> netdev_offload|INFO|erspan0: No suitable flow API found. >>> >>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this >>> error >> >> The change looks good to me, but there is no real unit test to verify this, and I have no real setup to test this. >> >> So based on a pure visible and compile only check: >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> >>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com> >>> --- > > I don't see why this can cause any problems or why this callback > wasn't set from the beginning, so applied. Thanks! > > Best regards, Ilya Maximets. Hi Abhiram/Ilya, Guess it takes a while but basic ping seems to be failing when offloading to TC with t his change. If you enable TC offload the following tests are failing on latest master: datapath - ping over erspan v1 tunnel FAILED (system-traffic.at:467) datapath - ping over erspan v2 tunnel FAILED (system-traffic.at:504) datapath - ping over ip6erspan v1 tunnel FAILED (system-traffic.at:544) datapath - ping over ip6erspan v2 tunnel FAILED (system-traffic.at:585) For example the following diff causes the first test to fail with tc: diff --git a/tests/system-traffic.at b/tests/system-traffic.at index e16ea6027..fa6b84b4e 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -440,6 +440,9 @@ OVS_CHECK_GRE() OVS_CHECK_ERSPAN() OVS_TRAFFIC_VSWITCHD_START() + +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true + ADD_BR([br-underlay]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) I think it’s because we are not adding any of the erspan fields to the offload tunnel encapsulated. Abhiram, wondering if this was really working in your case, as it seems only the first packet is passed on. I guess we might need to undo this change, or you need to come up with a fix for this. Cheers, Eelco
Hi Eelco, I do remember testing with this. I don't have full logs as such. But from one of the mail I had sent out I found below logs (this is with the change of course) where it's offloaded to TC (PSB). [ Because there seems to be support needed in HW driver (which is not present) and hence flows are not offloaded to HW which is expected]. But as you can see flows are in *dp:tc*.. (I can see 39 packets... so it's not only the first one). Not sure if anything got changed post that which might have caused this to fail now. [root@rhos-nfv-09 ~]# ovs-appctl dpctl/dump-flows -m ufid:edb7d581-9f14-42df-9e50-69a4923e1dbd, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_0),packet_type(ns=0/0,id=0/0),eth(src=6a:62:68:b2:61:f6,dst=66:f0:e4:1e:2c:bb),eth_type(0x0800),ipv4(src= 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys,enp4s0f0_1 ufid:519d8cfd-9821-467f-9f61-e2bf61cdde60, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_1),packet_type(ns=0/0,id=0/0),eth(src=66:f0:e4:1e:2c:bb,dst=6a:62:68:b2:61:f6),eth_type(0x0800),ipv4(src= 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:enp4s0f0_0,set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys [root@rhos-nfv-09 ~]# JFYI, Without this change still flows won't get offloaded to TC. Below is the error what I used to get and the flows used to be in *dp:ovs* 2022-02-25T14:33:04.134Z|00151|netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: Operation not supported 2022-02-25T14:33:04.134Z|00152|netdev_offload|INFO|erspan0: No suitable flow API found. Thanks & Regards, Abhiram R N On Mon, Jul 4, 2022 at 7:48 PM Eelco Chaudron <echaudro@redhat.com> wrote: > > > On 5 Apr 2022, at 0:48, Ilya Maximets wrote: > > > On 3/23/22 14:01, Eelco Chaudron wrote: > >> > >> > >> On 23 Mar 2022, at 11:43, Abhiram R N wrote: > >> > >>> When enabling offload for ERSPAN we are seeing one error as below > >>> > >>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: > >>> Operation not supported > >>> netdev_offload|INFO|erspan0: No suitable flow API found. > >>> > >>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this > >>> error > >> > >> The change looks good to me, but there is no real unit test to verify > this, and I have no real setup to test this. > >> > >> So based on a pure visible and compile only check: > >> > >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > >> > >>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com> > >>> --- > > > > I don't see why this can cause any problems or why this callback > > wasn't set from the beginning, so applied. Thanks! > > > > Best regards, Ilya Maximets. > > Hi Abhiram/Ilya, > > Guess it takes a while but basic ping seems to be failing when offloading > to TC with t his change. > > If you enable TC offload the following tests are failing on latest master: > > datapath - ping over erspan v1 tunnel FAILED ( > system-traffic.at:467) > datapath - ping over erspan v2 tunnel FAILED ( > system-traffic.at:504) > datapath - ping over ip6erspan v1 tunnel FAILED ( > system-traffic.at:544) > datapath - ping over ip6erspan v2 tunnel FAILED ( > system-traffic.at:585) > > > For example the following diff causes the first test to fail with tc: > > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index e16ea6027..fa6b84b4e 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -440,6 +440,9 @@ OVS_CHECK_GRE() > OVS_CHECK_ERSPAN() > > OVS_TRAFFIC_VSWITCHD_START() > + > +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true > + > ADD_BR([br-underlay]) > > AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > > > I think it’s because we are not adding any of the erspan fields to the > offload tunnel encapsulated. > > > Abhiram, wondering if this was really working in your case, as it seems > only the first packet is passed on. > > I guess we might need to undo this change, or you need to come up with a > fix for this. > > > Cheers, > > Eelco > >
Hi Abhiram, It’s been there since your patch was committed to the repo. I did a quick verification again, and it is: git checkout 7539b4e45 <edit test> git diff diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 4a7fa49fc..b55d757bf 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -393,6 +393,9 @@ OVS_CHECK_GRE() OVS_CHECK_ERSPAN() OVS_TRAFFIC_VSWITCHD_START() + +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true + ADD_BR([br-underlay]) make -j 32 check-kernel TESTSUITEFLAGS='-k "ping over erspan v1 tunnel" -v' ... ./system-traffic.at:417: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' NS_EXEC_HEREDOC ./system-traffic.at:423: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC ping -s 1200 -i 0.3 -c 3 10.1.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' NS_EXEC_HEREDOC --- - 2022-07-04 18:48:07.788952607 +0200 +++ /home/echaudron/Documents/Scratch/ovs/tests/system-kmod-testsuite.dir/at-groups/13/stdout 2022-07-04 18:48:07.785404896 +0200 @@ -1,2 +1,2 @@ -3 packets transmitted, 3 received, 0% packet loss, time 0ms +3 packets transmitted, 1 received, 66.6667% packet loss, time 0ms ... Can you please investigate? I know reverting the patch will not offload to TC, but it’s not working, so it’s better to not offload and work than to offload and not work ;) //Eelco On 4 Jul 2022, at 17:25, Abhiram RN wrote: > Hi Eelco, > > I do remember testing with this. I don't have full logs as such. But > from > one of the mail I had sent out I found below logs (this is with the > change > of course) where it's offloaded to TC (PSB). [ Because there seems to > be > support needed in HW driver (which is not present) and hence flows are > not > offloaded to HW which is expected]. But as you can see flows are in > *dp:tc*.. > (I can see 39 packets... so it's not only the first one). Not sure if > anything got changed post that which might have caused this to fail > now. > > [root@rhos-nfv-09 ~]# ovs-appctl dpctl/dump-flows -m > ufid:edb7d581-9f14-42df-9e50-69a4923e1dbd, > skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_0),packet_type(ns=0/0,id=0/0),eth(src=6a:62:68:b2:61:f6,dst=66:f0:e4:1e:2c:bb),eth_type(0x0800),ipv4(src= > 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), > packets:39, bytes:3276, used:0.260s, *dp:tc*, > actions:set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys,enp4s0f0_1 > ufid:519d8cfd-9821-467f-9f61-e2bf61cdde60, > skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_1),packet_type(ns=0/0,id=0/0),eth(src=66:f0:e4:1e:2c:bb,dst=6a:62:68:b2:61:f6),eth_type(0x0800),ipv4(src= > 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no), > packets:39, bytes:3276, used:0.260s, *dp:tc*, > actions:enp4s0f0_0,set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys > [root@rhos-nfv-09 ~]# > > JFYI, > Without this change still flows won't get offloaded to TC. Below is > the > error what I used to get and the flows used to be in *dp:ovs* > 2022-02-25T14:33:04.134Z|00151|netdev_offload_tc|INFO|init: failed to > get > ifindex for erspan0: Operation not supported > 2022-02-25T14:33:04.134Z|00152|netdev_offload|INFO|erspan0: No > suitable > flow API found. > > Thanks & Regards, > Abhiram R N > > On Mon, Jul 4, 2022 at 7:48 PM Eelco Chaudron <echaudro@redhat.com> > wrote: > >> >> >> On 5 Apr 2022, at 0:48, Ilya Maximets wrote: >> >>> On 3/23/22 14:01, Eelco Chaudron wrote: >>>> >>>> >>>> On 23 Mar 2022, at 11:43, Abhiram R N wrote: >>>> >>>>> When enabling offload for ERSPAN we are seeing one error as below >>>>> >>>>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: >>>>> Operation not supported >>>>> netdev_offload|INFO|erspan0: No suitable flow API found. >>>>> >>>>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this >>>>> error >>>> >>>> The change looks good to me, but there is no real unit test to >>>> verify >> this, and I have no real setup to test this. >>>> >>>> So based on a pure visible and compile only check: >>>> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>> >>>>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com> >>>>> --- >>> >>> I don't see why this can cause any problems or why this callback >>> wasn't set from the beginning, so applied. Thanks! >>> >>> Best regards, Ilya Maximets. >> >> Hi Abhiram/Ilya, >> >> Guess it takes a while but basic ping seems to be failing when >> offloading >> to TC with t his change. >> >> If you enable TC offload the following tests are failing on latest >> master: >> >> datapath - ping over erspan v1 tunnel FAILED ( >> system-traffic.at:467) >> datapath - ping over erspan v2 tunnel FAILED ( >> system-traffic.at:504) >> datapath - ping over ip6erspan v1 tunnel FAILED ( >> system-traffic.at:544) >> datapath - ping over ip6erspan v2 tunnel FAILED ( >> system-traffic.at:585) >> >> >> For example the following diff causes the first test to fail with tc: >> >> >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index e16ea6027..fa6b84b4e 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -440,6 +440,9 @@ OVS_CHECK_GRE() >> OVS_CHECK_ERSPAN() >> >> OVS_TRAFFIC_VSWITCHD_START() >> + >> +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true >> + >> ADD_BR([br-underlay]) >> >> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> >> >> I think it’s because we are not adding any of the erspan fields to >> the >> offload tunnel encapsulated. >> >> >> Abhiram, wondering if this was really working in your case, as it >> seems >> only the first packet is passed on. >> >> I guess we might need to undo this change, or you need to come up >> with a >> fix for this. >> >> >> Cheers, >> >> Eelco >> >> > > -- > Always put yourself in others' shoes. If you feel that it hurts you, > it > probably hurts the other person, too. > > Regards > ABHIRAM
Hi Eelco,
I looked at it. I have a few questions.
The way I had tested was on the actual system which had Mellanox NICs and
OVS and OVN installed and with ansible scripts.
and hence I am new to these tests.
So please bear with my questions.
1. One immediate thing I saw was in my scripts I have below configs being
set.
ovs-vsctl set Open_vSwitch . *other_config:hw-offload=true*
ovs-vsctl set Open_vSwitch . *other_config:tc-policy=none*
devlink dev eswitch set pci/{{ pfdomain }}:{{ pfbus }}:{{ pfslot
}}.{{ pffunction }} mode switchdev
ethtool -K {{ physical_interface }} *hw-tc-offload *on
Just for reference -->
https://github.com/rh-nfv-int/playbooks/blob/main/regression/ovs-hwol/testRemotePortMirroring.yml#L101
In the change you shared I see you are setting hw-offload but not others.
I didn't see the tc-policy. I wanted to try it out. But facing issues as
below in 2
So, maybe you can give a shot at it with the above* tc-policy* set ?.
Other configs I am not sure how to set in these tests. Need to figure it
out.
2. For some reason the unit test gets skipped on my setup. Am I missing
something?
14. system-traffic.at:436: testing datapath - ping over erspan v1
tunnel ...
14. system-traffic.at:436: skipped (system-traffic.at:439)
3. One last thing, Since you tried enabling hw-offload for the erspan.
Is the same change in the test working for GRE? (i.e for the test
'ping over gre tunnel').
Because we haven't changed anything there now but already hw-offload
we know we should be working there.
Thanks & Regards,
Abhiram R N
On Mon, Jul 4, 2022 at 10:22 PM Eelco Chaudron <echaudro@redhat.com> wrote:
> Hi Abhiram,
>
> It’s been there since your patch was committed to the repo.
> I did a quick verification again, and it is:
>
> git checkout 7539b4e45
> <edit test>
> git diff
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 4a7fa49fc..b55d757bf 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -393,6 +393,9 @@ OVS_CHECK_GRE()
> OVS_CHECK_ERSPAN()
>
> OVS_TRAFFIC_VSWITCHD_START()
> +
> +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true
> +
> ADD_BR([br-underlay])
>
> make -j 32 check-kernel TESTSUITEFLAGS='-k "ping over erspan v1 tunnel"
> -v'
> ...
> ./system-traffic.at:417: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC
> ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | grep "transmitted" | sed
> 's/time.*ms$/time 0ms/'
> NS_EXEC_HEREDOC
> ./system-traffic.at:423: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC
> ping -s 1200 -i 0.3 -c 3 10.1.1.100 | grep "transmitted" | sed
> 's/time.*ms$/time 0ms/'
> NS_EXEC_HEREDOC
> --- - 2022-07-04 18:48:07.788952607 +0200
> +++
> /home/echaudron/Documents/Scratch/ovs/tests/system-kmod-testsuite.dir/at-groups/13/stdout
> 2022-07-04 18:48:07.785404896 +0200
> @@ -1,2 +1,2 @@
> -3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +3 packets transmitted, 1 received, 66.6667% packet loss, time 0ms
> ...
>
> Can you please investigate? I know reverting the patch will not offload to
> TC, but it’s not working, so it’s better to not offload and work than to
> offload and not work ;)
>
> //Eelco
>
> On 4 Jul 2022, at 17:25, Abhiram RN wrote:
>
> Hi Eelco,
>
> I do remember testing with this. I don't have full logs as such. But from
> one of the mail I had sent out I found below logs (this is with the change
> of course) where it's offloaded to TC (PSB). [ Because there seems to be
> support needed in HW driver (which is not present) and hence flows are not
> offloaded to HW which is expected]. But as you can see flows are in
> *dp:tc*.. (I can see 39 packets... so it's not only the first one). Not
> sure if anything got changed post that which might have caused this to fail
> now.
>
> [root@rhos-nfv-09 ~]# ovs-appctl dpctl/dump-flows -m
> ufid:edb7d581-9f14-42df-9e50-69a4923e1dbd,
> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_0),packet_type(ns=0/0,id=0/0),eth(src=6a:62:68:b2:61:f6,dst=66:f0:e4:1e:2c:bb),eth_type(0x0800),ipv4(src=
> 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no),
> packets:39, bytes:3276, used:0.260s, *dp:tc*,
> actions:set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys,enp4s0f0_1
> ufid:519d8cfd-9821-467f-9f61-e2bf61cdde60,
> skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_1),packet_type(ns=0/0,id=0/0),eth(src=66:f0:e4:1e:2c:bb,dst=6a:62:68:b2:61:f6),eth_type(0x0800),ipv4(src=
> 0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no),
> packets:39, bytes:3276, used:0.260s, *dp:tc*,
> actions:enp4s0f0_0,set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys
> [root@rhos-nfv-09 ~]#
>
> JFYI,
> Without this change still flows won't get offloaded to TC. Below is the
> error what I used to get and the flows used to be in *dp:ovs*
> 2022-02-25T14:33:04.134Z|00151|netdev_offload_tc|INFO|init: failed to get
> ifindex for erspan0: Operation not supported
> 2022-02-25T14:33:04.134Z|00152|netdev_offload|INFO|erspan0: No suitable
> flow API found.
>
> Thanks & Regards,
> Abhiram R N
>
> On Mon, Jul 4, 2022 at 7:48 PM Eelco Chaudron <echaudro@redhat.com> wrote:
>
>>
>>
>> On 5 Apr 2022, at 0:48, Ilya Maximets wrote:
>>
>> > On 3/23/22 14:01, Eelco Chaudron wrote:
>> >>
>> >>
>> >> On 23 Mar 2022, at 11:43, Abhiram R N wrote:
>> >>
>> >>> When enabling offload for ERSPAN we are seeing one error as below
>> >>>
>> >>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0:
>> >>> Operation not supported
>> >>> netdev_offload|INFO|erspan0: No suitable flow API found.
>> >>>
>> >>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this
>> >>> error
>> >>
>> >> The change looks good to me, but there is no real unit test to verify
>> this, and I have no real setup to test this.
>> >>
>> >> So based on a pure visible and compile only check:
>> >>
>> >> Acked-by: Eelco Chaudron <echaudro@redhat.com>
>> >>
>> >>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com>
>> >>> ---
>> >
>> > I don't see why this can cause any problems or why this callback
>> > wasn't set from the beginning, so applied. Thanks!
>> >
>> > Best regards, Ilya Maximets.
>>
>> Hi Abhiram/Ilya,
>>
>> Guess it takes a while but basic ping seems to be failing when offloading
>> to TC with t his change.
>>
>> If you enable TC offload the following tests are failing on latest master:
>>
>> datapath - ping over erspan v1 tunnel FAILED (
>> system-traffic.at:467)
>> datapath - ping over erspan v2 tunnel FAILED (
>> system-traffic.at:504)
>> datapath - ping over ip6erspan v1 tunnel FAILED (
>> system-traffic.at:544)
>> datapath - ping over ip6erspan v2 tunnel FAILED (
>> system-traffic.at:585)
>>
>>
>> For example the following diff causes the first test to fail with tc:
>>
>>
>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
>> index e16ea6027..fa6b84b4e 100644
>> --- a/tests/system-traffic.at
>> +++ b/tests/system-traffic.at
>> @@ -440,6 +440,9 @@ OVS_CHECK_GRE()
>> OVS_CHECK_ERSPAN()
>>
>> OVS_TRAFFIC_VSWITCHD_START()
>> +
>> +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true
>> +
>> ADD_BR([br-underlay])
>>
>> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
>>
>>
>> I think it’s because we are not adding any of the erspan fields to the
>> offload tunnel encapsulated.
>>
>>
>> Abhiram, wondering if this was really working in your case, as it seems
>> only the first packet is passed on.
>>
>> I guess we might need to undo this change, or you need to come up with a
>> fix for this.
>>
>>
>> Cheers,
>>
>> Eelco
>>
>>
>
> --
> Always put yourself in others' shoes. If you feel that it hurts you, it
> probably hurts the other person, too.
>
> Regards
> ABHIRAM
>
>
On 7/4/22 18:52, Eelco Chaudron wrote: > Hi Abhiram, > > It’s been there since your patch was committed to the repo. > I did a quick verification again, and it is: > > git checkout 7539b4e45 > <edit test> > git diff > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 4a7fa49fc..b55d757bf 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -393,6 +393,9 @@ OVS_CHECK_GRE() > OVS_CHECK_ERSPAN() > > OVS_TRAFFIC_VSWITCHD_START() > + > +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true > + > ADD_BR([br-underlay]) > > make -j 32 check-kernel TESTSUITEFLAGS='-k "ping over erspan v1 tunnel" -v' > ... > ./system-traffic.at:417: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC > ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' > NS_EXEC_HEREDOC > ./system-traffic.at:423: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC > ping -s 1200 -i 0.3 -c 3 10.1.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' > NS_EXEC_HEREDOC > --- - 2022-07-04 18:48:07.788952607 +0200 > +++ /home/echaudron/Documents/Scratch/ovs/tests/system-kmod-testsuite.dir/at-groups/13/stdout 2022-07-04 18:48:07.785404896 +0200 > @@ -1,2 +1,2 @@ > -3 packets transmitted, 3 received, 0% packet loss, time 0ms > +3 packets transmitted, 1 received, 66.6667% packet loss, time 0ms > ... > > > Can you please investigate? I know reverting the patch will not offload to TC, but it’s not working, so it’s better to not offload and work than to offload and not work ;) I sent some patches to resolve the issue: https://patchwork.ozlabs.org/project/openvswitch/list/?series=307976&state=* It's not a problem of ERSPAN in particular, but a wider problem with ignoring a lot of tunnel keys and flags. ERSPAN offload will probably work if no options are specified, but I'm not sure how much sense it makes this way. Best regards, Ilya Maximets. > > //Eelco > > > On 4 Jul 2022, at 17:25, Abhiram RN wrote: > > Hi Eelco, > > I do remember testing with this. I don't have full logs as such. But from one of the mail I had sent out I found below logs (this is with the change of course) where it's offloaded to TC (PSB). [ Because there seems to be support needed in HW driver (which is not present) and hence flows are not offloaded to HW which is expected]. But as you can see flows are in *dp:tc*.. (I can see 39 packets... so it's not only the first one). Not sure if anything got changed post that which might have caused this to fail now. > > [root@rhos-nfv-09 ~]# ovs-appctl dpctl/dump-flows -m > ufid:edb7d581-9f14-42df-9e50-69a4923e1dbd, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_0),packet_type(ns=0/0,id=0/0),eth(src=6a:62:68:b2:61:f6,dst=66:f0:e4:1e:2c:bb),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no <http://0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no>), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys,enp4s0f0_1 > ufid:519d8cfd-9821-467f-9f61-e2bf61cdde60, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_1),packet_type(ns=0/0,id=0/0),eth(src=66:f0:e4:1e:2c:bb,dst=6a:62:68:b2:61:f6),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no <http://0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no>), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:enp4s0f0_0,set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys > [root@rhos-nfv-09 ~]# > > JFYI, > Without this change still flows won't get offloaded to TC. Below is the error what I used to get and the flows used to be in *dp:ovs* > 2022-02-25T14:33:04.134Z|00151|netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: Operation not supported > 2022-02-25T14:33:04.134Z|00152|netdev_offload|INFO|erspan0: No suitable flow API found. > > Thanks & Regards, > Abhiram R N > > On Mon, Jul 4, 2022 at 7:48 PM Eelco Chaudron <echaudro@redhat.com <mailto:echaudro@redhat.com>> wrote: > > > > On 5 Apr 2022, at 0:48, Ilya Maximets wrote: > > > On 3/23/22 14:01, Eelco Chaudron wrote: > >> > >> > >> On 23 Mar 2022, at 11:43, Abhiram R N wrote: > >> > >>> When enabling offload for ERSPAN we are seeing one error as below > >>> > >>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: > >>> Operation not supported > >>> netdev_offload|INFO|erspan0: No suitable flow API found. > >>> > >>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this > >>> error > >> > >> The change looks good to me, but there is no real unit test to verify this, and I have no real setup to test this. > >> > >> So based on a pure visible and compile only check: > >> > >> Acked-by: Eelco Chaudron <echaudro@redhat.com <mailto:echaudro@redhat.com>> > >> > >>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com <mailto:abhiramrn@gmail.com>> > >>> --- > > > > I don't see why this can cause any problems or why this callback > > wasn't set from the beginning, so applied. Thanks! > > > > Best regards, Ilya Maximets. > > Hi Abhiram/Ilya, > > Guess it takes a while but basic ping seems to be failing when offloading to TC with t his change. > > If you enable TC offload the following tests are failing on latest master: > > datapath - ping over erspan v1 tunnel FAILED (system-traffic.at:467 <http://system-traffic.at:467>) > datapath - ping over erspan v2 tunnel FAILED (system-traffic.at:504 <http://system-traffic.at:504>) > datapath - ping over ip6erspan v1 tunnel FAILED (system-traffic.at:544 <http://system-traffic.at:544>) > datapath - ping over ip6erspan v2 tunnel FAILED (system-traffic.at:585 <http://system-traffic.at:585>) > > > For example the following diff causes the first test to fail with tc: > > > diff --git a/tests/system-traffic.at <http://system-traffic.at> b/tests/system-traffic.at <http://system-traffic.at> > index e16ea6027..fa6b84b4e 100644 > --- a/tests/system-traffic.at <http://system-traffic.at> > +++ b/tests/system-traffic.at <http://system-traffic.at> > @@ -440,6 +440,9 @@ OVS_CHECK_GRE() > OVS_CHECK_ERSPAN() > > OVS_TRAFFIC_VSWITCHD_START() > + > +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true > + > ADD_BR([br-underlay]) > > AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > > > I think it’s because we are not adding any of the erspan fields to the offload tunnel encapsulated. > > > Abhiram, wondering if this was really working in your case, as it seems only the first packet is passed on. > > I guess we might need to undo this change, or you need to come up with a fix for this. > > > Cheers, > > Eelco > > > > -- > Always put yourself in others' shoes. If you feel that it hurts you, it probably hurts the other person, too. > > Regards > ABHIRAM >
On 5 Jul 2022, at 0:48, Ilya Maximets wrote: > On 7/4/22 18:52, Eelco Chaudron wrote: >> Hi Abhiram, >> >> It’s been there since your patch was committed to the repo. >> I did a quick verification again, and it is: >> >> git checkout 7539b4e45 >> <edit test> >> git diff >> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >> index 4a7fa49fc..b55d757bf 100644 >> --- a/tests/system-traffic.at >> +++ b/tests/system-traffic.at >> @@ -393,6 +393,9 @@ OVS_CHECK_GRE() >> OVS_CHECK_ERSPAN() >> >> OVS_TRAFFIC_VSWITCHD_START() >> + >> +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true >> + >> ADD_BR([br-underlay]) >> >> make -j 32 check-kernel TESTSUITEFLAGS='-k "ping over erspan v1 tunnel" -v' >> ... >> ./system-traffic.at:417: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC >> ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' >> NS_EXEC_HEREDOC >> ./system-traffic.at:423: ip netns exec at_ns0 sh << NS_EXEC_HEREDOC >> ping -s 1200 -i 0.3 -c 3 10.1.1.100 | grep "transmitted" | sed 's/time.*ms$/time 0ms/' >> NS_EXEC_HEREDOC >> --- - 2022-07-04 18:48:07.788952607 +0200 >> +++ /home/echaudron/Documents/Scratch/ovs/tests/system-kmod-testsuite.dir/at-groups/13/stdout 2022-07-04 18:48:07.785404896 +0200 >> @@ -1,2 +1,2 @@ >> -3 packets transmitted, 3 received, 0% packet loss, time 0ms >> +3 packets transmitted, 1 received, 66.6667% packet loss, time 0ms >> ... >> >> >> Can you please investigate? I know reverting the patch will not offload to TC, but it’s not working, so it’s better to not offload and work than to offload and not work ;) > > I sent some patches to resolve the issue: > https://patchwork.ozlabs.org/project/openvswitch/list/?series=307976&state=* > > It's not a problem of ERSPAN in particular, but a wider > problem with ignoring a lot of tunnel keys and flags. > > ERSPAN offload will probably work if no options are specified, > but I'm not sure how much sense it makes this way. Ilya, Your patches look good, however, the test cases for ERSPAN use ERSPAN-specific options, so they will fall back to kerne dp. Abhiram, so if you need a specific ERSPAN option with HW offload this will not work. This is because the TC kernel infrastructure does not allow these to be passed to the kernel. >> >> //Eelco >> >> >> On 4 Jul 2022, at 17:25, Abhiram RN wrote: >> >> Hi Eelco, >> >> I do remember testing with this. I don't have full logs as such. But from one of the mail I had sent out I found below logs (this is with the change of course) where it's offloaded to TC (PSB). [ Because there seems to be support needed in HW driver (which is not present) and hence flows are not offloaded to HW which is expected]. But as you can see flows are in *dp:tc*.. (I can see 39 packets... so it's not only the first one). Not sure if anything got changed post that which might have caused this to fail now. >> >> [root@rhos-nfv-09 ~]# ovs-appctl dpctl/dump-flows -m >> ufid:edb7d581-9f14-42df-9e50-69a4923e1dbd, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_0),packet_type(ns=0/0,id=0/0),eth(src=6a:62:68:b2:61:f6,dst=66:f0:e4:1e:2c:bb),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no <http://0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no> ), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys,enp4s0f0_1 >> ufid:519d8cfd-9821-467f-9f61-e2bf61cdde60, skb_priority(0/0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),recirc_id(0),dp_hash(0/0),in_port(enp4s0f0_1),packet_type(ns=0/0,id=0/0),eth(src=66:f0:e4:1e:2c:bb,dst=6a:62:68:b2:61:f6),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no <http://0.0.0.0/0.0.0.0,dst=5.5.5.2/255.255.255.254,proto=0/0,tos=0/0x3,ttl=0/0,frag=no> ), packets:39, bytes:3276, used:0.260s, *dp:tc*, actions:enp4s0f0_0,set(tunnel(tun_id=0x0,src=10.10.10.1,dst=10.10.10.2,ttl=64,flags(key))),erspan_sys >> [root@rhos-nfv-09 ~]# >> >> JFYI, >> Without this change still flows won't get offloaded to TC. Below is the error what I used to get and the flows used to be in *dp:ovs* >> 2022-02-25T14:33:04.134Z|00151|netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: Operation not supported >> 2022-02-25T14:33:04.134Z|00152|netdev_offload|INFO|erspan0: No suitable flow API found. >> >> Thanks & Regards, >> Abhiram R N >> >> On Mon, Jul 4, 2022 at 7:48 PM Eelco Chaudron <echaudro@redhat.com <mailto:echaudro@redhat.com>> wrote: >> >> >> >> On 5 Apr 2022, at 0:48, Ilya Maximets wrote: >> >> > On 3/23/22 14:01, Eelco Chaudron wrote: >> >> >> >> >> >> On 23 Mar 2022, at 11:43, Abhiram R N wrote: >> >> >> >>> When enabling offload for ERSPAN we are seeing one error as below >> >>> >> >>> netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: >> >>> Operation not supported >> >>> netdev_offload|INFO|erspan0: No suitable flow API found. >> >>> >> >>> Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this >> >>> error >> >> >> >> The change looks good to me, but there is no real unit test to verify this, and I have no real setup to test this. >> >> >> >> So based on a pure visible and compile only check: >> >> >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com <mailto:echaudro@redhat.com>> >> >> >> >>> Signed-off-by: Abhiram R N <abhiramrn@gmail.com <mailto:abhiramrn@gmail.com>> >> >>> --- >> > >> > I don't see why this can cause any problems or why this callback >> > wasn't set from the beginning, so applied. Thanks! >> > >> > Best regards, Ilya Maximets. >> >> Hi Abhiram/Ilya, >> >> Guess it takes a while but basic ping seems to be failing when offloading to TC with t his change. >> >> If you enable TC offload the following tests are failing on latest master: >> >> datapath - ping over erspan v1 tunnel FAILED (system-traffic.at:467 <http://system-traffic.at:467> ) >> datapath - ping over erspan v2 tunnel FAILED (system-traffic.at:504 <http://system-traffic.at:504> ) >> datapath - ping over ip6erspan v1 tunnel FAILED (system-traffic.at:544 <http://system-traffic.at:544> ) >> datapath - ping over ip6erspan v2 tunnel FAILED (system-traffic.at:585 <http://system-traffic.at:585> ) >> >> >> For example the following diff causes the first test to fail with tc: >> >> >> diff --git a/tests/system-traffic.at <http://system-traffic.at> b/tests/system-traffic.at <http://system-traffic.at> >> index e16ea6027..fa6b84b4e 100644 >> --- a/tests/system-traffic.at <http://system-traffic.at> >> +++ b/tests/system-traffic.at <http://system-traffic.at> >> @@ -440,6 +440,9 @@ OVS_CHECK_GRE() >> OVS_CHECK_ERSPAN() >> >> OVS_TRAFFIC_VSWITCHD_START() >> + >> +ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true >> + >> ADD_BR([br-underlay]) >> >> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> >> >> I think it’s because we are not adding any of the erspan fields to the offload tunnel encapsulated. >> >> >> Abhiram, wondering if this was really working in your case, as it seems only the first packet is passed on. >> >> I guess we might need to undo this change, or you need to come up with a fix for this. >> >> >> Cheers, >> >> Eelco >> >> >> >> -- >> Always put yourself in others' shoes. If you feel that it hurts you, it probably hurts the other person, too. >> >> Regards >> ABHIRAM >>
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 64331f74b..3b3927865 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -1245,7 +1245,8 @@ netdev_vport_tunnel_register(void) .type = "erspan", .build_header = netdev_erspan_build_header, .push_header = netdev_erspan_push_header, - .pop_header = netdev_erspan_pop_header + .pop_header = netdev_erspan_pop_header, + .get_ifindex = NETDEV_VPORT_GET_IFINDEX }, {{NULL, NULL, 0, 0}} }, @@ -1255,7 +1256,8 @@ netdev_vport_tunnel_register(void) .type = "ip6erspan", .build_header = netdev_erspan_build_header, .push_header = netdev_erspan_push_header, - .pop_header = netdev_erspan_pop_header + .pop_header = netdev_erspan_pop_header, + .get_ifindex = NETDEV_VPORT_GET_IFINDEX }, {{NULL, NULL, 0, 0}} },
When enabling offload for ERSPAN we are seeing one error as below netdev_offload_tc|INFO|init: failed to get ifindex for erspan0: Operation not supported netdev_offload|INFO|erspan0: No suitable flow API found. Adding the NETDEV_VPORT_GET_IFINDEX to ERSPAN device resolves this error Signed-off-by: Abhiram R N <abhiramrn@gmail.com> --- lib/netdev-vport.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)