| Message ID | 165271547144.991159.143404720230756066.stgit@ebuild |
|---|---|
| State | Superseded |
| Headers | show |
| Series | netdev-offload-tc: Add support for the check_pkt_len action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 2022-05-16 6:38 PM, Eelco Chaudron wrote: > This patch will properly initialize offload, as it requires the > setting to be enabled before starting ovs-vswitchd (or do a > restart once configured). > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Acked-by: Mike Pattrick <mkp@redhat.com> > --- > v2: > - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's > just a note. setting hw-offload true first time doesn't actually require restarting ovs as netdev_set_flow_api_enabled() calls ovsthread_once_start() and does its logic once hw-offload is true. But changing from true to false does require restarting ovs. Acked-by: Roi Dayan <roid@nvidia.com> > tests/ofproto-macros.at | 6 +++++- > tests/system-kmod-macros.at | 4 ++-- > tests/system-offloads-traffic.at | 9 +++------ > tests/system-tso-macros.at | 4 ++-- > tests/system-userspace-macros.at | 4 ++-- > 5 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at > index 7051d9539..de6bd6c2e 100644 > --- a/tests/ofproto-macros.at > +++ b/tests/ofproto-macros.at > @@ -138,7 +138,7 @@ m4_divert_pop([PREPARE_TESTS]) > > m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) > > -# _OVS_VSWITCHD_START([vswitchd-aux-args]) > +# _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args] [pre-vswitchd-commands]) > # > # Creates an empty database and starts ovsdb-server. > # Starts ovs-vswitchd, with additional arguments 'vswitchd-aux-args'. > @@ -159,6 +159,9 @@ m4_define([_OVS_VSWITCHD_START], > dnl Initialize database. > AT_CHECK([ovs-vsctl --no-wait init $2]) > > + dnl Run extra commands before ovs-vswitchd starts. > + AT_CHECK([:; $3]) > + > dnl Start ovs-vswitchd. > AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) > AT_CAPTURE_FILE([ovs-vswitchd.log]) > @@ -174,6 +177,7 @@ m4_define([_OVS_VSWITCHD_START], > /ofproto|INFO|datapath ID changed to fedcba9876543210/d > /dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d > /netlink_socket|INFO|netlink: could not enable listening to all nsid/d > +/netdev_offload|INFO|netdev: Flow API Enabled/d > /probe tc:/d > /setting extended ack support failed/d > /tc: Using policy/d']]) > diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at > index 86d633ac4..a8eadc483 100644 > --- a/tests/system-kmod-macros.at > +++ b/tests/system-kmod-macros.at > @@ -4,7 +4,7 @@ > # appropriate type and properties > m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) > > -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) > +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) > # > # Creates a database and starts ovsdb-server, starts ovs-vswitchd > # connected to that database, calls ovs-vsctl to create a bridge named > @@ -24,7 +24,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], > ]) > on_exit 'ovs-dpctl del-dp ovs-system' > on_exit 'ovs-appctl dpctl/flush-conntrack' > - _OVS_VSWITCHD_START([]) > + _OVS_VSWITCHD_START([], [], [$3]) > dnl Add bridges, ports, etc. > AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) > ]) > diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at > index 80bc1dd5c..705a50079 100644 > --- a/tests/system-offloads-traffic.at > +++ b/tests/system-offloads-traffic.at > @@ -39,9 +39,8 @@ AT_CLEANUP > > > AT_SETUP([offloads - ping between two ports - offloads enabled]) > -OVS_TRAFFIC_VSWITCHD_START() > +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) > > -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) > AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > > ADD_NAMESPACES(at_ns0, at_ns1) > @@ -97,8 +96,7 @@ AT_CLEANUP > AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled]) > AT_KEYWORDS([ingress_policing]) > AT_SKIP_IF([test $HAVE_TC = "no"]) > -OVS_TRAFFIC_VSWITCHD_START() > -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) > +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) > AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > ADD_NAMESPACES(at_ns0) > ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > @@ -146,8 +144,7 @@ AT_CLEANUP > AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled]) > AT_KEYWORDS([ingress_policing_kpkts]) > AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"]) > -OVS_TRAFFIC_VSWITCHD_START() > -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) > +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) > AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > ADD_NAMESPACES(at_ns0) > ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > diff --git a/tests/system-tso-macros.at b/tests/system-tso-macros.at > index 1a8004761..1881c28fb 100644 > --- a/tests/system-tso-macros.at > +++ b/tests/system-tso-macros.at > @@ -4,7 +4,7 @@ > # appropriate type and properties > m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) > > -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) > +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) > # > # Creates a database and starts ovsdb-server, starts ovs-vswitchd > # connected to that database, calls ovs-vsctl to create a bridge named > @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco > m4_define([OVS_TRAFFIC_VSWITCHD_START], > [ > OVS_WAIT_WHILE([ip link show ovs-netdev]) > - _OVS_VSWITCHD_START([--disable-system]) > + _OVS_VSWITCHD_START([--disable-system] [] [$3]) > dnl Add bridges, ports, etc. > OVS_WAIT_WHILE([ip link show br0]) > AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:userspace-tso-enable=true]) > diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at > index f639ba53a..24ece8e5c 100644 > --- a/tests/system-userspace-macros.at > +++ b/tests/system-userspace-macros.at > @@ -4,7 +4,7 @@ > # appropriate type and properties > m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) > > -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) > +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) > # > # Creates a database and starts ovsdb-server, starts ovs-vswitchd > # connected to that database, calls ovs-vsctl to create a bridge named > @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco > m4_define([OVS_TRAFFIC_VSWITCHD_START], > [ > OVS_WAIT_WHILE([ip link show ovs-netdev]) > - _OVS_VSWITCHD_START([--disable-system]) > + _OVS_VSWITCHD_START([--disable-system] [] [$3]) > dnl Add bridges, ports, etc. > OVS_WAIT_WHILE([ip link show br0]) > AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) >
On 1 Jun 2022, at 12:14, Roi Dayan wrote: > On 2022-05-16 6:38 PM, Eelco Chaudron wrote: >> This patch will properly initialize offload, as it requires the >> setting to be enabled before starting ovs-vswitchd (or do a >> restart once configured). >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> Acked-by: Mike Pattrick <mkp@redhat.com> >> --- >> v2: >> - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's >> > > just a note. setting hw-offload true first time doesn't actually > require restarting ovs as netdev_set_flow_api_enabled() calls > ovsthread_once_start() and does its logic once hw-offload is true. > But changing from true to false does require restarting ovs. Thanks for the review Roi! Unfortunately, I can remember a case where I was doing some testing and then enabled offloading, and it would not work. After a restart, it would, and it was definitely related to not being enabled at startup. When I brought it up someone pointed me to the documentation, and I did not bother :( Hopefully, my memory will come back, and I remember how I did it :) > > Acked-by: Roi Dayan <roid@nvidia.com> > > >> tests/ofproto-macros.at | 6 +++++- >> tests/system-kmod-macros.at | 4 ++-- >> tests/system-offloads-traffic.at | 9 +++------ >> tests/system-tso-macros.at | 4 ++-- >> tests/system-userspace-macros.at | 4 ++-- >> 5 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >> index 7051d9539..de6bd6c2e 100644 >> --- a/tests/ofproto-macros.at >> +++ b/tests/ofproto-macros.at >> @@ -138,7 +138,7 @@ m4_divert_pop([PREPARE_TESTS]) >> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >> -# _OVS_VSWITCHD_START([vswitchd-aux-args]) >> +# _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args] [pre-vswitchd-commands]) >> # >> # Creates an empty database and starts ovsdb-server. >> # Starts ovs-vswitchd, with additional arguments 'vswitchd-aux-args'. >> @@ -159,6 +159,9 @@ m4_define([_OVS_VSWITCHD_START], >> dnl Initialize database. >> AT_CHECK([ovs-vsctl --no-wait init $2]) >> + dnl Run extra commands before ovs-vswitchd starts. >> + AT_CHECK([:; $3]) >> + >> dnl Start ovs-vswitchd. >> AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) >> AT_CAPTURE_FILE([ovs-vswitchd.log]) >> @@ -174,6 +177,7 @@ m4_define([_OVS_VSWITCHD_START], >> /ofproto|INFO|datapath ID changed to fedcba9876543210/d >> /dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d >> /netlink_socket|INFO|netlink: could not enable listening to all nsid/d >> +/netdev_offload|INFO|netdev: Flow API Enabled/d >> /probe tc:/d >> /setting extended ack support failed/d >> /tc: Using policy/d']]) >> diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at >> index 86d633ac4..a8eadc483 100644 >> --- a/tests/system-kmod-macros.at >> +++ b/tests/system-kmod-macros.at >> @@ -4,7 +4,7 @@ >> # appropriate type and properties >> m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) >> -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) >> +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) >> # >> # Creates a database and starts ovsdb-server, starts ovs-vswitchd >> # connected to that database, calls ovs-vsctl to create a bridge named >> @@ -24,7 +24,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], >> ]) >> on_exit 'ovs-dpctl del-dp ovs-system' >> on_exit 'ovs-appctl dpctl/flush-conntrack' >> - _OVS_VSWITCHD_START([]) >> + _OVS_VSWITCHD_START([], [], [$3]) >> dnl Add bridges, ports, etc. >> AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) >> ]) >> diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at >> index 80bc1dd5c..705a50079 100644 >> --- a/tests/system-offloads-traffic.at >> +++ b/tests/system-offloads-traffic.at >> @@ -39,9 +39,8 @@ AT_CLEANUP >> AT_SETUP([offloads - ping between two ports - offloads enabled]) >> -OVS_TRAFFIC_VSWITCHD_START() >> +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) >> -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> ADD_NAMESPACES(at_ns0, at_ns1) >> @@ -97,8 +96,7 @@ AT_CLEANUP >> AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled]) >> AT_KEYWORDS([ingress_policing]) >> AT_SKIP_IF([test $HAVE_TC = "no"]) >> -OVS_TRAFFIC_VSWITCHD_START() >> -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) >> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> ADD_NAMESPACES(at_ns0) >> ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> @@ -146,8 +144,7 @@ AT_CLEANUP >> AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled]) >> AT_KEYWORDS([ingress_policing_kpkts]) >> AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"]) >> -OVS_TRAFFIC_VSWITCHD_START() >> -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) >> AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) >> ADD_NAMESPACES(at_ns0) >> ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") >> diff --git a/tests/system-tso-macros.at b/tests/system-tso-macros.at >> index 1a8004761..1881c28fb 100644 >> --- a/tests/system-tso-macros.at >> +++ b/tests/system-tso-macros.at >> @@ -4,7 +4,7 @@ >> # appropriate type and properties >> m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) >> -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) >> +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) >> # >> # Creates a database and starts ovsdb-server, starts ovs-vswitchd >> # connected to that database, calls ovs-vsctl to create a bridge named >> @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco >> m4_define([OVS_TRAFFIC_VSWITCHD_START], >> [ >> OVS_WAIT_WHILE([ip link show ovs-netdev]) >> - _OVS_VSWITCHD_START([--disable-system]) >> + _OVS_VSWITCHD_START([--disable-system] [] [$3]) >> dnl Add bridges, ports, etc. >> OVS_WAIT_WHILE([ip link show br0]) >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:userspace-tso-enable=true]) >> diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at >> index f639ba53a..24ece8e5c 100644 >> --- a/tests/system-userspace-macros.at >> +++ b/tests/system-userspace-macros.at >> @@ -4,7 +4,7 @@ >> # appropriate type and properties >> m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) >> -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) >> +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) >> # >> # Creates a database and starts ovsdb-server, starts ovs-vswitchd >> # connected to that database, calls ovs-vsctl to create a bridge named >> @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco >> m4_define([OVS_TRAFFIC_VSWITCHD_START], >> [ >> OVS_WAIT_WHILE([ip link show ovs-netdev]) >> - _OVS_VSWITCHD_START([--disable-system]) >> + _OVS_VSWITCHD_START([--disable-system] [] [$3]) >> dnl Add bridges, ports, etc. >> OVS_WAIT_WHILE([ip link show br0]) >> AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) >>
On 6/1/22 12:37, Eelco Chaudron wrote: > > > On 1 Jun 2022, at 12:14, Roi Dayan wrote: > >> On 2022-05-16 6:38 PM, Eelco Chaudron wrote: >>> This patch will properly initialize offload, as it requires the >>> setting to be enabled before starting ovs-vswitchd (or do a >>> restart once configured). >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> Acked-by: Mike Pattrick <mkp@redhat.com> >>> --- >>> v2: >>> - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's >>> >> >> just a note. setting hw-offload true first time doesn't actually >> require restarting ovs as netdev_set_flow_api_enabled() calls >> ovsthread_once_start() and does its logic once hw-offload is true. >> But changing from true to false does require restarting ovs. > > Thanks for the review Roi! Unfortunately, I can remember a case where I > was doing some testing and then enabled offloading, and it would not work. > After a restart, it would, and it was definitely related to not being > enabled at startup. When I brought it up someone pointed me to the > documentation, and I did not bother :( Hopefully, my memory will come back, > and I remember how I did it :) One of the possible issues, I think, is if some traffic manages to pass through OVS before hw-offload is enabled, it will be installed to the kernel and will not be offloaded once the knob is turned on. I remember having some weird issues with the initialization sequence, but I don't remember what that was exactly... :/ Anyway, I guess, it's a safe bet to set the value before the start. > >> >> Acked-by: Roi Dayan <roid@nvidia.com> >> >> >>> tests/ofproto-macros.at | 6 +++++- >>> tests/system-kmod-macros.at | 4 ++-- >>> tests/system-offloads-traffic.at | 9 +++------ >>> tests/system-tso-macros.at | 4 ++-- >>> tests/system-userspace-macros.at | 4 ++-- >>> 5 files changed, 14 insertions(+), 13 deletions(-) >>> >>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>> index 7051d9539..de6bd6c2e 100644 >>> --- a/tests/ofproto-macros.at >>> +++ b/tests/ofproto-macros.at >>> @@ -138,7 +138,7 @@ m4_divert_pop([PREPARE_TESTS]) >>> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >>> -# _OVS_VSWITCHD_START([vswitchd-aux-args]) >>> +# _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args] [pre-vswitchd-commands]) >>> # >>> # Creates an empty database and starts ovsdb-server. >>> # Starts ovs-vswitchd, with additional arguments 'vswitchd-aux-args'. >>> @@ -159,6 +159,9 @@ m4_define([_OVS_VSWITCHD_START], >>> dnl Initialize database. >>> AT_CHECK([ovs-vsctl --no-wait init $2]) Here we already have the $2 argument that can be used to set any database values by passing '-- set something' and I don't see it actually being used anywhere in the testsuite. Can we just use that instead of adding a third argument? Best regards, Ilya Maximets.
On 2 Jun 2022, at 14:01, Ilya Maximets wrote: > On 6/1/22 12:37, Eelco Chaudron wrote: >> >> >> On 1 Jun 2022, at 12:14, Roi Dayan wrote: >> >>> On 2022-05-16 6:38 PM, Eelco Chaudron wrote: >>>> This patch will properly initialize offload, as it requires the >>>> setting to be enabled before starting ovs-vswitchd (or do a >>>> restart once configured). >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> Acked-by: Mike Pattrick <mkp@redhat.com> >>>> --- >>>> v2: >>>> - Unified all the OVS_TRAFFIC_VSWITCHD_START macro's >>>> >>> >>> just a note. setting hw-offload true first time doesn't actually >>> require restarting ovs as netdev_set_flow_api_enabled() calls >>> ovsthread_once_start() and does its logic once hw-offload is true. >>> But changing from true to false does require restarting ovs. >> >> Thanks for the review Roi! Unfortunately, I can remember a case where I >> was doing some testing and then enabled offloading, and it would not work. >> After a restart, it would, and it was definitely related to not being >> enabled at startup. When I brought it up someone pointed me to the >> documentation, and I did not bother :( Hopefully, my memory will come back, >> and I remember how I did it :) > > One of the possible issues, I think, is if some traffic manages > to pass through OVS before hw-offload is enabled, it will be > installed to the kernel and will not be offloaded once the > knob is turned on. > > I remember having some weird issues with the initialization > sequence, but I don't remember what that was exactly... :/ > > Anyway, I guess, it's a safe bet to set the value before the start. > >> >>> >>> Acked-by: Roi Dayan <roid@nvidia.com> >>> >>> >>>> tests/ofproto-macros.at | 6 +++++- >>>> tests/system-kmod-macros.at | 4 ++-- >>>> tests/system-offloads-traffic.at | 9 +++------ >>>> tests/system-tso-macros.at | 4 ++-- >>>> tests/system-userspace-macros.at | 4 ++-- >>>> 5 files changed, 14 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at >>>> index 7051d9539..de6bd6c2e 100644 >>>> --- a/tests/ofproto-macros.at >>>> +++ b/tests/ofproto-macros.at >>>> @@ -138,7 +138,7 @@ m4_divert_pop([PREPARE_TESTS]) >>>> m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) >>>> -# _OVS_VSWITCHD_START([vswitchd-aux-args]) >>>> +# _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args] [pre-vswitchd-commands]) >>>> # >>>> # Creates an empty database and starts ovsdb-server. >>>> # Starts ovs-vswitchd, with additional arguments 'vswitchd-aux-args'. >>>> @@ -159,6 +159,9 @@ m4_define([_OVS_VSWITCHD_START], >>>> dnl Initialize database. >>>> AT_CHECK([ovs-vsctl --no-wait init $2]) > > Here we already have the $2 argument that can be used to set > any database values by passing '-- set something' and I don't > see it actually being used anywhere in the testsuite. > Can we just use that instead of adding a third argument? Good catch! I’ve re-used this and it works fine! I will sent out a v4 tomorrow. //Eelco
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 7051d9539..de6bd6c2e 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -138,7 +138,7 @@ m4_divert_pop([PREPARE_TESTS]) m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) -# _OVS_VSWITCHD_START([vswitchd-aux-args]) +# _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args] [pre-vswitchd-commands]) # # Creates an empty database and starts ovsdb-server. # Starts ovs-vswitchd, with additional arguments 'vswitchd-aux-args'. @@ -159,6 +159,9 @@ m4_define([_OVS_VSWITCHD_START], dnl Initialize database. AT_CHECK([ovs-vsctl --no-wait init $2]) + dnl Run extra commands before ovs-vswitchd starts. + AT_CHECK([:; $3]) + dnl Start ovs-vswitchd. AT_CHECK([ovs-vswitchd $1 --detach --no-chdir --pidfile --log-file -vvconn -vofproto_dpif -vunixctl], [0], [], [stderr]) AT_CAPTURE_FILE([ovs-vswitchd.log]) @@ -174,6 +177,7 @@ m4_define([_OVS_VSWITCHD_START], /ofproto|INFO|datapath ID changed to fedcba9876543210/d /dpdk|INFO|DPDK Disabled - Use other_config:dpdk-init to enable/d /netlink_socket|INFO|netlink: could not enable listening to all nsid/d +/netdev_offload|INFO|netdev: Flow API Enabled/d /probe tc:/d /setting extended ack support failed/d /tc: Using policy/d']]) diff --git a/tests/system-kmod-macros.at b/tests/system-kmod-macros.at index 86d633ac4..a8eadc483 100644 --- a/tests/system-kmod-macros.at +++ b/tests/system-kmod-macros.at @@ -4,7 +4,7 @@ # appropriate type and properties m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) # # Creates a database and starts ovsdb-server, starts ovs-vswitchd # connected to that database, calls ovs-vsctl to create a bridge named @@ -24,7 +24,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], ]) on_exit 'ovs-dpctl del-dp ovs-system' on_exit 'ovs-appctl dpctl/flush-conntrack' - _OVS_VSWITCHD_START([]) + _OVS_VSWITCHD_START([], [], [$3]) dnl Add bridges, ports, etc. AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2]) ]) diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at index 80bc1dd5c..705a50079 100644 --- a/tests/system-offloads-traffic.at +++ b/tests/system-offloads-traffic.at @@ -39,9 +39,8 @@ AT_CLEANUP AT_SETUP([offloads - ping between two ports - offloads enabled]) -OVS_TRAFFIC_VSWITCHD_START() +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) ADD_NAMESPACES(at_ns0, at_ns1) @@ -97,8 +96,7 @@ AT_CLEANUP AT_SETUP([offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled]) AT_KEYWORDS([ingress_policing]) AT_SKIP_IF([test $HAVE_TC = "no"]) -OVS_TRAFFIC_VSWITCHD_START() -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) ADD_NAMESPACES(at_ns0) ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") @@ -146,8 +144,7 @@ AT_CLEANUP AT_SETUP([offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled]) AT_KEYWORDS([ingress_policing_kpkts]) AT_SKIP_IF([test $SUPPORT_TC_INGRESS_PPS = "no"]) -OVS_TRAFFIC_VSWITCHD_START() -AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) +OVS_TRAFFIC_VSWITCHD_START([], [], [ovs-vsctl --no-wait set Open_vSwitch . other_config:hw-offload=true]) AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) ADD_NAMESPACES(at_ns0) ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") diff --git a/tests/system-tso-macros.at b/tests/system-tso-macros.at index 1a8004761..1881c28fb 100644 --- a/tests/system-tso-macros.at +++ b/tests/system-tso-macros.at @@ -4,7 +4,7 @@ # appropriate type and properties m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) # # Creates a database and starts ovsdb-server, starts ovs-vswitchd # connected to that database, calls ovs-vsctl to create a bridge named @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco m4_define([OVS_TRAFFIC_VSWITCHD_START], [ OVS_WAIT_WHILE([ip link show ovs-netdev]) - _OVS_VSWITCHD_START([--disable-system]) + _OVS_VSWITCHD_START([--disable-system] [] [$3]) dnl Add bridges, ports, etc. OVS_WAIT_WHILE([ip link show br0]) AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:userspace-tso-enable=true]) diff --git a/tests/system-userspace-macros.at b/tests/system-userspace-macros.at index f639ba53a..24ece8e5c 100644 --- a/tests/system-userspace-macros.at +++ b/tests/system-userspace-macros.at @@ -4,7 +4,7 @@ # appropriate type and properties m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13,OpenFlow14,OpenFlow15 fail-mode=secure ]]) -# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [=override]) +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [pre-vswitchd-commands]) # # Creates a database and starts ovsdb-server, starts ovs-vswitchd # connected to that database, calls ovs-vsctl to create a bridge named @@ -15,7 +15,7 @@ m4_define([_ADD_BR], [[add-br $1 -- set Bridge $1 datapath_type="netdev" protoco m4_define([OVS_TRAFFIC_VSWITCHD_START], [ OVS_WAIT_WHILE([ip link show ovs-netdev]) - _OVS_VSWITCHD_START([--disable-system]) + _OVS_VSWITCHD_START([--disable-system] [] [$3]) dnl Add bridges, ports, etc. OVS_WAIT_WHILE([ip link show br0]) AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| uuidfilt])], [0], [$2])