| Message ID | 20230908234009.892803-1-i.maximets@ovn.org |
|---|---|
| State | RFC |
| Headers | show |
| Series | [ovs-dev,RFC] bridge: Re-create bridges when Flow API is enabled. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 9 Sep 2023, at 1:40, Ilya Maximets wrote: > Currently OVS requires restart of ovs-vswitchd after enabling hardware > offload. This is necessary to make sure all the correct features are > probed and all the internal configurations are in the right state. > > Making that process a bit less invasive by re-creating bridges instead > of a full process restart. Documentation is not updated, because > disabling hardware offload still can take effect only after restart. > Hi Ilya, I like the idea of re-creating the bridges, however not sure if it generated any side effects other than the ones you mention in the FIXME. //Eelco > [ > This is not a full implementation, see the FIXME comment in the code. > Posted in support for review of the OVS_ACTION_ATTR_DROP patch set. > I can continue working on the proper implementation once I'm back > from PTO. > ] > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > tests/dpif-netdev.at | 6 +++--- > vswitchd/bridge.c | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at > index 85119fb81..a3e07895a 100644 > --- a/tests/dpif-netdev.at > +++ b/tests/dpif-netdev.at > @@ -410,10 +410,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], > set bridge br0 datapath-type=dummy \ > other-config:datapath-id=1234 fail-mode=secure], > [], [], > [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], > [])]) > - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-vsctl set Open_vSwitch . > other_config:hw-offload=true]) > OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) > + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-ofctl del-flows br0]) > AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) > @@ -473,10 +473,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], > set bridge br0 datapath-type=dummy \ > other-config:datapath-id=1234 fail-mode=secure], > [], [], > [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], > [])]) > - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-vsctl set Open_vSwitch . > other_config:hw-offload=true]) > OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) > + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-ofctl del-flows br0]) > > @@ -550,10 +550,10 @@ > m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], > set bridge br0 datapath-type=dummy \ > other-config:datapath-id=1234 fail-mode=secure], > [], [], > [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], > [])]) > - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-vsctl set Open_vSwitch . > other_config:hw-offload=true]) > OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) > + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg > netdev_dummy:file:dbg]) > > AT_CHECK([ovs-ofctl del-flows br0]) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index e9110c1d8..b3ccdaa06 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -1986,6 +1986,27 @@ port_is_bond_fake_iface(const struct port > *port) > return port->cfg->bond_fake_iface && > !ovs_list_is_short(&port->ifaces); > } > > +static bool > +flow_api_status_changed(void) > +{ > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + static bool current_status = false; > + bool new_status; > + > + new_status = netdev_is_flow_api_enabled(); > + > + if (ovsthread_once_start(&once)) { > + current_status = new_status; > + ovsthread_once_done(&once); > + } > + > + if (current_status != new_status) { > + current_status = new_status; > + return true; > + } > + return false; > +} > + > static void > add_del_bridges(const struct ovsrec_open_vswitch *cfg) > { > @@ -2023,6 +2044,22 @@ add_del_bridges(const struct > ovsrec_open_vswitch *cfg) > } > } > > + if (flow_api_status_changed()) { > + /* Destroy all the remaining bridges if Flow API status > changed > + * as we need to re-probe supported features. Do not delete > + * bridge resources to avoid datapath disruption. */ > + HMAP_FOR_EACH_SAFE (br, node, &all_bridges) { > + /* FIXME: This has to be called with 'false' in order to > not > + * destroy the datapath, but it requires re-working the > + * 'iface_hints' mechanism by supplying hints every time > + * ofproto is created, not only when the type is > initialized. > + * Oterwise, ports will be destroyed anyway. For > userspace > + * datapath that will also mean complete removal of a > bridge > + * port without re-creation that we obviously do not > want. */ > + bridge_destroy(br, true /* false! */); > + } > + } > + > /* Add new bridges. */ > SHASH_FOR_EACH(node, &new_br) { > const struct ovsrec_bridge *br_cfg = node->data; > -- > 2.41.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 9/15/23 17:25, Eelco Chaudron wrote: > > > On 9 Sep 2023, at 1:40, Ilya Maximets wrote: > >> Currently OVS requires restart of ovs-vswitchd after enabling hardware >> offload. This is necessary to make sure all the correct features are >> probed and all the internal configurations are in the right state. >> >> Making that process a bit less invasive by re-creating bridges instead >> of a full process restart. Documentation is not updated, because >> disabling hardware offload still can take effect only after restart. >> > > Hi Ilya, > > I like the idea of re-creating the bridges, however not sure if it generated any > side effects other than the ones you mention in the FIXME. > I have taken a look at the patch, tested it and added some rework of the iface_hints to avoid the datapath ports from being deleted when the bridge is re-created. The recreation of the bridge seems to work but my concern is that, unlike a restart which uses ovs-save to save & restore the flows, groups and tunnel metadata, doing this will leave the bridge with no OpenFlow configuration. During the time the controller takes to reconfigure the bridge, reval threads will wipe the datapath flows. If there is no controller ... well we'll need to wait for the user to program the flows back. This is quite a big behavior change so I'd say if we go for this option we better documment it pretty well. Apart from that I see an error in the log says: 2023-12-13T10:43:44.791Z|00064|ofproto_dpif_rid|ERR|recirc_id 1 left allocated when ofproto (ktest) is destructed But I think (will double check) the recirc node gets cleaned afterwards. Cheers, Adrián > //Eelco > > >> [ >> This is not a full implementation, see the FIXME comment in the code. >> Posted in support for review of the OVS_ACTION_ATTR_DROP patch set. >> I can continue working on the proper implementation once I'm back >> from PTO. >> ] >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> tests/dpif-netdev.at | 6 +++--- >> vswitchd/bridge.c | 37 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >> index 85119fb81..a3e07895a 100644 >> --- a/tests/dpif-netdev.at >> +++ b/tests/dpif-netdev.at >> @@ -410,10 +410,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], >> set bridge br0 datapath-type=dummy \ >> other-config:datapath-id=1234 fail-mode=secure], [], [], >> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) >> @@ -473,10 +473,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], >> set bridge br0 datapath-type=dummy \ >> other-config:datapath-id=1234 fail-mode=secure], [], [], >> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> >> @@ -550,10 +550,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], >> set bridge br0 datapath-type=dummy \ >> other-config:datapath-id=1234 fail-mode=secure], [], [], >> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >> netdev_dummy:file:dbg]) >> >> AT_CHECK([ovs-ofctl del-flows br0]) >> >> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >> index e9110c1d8..b3ccdaa06 100644 >> --- a/vswitchd/bridge.c >> +++ b/vswitchd/bridge.c >> @@ -1986,6 +1986,27 @@ port_is_bond_fake_iface(const struct port *port) >> return port->cfg->bond_fake_iface && !ovs_list_is_short(&port->ifaces); >> } >> >> +static bool >> +flow_api_status_changed(void) >> +{ >> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + static bool current_status = false; >> + bool new_status; >> + >> + new_status = netdev_is_flow_api_enabled(); >> + >> + if (ovsthread_once_start(&once)) { >> + current_status = new_status; >> + ovsthread_once_done(&once); >> + } >> + >> + if (current_status != new_status) { >> + current_status = new_status; >> + return true; >> + } >> + return false; >> +} >> + >> static void >> add_del_bridges(const struct ovsrec_open_vswitch *cfg) >> { >> @@ -2023,6 +2044,22 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg) >> } >> } >> >> + if (flow_api_status_changed()) { >> + /* Destroy all the remaining bridges if Flow API status changed >> + * as we need to re-probe supported features. Do not delete >> + * bridge resources to avoid datapath disruption. */ >> + HMAP_FOR_EACH_SAFE (br, node, &all_bridges) { >> + /* FIXME: This has to be called with 'false' in order to not >> + * destroy the datapath, but it requires re-working the >> + * 'iface_hints' mechanism by supplying hints every time >> + * ofproto is created, not only when the type is initialized. >> + * Oterwise, ports will be destroyed anyway. For userspace >> + * datapath that will also mean complete removal of a bridge >> + * port without re-creation that we obviously do not want. */ >> + bridge_destroy(br, true /* false! */); >> + } >> + } >> + >> /* Add new bridges. */ >> SHASH_FOR_EACH(node, &new_br) { >> const struct ovsrec_bridge *br_cfg = node->data; >> -- >> 2.41.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12/13/23 11:47, Adrian Moreno wrote: > > > On 9/15/23 17:25, Eelco Chaudron wrote: >> >> >> On 9 Sep 2023, at 1:40, Ilya Maximets wrote: >> >>> Currently OVS requires restart of ovs-vswitchd after enabling hardware >>> offload. This is necessary to make sure all the correct features are >>> probed and all the internal configurations are in the right state. >>> >>> Making that process a bit less invasive by re-creating bridges instead >>> of a full process restart. Documentation is not updated, because >>> disabling hardware offload still can take effect only after restart. >>> >> >> Hi Ilya, >> >> I like the idea of re-creating the bridges, however not sure if it generated >> any side effects other than the ones you mention in the FIXME. >> > > I have taken a look at the patch, tested it and added some rework of the > iface_hints to avoid the datapath ports from being deleted when the bridge is > re-created. > > The recreation of the bridge seems to work but my concern is that, unlike a > restart which uses ovs-save to save & restore the flows, groups and tunnel > metadata, doing this will leave the bridge with no OpenFlow configuration. > > During the time the controller takes to reconfigure the bridge, reval threads > will wipe the datapath flows. If there is no controller ... well we'll need to > wait for the user to program the flows back. This is quite a big behavior change > so I'd say if we go for this option we better documment it pretty well. > I'm thinking that, considering this hassle (loosing all flows, meters, tunnel tlv info, etc), isn't it better to simply restart vswitchd? A more more complex solution (probably big enough for this release which is coming fast) could be to recreate ovs-save internally and: - Collect all the flow table, groups, tlv data and meters - Delete the bridge and recreate it - Reconfigure everything back Flow stats will probably be lost in the process, which is a weird outcome of enabling hwol but it's better than loosing all the flows. +Mike Any other ideas? > Apart from that I see an error in the log says: > > 2023-12-13T10:43:44.791Z|00064|ofproto_dpif_rid|ERR|recirc_id 1 left allocated > when ofproto (ktest) is destructed > > But I think (will double check) the recirc node gets cleaned afterwards. > > Cheers, > Adrián > > >> //Eelco >> >> >>> [ >>> This is not a full implementation, see the FIXME comment in the code. >>> Posted in support for review of the OVS_ACTION_ATTR_DROP patch set. >>> I can continue working on the proper implementation once I'm back >>> from PTO. >>> ] >>> >>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>> --- >>> tests/dpif-netdev.at | 6 +++--- >>> vswitchd/bridge.c | 37 +++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 40 insertions(+), 3 deletions(-) >>> >>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >>> index 85119fb81..a3e07895a 100644 >>> --- a/tests/dpif-netdev.at >>> +++ b/tests/dpif-netdev.at >>> @@ -410,10 +410,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], >>> set bridge br0 datapath-type=dummy \ >>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-ofctl del-flows br0]) >>> AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) >>> @@ -473,10 +473,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], >>> set bridge br0 datapath-type=dummy \ >>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-ofctl del-flows br0]) >>> >>> @@ -550,10 +550,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], >>> set bridge br0 datapath-type=dummy \ >>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg >>> netdev_dummy:file:dbg]) >>> >>> AT_CHECK([ovs-ofctl del-flows br0]) >>> >>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>> index e9110c1d8..b3ccdaa06 100644 >>> --- a/vswitchd/bridge.c >>> +++ b/vswitchd/bridge.c >>> @@ -1986,6 +1986,27 @@ port_is_bond_fake_iface(const struct port *port) >>> return port->cfg->bond_fake_iface && !ovs_list_is_short(&port->ifaces); >>> } >>> >>> +static bool >>> +flow_api_status_changed(void) >>> +{ >>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>> + static bool current_status = false; >>> + bool new_status; >>> + >>> + new_status = netdev_is_flow_api_enabled(); >>> + >>> + if (ovsthread_once_start(&once)) { >>> + current_status = new_status; >>> + ovsthread_once_done(&once); >>> + } >>> + >>> + if (current_status != new_status) { >>> + current_status = new_status; >>> + return true; >>> + } >>> + return false; >>> +} >>> + >>> static void >>> add_del_bridges(const struct ovsrec_open_vswitch *cfg) >>> { >>> @@ -2023,6 +2044,22 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg) >>> } >>> } >>> >>> + if (flow_api_status_changed()) { >>> + /* Destroy all the remaining bridges if Flow API status changed >>> + * as we need to re-probe supported features. Do not delete >>> + * bridge resources to avoid datapath disruption. */ >>> + HMAP_FOR_EACH_SAFE (br, node, &all_bridges) { >>> + /* FIXME: This has to be called with 'false' in order to not >>> + * destroy the datapath, but it requires re-working the >>> + * 'iface_hints' mechanism by supplying hints every time >>> + * ofproto is created, not only when the type is initialized. >>> + * Oterwise, ports will be destroyed anyway. For userspace >>> + * datapath that will also mean complete removal of a bridge >>> + * port without re-creation that we obviously do not want. */ >>> + bridge_destroy(br, true /* false! */); >>> + } >>> + } >>> + >>> /* Add new bridges. */ >>> SHASH_FOR_EACH(node, &new_br) { >>> const struct ovsrec_bridge *br_cfg = node->data; >>> -- >>> 2.41.0 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>
On 14 Dec 2023, at 19:15, Adrian Moreno wrote: > On 12/13/23 11:47, Adrian Moreno wrote: >> >> >> On 9/15/23 17:25, Eelco Chaudron wrote: >>> >>> >>> On 9 Sep 2023, at 1:40, Ilya Maximets wrote: >>> >>>> Currently OVS requires restart of ovs-vswitchd after enabling hardware >>>> offload. This is necessary to make sure all the correct features are >>>> probed and all the internal configurations are in the right state. >>>> >>>> Making that process a bit less invasive by re-creating bridges instead >>>> of a full process restart. Documentation is not updated, because >>>> disabling hardware offload still can take effect only after restart. >>>> >>> >>> Hi Ilya, >>> >>> I like the idea of re-creating the bridges, however not sure if it generated any side effects other than the ones you mention in the FIXME. >>> >> >> I have taken a look at the patch, tested it and added some rework of the iface_hints to avoid the datapath ports from being deleted when the bridge is re-created. >> >> The recreation of the bridge seems to work but my concern is that, unlike a restart which uses ovs-save to save & restore the flows, groups and tunnel metadata, doing this will leave the bridge with no OpenFlow configuration. >> >> During the time the controller takes to reconfigure the bridge, reval threads will wipe the datapath flows. If there is no controller ... well we'll need to wait for the user to program the flows back. This is quite a big behavior change so I'd say if we go for this option we better documment it pretty well. >> > > I'm thinking that, considering this hassle (loosing all flows, meters, tunnel tlv info, etc), isn't it better to simply restart vswitchd? > > A more more complex solution (probably big enough for this release which is coming fast) could be to recreate ovs-save internally and: > - Collect all the flow table, groups, tlv data and meters > - Delete the bridge and recreate it > - Reconfigure everything back > > Flow stats will probably be lost in the process, which is a weird outcome of enabling hwol but it's better than loosing all the flows. I guess the goal for this patch was to NOT touch any actual resources, only re-probe the capabilities. See comment below: + /* Destroy all the remaining bridges if Flow API status changed + * as we need to re-probe supported features. Do not delete + * bridge resources to avoid datapath disruption. */ Ilya can you comment on this? > +Mike > Any other ideas? > > >> Apart from that I see an error in the log says: >> >> 2023-12-13T10:43:44.791Z|00064|ofproto_dpif_rid|ERR|recirc_id 1 left allocated when ofproto (ktest) is destructed >> >> But I think (will double check) the recirc node gets cleaned afterwards. >> >> Cheers, >> Adrián >> >> >>> //Eelco >>> >>> >>>> [ >>>> This is not a full implementation, see the FIXME comment in the code. >>>> Posted in support for review of the OVS_ACTION_ATTR_DROP patch set. >>>> I can continue working on the proper implementation once I'm back >>>> from PTO. >>>> ] >>>> >>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >>>> --- >>>> tests/dpif-netdev.at | 6 +++--- >>>> vswitchd/bridge.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 40 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at >>>> index 85119fb81..a3e07895a 100644 >>>> --- a/tests/dpif-netdev.at >>>> +++ b/tests/dpif-netdev.at >>>> @@ -410,10 +410,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], >>>> set bridge br0 datapath-type=dummy \ >>>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-ofctl del-flows br0]) >>>> AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) >>>> @@ -473,10 +473,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], >>>> set bridge br0 datapath-type=dummy \ >>>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-ofctl del-flows br0]) >>>> >>>> @@ -550,10 +550,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], >>>> set bridge br0 datapath-type=dummy \ >>>> other-config:datapath-id=1234 fail-mode=secure], [], [], >>>> [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) >>>> - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) >>>> OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) >>>> + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) >>>> >>>> AT_CHECK([ovs-ofctl del-flows br0]) >>>> >>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c >>>> index e9110c1d8..b3ccdaa06 100644 >>>> --- a/vswitchd/bridge.c >>>> +++ b/vswitchd/bridge.c >>>> @@ -1986,6 +1986,27 @@ port_is_bond_fake_iface(const struct port *port) >>>> return port->cfg->bond_fake_iface && !ovs_list_is_short(&port->ifaces); >>>> } >>>> >>>> +static bool >>>> +flow_api_status_changed(void) >>>> +{ >>>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>>> + static bool current_status = false; >>>> + bool new_status; >>>> + >>>> + new_status = netdev_is_flow_api_enabled(); >>>> + >>>> + if (ovsthread_once_start(&once)) { >>>> + current_status = new_status; >>>> + ovsthread_once_done(&once); >>>> + } >>>> + >>>> + if (current_status != new_status) { >>>> + current_status = new_status; >>>> + return true; >>>> + } >>>> + return false; >>>> +} >>>> + >>>> static void >>>> add_del_bridges(const struct ovsrec_open_vswitch *cfg) >>>> { >>>> @@ -2023,6 +2044,22 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg) >>>> } >>>> } >>>> >>>> + if (flow_api_status_changed()) { >>>> + /* Destroy all the remaining bridges if Flow API status changed >>>> + * as we need to re-probe supported features. Do not delete >>>> + * bridge resources to avoid datapath disruption. */ >>>> + HMAP_FOR_EACH_SAFE (br, node, &all_bridges) { >>>> + /* FIXME: This has to be called with 'false' in order to not >>>> + * destroy the datapath, but it requires re-working the >>>> + * 'iface_hints' mechanism by supplying hints every time >>>> + * ofproto is created, not only when the type is initialized. >>>> + * Oterwise, ports will be destroyed anyway. For userspace >>>> + * datapath that will also mean complete removal of a bridge >>>> + * port without re-creation that we obviously do not want. */ >>>> + bridge_destroy(br, true /* false! */); >>>> + } >>>> + } >>>> + >>>> /* Add new bridges. */ >>>> SHASH_FOR_EACH(node, &new_br) { >>>> const struct ovsrec_bridge *br_cfg = node->data; >>>> -- >>>> 2.41.0 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> > > -- > Adrián Moreno
On 12/15/23 11:03, Eelco Chaudron wrote: > > > On 14 Dec 2023, at 19:15, Adrian Moreno wrote: > >> On 12/13/23 11:47, Adrian Moreno wrote: >>> >>> >>> On 9/15/23 17:25, Eelco Chaudron wrote: >>>> >>>> >>>> On 9 Sep 2023, at 1:40, Ilya Maximets wrote: >>>> >>>>> Currently OVS requires restart of ovs-vswitchd after enabling hardware >>>>> offload. This is necessary to make sure all the correct features are >>>>> probed and all the internal configurations are in the right state. >>>>> >>>>> Making that process a bit less invasive by re-creating bridges instead >>>>> of a full process restart. Documentation is not updated, because >>>>> disabling hardware offload still can take effect only after restart. >>>>> >>>> >>>> Hi Ilya, >>>> >>>> I like the idea of re-creating the bridges, however not sure if it generated any side effects other than the ones you mention in the FIXME. >>>> >>> >>> I have taken a look at the patch, tested it and added some rework of the iface_hints to avoid the datapath ports from being deleted when the bridge is re-created. >>> >>> The recreation of the bridge seems to work but my concern is that, unlike a restart which uses ovs-save to save & restore the flows, groups and tunnel metadata, doing this will leave the bridge with no OpenFlow configuration. >>> >>> During the time the controller takes to reconfigure the bridge, reval threads will wipe the datapath flows. If there is no controller ... well we'll need to wait for the user to program the flows back. This is quite a big behavior change so I'd say if we go for this option we better documment it pretty well. >>> >> >> I'm thinking that, considering this hassle (loosing all flows, meters, tunnel tlv info, etc), isn't it better to simply restart vswitchd? >> >> A more more complex solution (probably big enough for this release which is coming fast) could be to recreate ovs-save internally and: >> - Collect all the flow table, groups, tlv data and meters >> - Delete the bridge and recreate it >> - Reconfigure everything back >> >> Flow stats will probably be lost in the process, which is a weird outcome of enabling hwol but it's better than loosing all the flows. > > I guess the goal for this patch was to NOT touch any actual resources, only re-probe the capabilities. See comment below: > > + /* Destroy all the remaining bridges if Flow API status changed > + * as we need to re-probe supported features. Do not delete > + * bridge resources to avoid datapath disruption. */ > > Ilya can you comment on this? Summarizing the conversation we had with Ilya and adding a bit of context for the sake of completeness: When reviewing Eric's series to add OVS_ACTION_ATTR_DROP support for the kernel datapath. The problem comes when hw-offload is enabled since the tc datapath does not support this, we need a way to re-evaluate the feature and stop adding that action. An approach was proposed (suggested by Eelco) in version 6 [1] of that series: manually reprobing that feature bit. Ilya raised concerns about thread-safety and maintainability and proposed exploring this alternative idea. Given the consequences of this approach (loosing all the OFP flows), it doesn't seem as a good way to go, and we don't have any other good idea to do this easily. Therefore, the way to move forward would be to add atomicity to Eric's V6 version. Note that maintainability is still a concern and re-probing independent features should not be the general case. Also, we need to make sure reprobing does not add dp flows that affect ongoing traffic. [1] https://patchwork.ozlabs.org/project/openvswitch/patch/20230901194234.3409915-3-eric@garver.life/
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at index 85119fb81..a3e07895a 100644 --- a/tests/dpif-netdev.at +++ b/tests/dpif-netdev.at @@ -410,10 +410,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD], set bridge br0 datapath-type=dummy \ other-config:datapath-id=1234 fail-mode=secure], [], [], [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-ofctl del-flows br0]) AT_CHECK([ovs-ofctl add-flow br0 in_port=1,actions=IN_PORT]) @@ -473,10 +473,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS], set bridge br0 datapath-type=dummy \ other-config:datapath-id=1234 fail-mode=secure], [], [], [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-ofctl del-flows br0]) @@ -550,10 +550,10 @@ m4_define([DPIF_NETDEV_FLOW_HW_OFFLOAD_OFFSETS_VID_ARP], set bridge br0 datapath-type=dummy \ other-config:datapath-id=1234 fail-mode=secure], [], [], [m4_if([$1], [dummy-pmd], [--dummy-numa="0,0,0,0,1,1,1,1"], [])]) - AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-vsctl set Open_vSwitch . other_config:hw-offload=true]) OVS_WAIT_UNTIL([grep "netdev: Flow API Enabled" ovs-vswitchd.log]) + AT_CHECK([ovs-appctl vlog/set dpif:file:dbg dpif_netdev:file:dbg netdev_dummy:file:dbg]) AT_CHECK([ovs-ofctl del-flows br0]) diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index e9110c1d8..b3ccdaa06 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -1986,6 +1986,27 @@ port_is_bond_fake_iface(const struct port *port) return port->cfg->bond_fake_iface && !ovs_list_is_short(&port->ifaces); } +static bool +flow_api_status_changed(void) +{ + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + static bool current_status = false; + bool new_status; + + new_status = netdev_is_flow_api_enabled(); + + if (ovsthread_once_start(&once)) { + current_status = new_status; + ovsthread_once_done(&once); + } + + if (current_status != new_status) { + current_status = new_status; + return true; + } + return false; +} + static void add_del_bridges(const struct ovsrec_open_vswitch *cfg) { @@ -2023,6 +2044,22 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg) } } + if (flow_api_status_changed()) { + /* Destroy all the remaining bridges if Flow API status changed + * as we need to re-probe supported features. Do not delete + * bridge resources to avoid datapath disruption. */ + HMAP_FOR_EACH_SAFE (br, node, &all_bridges) { + /* FIXME: This has to be called with 'false' in order to not + * destroy the datapath, but it requires re-working the + * 'iface_hints' mechanism by supplying hints every time + * ofproto is created, not only when the type is initialized. + * Oterwise, ports will be destroyed anyway. For userspace + * datapath that will also mean complete removal of a bridge + * port without re-creation that we obviously do not want. */ + bridge_destroy(br, true /* false! */); + } + } + /* Add new bridges. */ SHASH_FOR_EACH(node, &new_br) { const struct ovsrec_bridge *br_cfg = node->data;
Currently OVS requires restart of ovs-vswitchd after enabling hardware offload. This is necessary to make sure all the correct features are probed and all the internal configurations are in the right state. Making that process a bit less invasive by re-creating bridges instead of a full process restart. Documentation is not updated, because disabling hardware offload still can take effect only after restart. [ This is not a full implementation, see the FIXME comment in the code. Posted in support for review of the OVS_ACTION_ATTR_DROP patch set. I can continue working on the proper implementation once I'm back from PTO. ] Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- tests/dpif-netdev.at | 6 +++--- vswitchd/bridge.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)