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