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 <[email protected]> >>>> Acked-by: Mike Pattrick <[email protected]> >>>> --- >>>> 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 <[email protected]> >>> >>> >>>> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
