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.maxim...@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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to