On 21 May 2025, at 6:46, 이병건 wrote:
> The current ovs-vswitchd binary implementation clears up kernel datapath
> whenever the user types a command: 'ovs-vsctl del-br {br}'. The culprit is
> 'ofproto->ofproto_class->flush(ofproto)' in 'ofproto_flush__', which is
> called every time for bridge delete. It clears up all the kernel datapath's
> entries, resulting in a latency spike.
>
> In the multi-tenant environment, each bridge represents an individual tenant.
> However, when a specific tenant requests a bridge deletion, it affects all
> the other tenants due to the lack of isolation of the datapath flush. As a
> result, the flush clears up all datapaths, causing packet upcalls and
> rebuilding the datapath that were not targeted for deletion.
>
> The code below suggests the functionality that bypasses
> 'ofproto->ofproto_class->flush(ofproto)' operation. This is unnecessary
> since the port delete operation already removes corresponding datapaths
> selectively, and further, the datapath resident time is relatively short.
> This behavior is choosable by toggling the other-config option.
>
> related-mail: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046818.html
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046818.html
>
> Any feedback would be sincerely appreciated.
Hi,
Thanks for submitting a patch!
However, it looks like the patch is not correctly formatted. Specifically,
patches should be sent as inline plain text, not as attachments or HTML. This
ensures they can be properly reviewed, applied, and tracked by the mailing list
tools and other contributors.
Please review your email client settings to ensure it's sending plain text.
Alternatively, you can use git send-email, which is the recommended way to
submit patches to the OVS mailing list. You can find more details in the OVS
contributing guide:
https://docs.openvswitch.org/en/latest/internals/contributing/submitting-patches/
In addition, please run the checkpatch.pl script (found in the utilities
directory of the OVS tree) before resubmitting. This tool helps catch common
formatting issues.
Looking forward to your revised patch!
Cheers,
Eelco
>
> diff --git a/ovs/ofproto/ofproto.c b/ovs/ofproto/ofproto.c
> index ef615e5..3be7571 100644
> --- a/ovs/ofproto/ofproto.c
> +++ b/ovs/ofproto/ofproto.c
> @@ -315,6 +315,7 @@ unsigned ofproto_offloaded_stats_delay =
> OFPROTO_OFFLOADED_STATS_DELAY;
> bool ofproto_explicit_sampled_drops =
> OFPROTO_EXPLICIT_SAMPLED_DROPS_DEFAULT;
>
> uint32_t n_handlers, n_revalidators;
> +bool no_flush_on_bridge_delete;
>
> /* Map from datapath name to struct ofproto, for use by unixctl
> commands. */
> static struct hmap all_ofprotos = HMAP_INITIALIZER(&all_ofprotos);
> @@ -821,6 +822,12 @@ ofproto_set_threads(int n_handlers_, int n_revalidators_)
> n_handlers = MAX(n_handlers_, 0);
> }
>
> +void
> +ofproto_set_no_flush_on_bridge_delete(bool no_flush)
> +{
> + no_flush_on_bridge_delete = no_flush;
> +}
> +
> void
> ofproto_set_dp_desc(struct ofproto *p, const char *dp_desc)
> {
> @@ -1680,13 +1687,13 @@ ofproto_rule_delete(struct ofproto *ofproto, struct
> rule *rule)
> }
>
> static void
> -ofproto_flush__(struct ofproto *ofproto, bool del)
> +ofproto_flush__(struct ofproto *ofproto, bool del, bool force_flush)
> OVS_EXCLUDED(ofproto_mutex)
> {
> struct oftable *table;
>
> /* This will flush all datapath flows. */
> - if (del && ofproto->ofproto_class->flush) {
> + if (del && ofproto->ofproto_class->flush
> && (force_flush || !no_flush_on_bridge_delete)) {
>
> ofproto->ofproto_class->flush(ofproto);
> }
>
> @@ -1829,7 +1836,7 @@ ofproto_unref(struct ofproto *ofproto)
> }
>
> void
> -ofproto_destroy(struct ofproto *p, bool del)
> +ofproto_destroy(struct ofproto *p, bool del, bool force_flush)
> OVS_EXCLUDED(ofproto_mutex)
> {
> struct ofport *ofport;
> @@ -1839,7 +1846,7 @@ ofproto_destroy(struct ofproto *p, bool del)
> return;
> }
>
> - ofproto_flush__(p, del);
> + ofproto_flush__(p, del, force_flush);
> HMAP_FOR_EACH_SAFE (ofport, hmap_node, &p->ports) {
> ofport_destroy(ofport, del);
> }
> @@ -2422,7 +2429,7 @@ void
> ofproto_flush_flows(struct ofproto *ofproto)
> {
> COVERAGE_INC(ofproto_flush);
> - ofproto_flush__(ofproto, false);
> + ofproto_flush__(ofproto, false, false);
> connmgr_flushed(ofproto->connmgr);
> }
>
>
> diff --git a/ovs/ofproto/ofproto.h b/ovs/ofproto/ofproto.h
> index 3f85509..96148b5 100644
> --- a/ovs/ofproto/ofproto.h
> +++ b/ovs/ofproto/ofproto.h
> @@ -277,7 +277,7 @@ void ofproto_type_wait(const char *datapath_type);
> int ofproto_create(const char *datapath, const char *datapath_type,
> struct
> ofproto **ofprotop)
> OVS_EXCLUDED(ofproto_mutex);
> -void ofproto_destroy(struct ofproto *, bool del);
> +void ofproto_destroy(struct ofproto *, bool del, bool force_flush);
> int ofproto_delete(const char *name, const char *type);
>
> int ofproto_run(struct ofproto *);
> @@ -365,6 +365,7 @@ int ofproto_set_mcast_snooping(struct ofproto *ofproto,
> int ofproto_port_set_mcast_snooping(struct ofproto *ofproto, void *aux,
>
> const struct ofproto_mcast_snooping_port_settings *s);
> void ofproto_set_threads(int n_handlers, int n_revalidators);
> +void ofproto_set_no_flush_on_bridge_delete(bool no_flush);
> void ofproto_type_set_config(const char *type,
>
> const struct smap *other_config);
> void ofproto_set_dp_desc(struct ofproto *, const char *dp_desc);
> diff --git a/ovs/vswitchd/bridge.c b/ovs/vswitchd/bridge.c
> index 456b784..1b96a93 100644
> --- a/ovs/vswitchd/bridge.c
> +++ b/ovs/vswitchd/bridge.c
> @@ -266,7 +266,7 @@ static uint64_t last_ifaces_changed;
> static void add_del_bridges(const struct ovsrec_open_vswitch *);
> static void bridge_run__(void);
> static void bridge_create(const struct ovsrec_bridge *);
> -static void bridge_destroy(struct bridge *, bool del);
> +static void bridge_destroy(struct bridge *, bool del, bool force_flush);
> static struct bridge *bridge_lookup(const char *name);
> static unixctl_cb_func bridge_unixctl_dump_flows;
> static unixctl_cb_func bridge_unixctl_reconnect;
> @@ -554,7 +554,7 @@ bridge_exit(bool delete_datapath)
>
> struct bridge *br;
> HMAP_FOR_EACH_SAFE (br, node, &all_bridges) {
> - bridge_destroy(br, delete_datapath);
> + bridge_destroy(br, delete_datapath, true);
> }
>
> ovsdb_idl_destroy(idl);
> @@ -890,6 +890,9 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> smap_get_int(&ovs_cfg->other_config,
> "n-handler-threads", 0),
> smap_get_int(&ovs_cfg->other_config,
> "n-revalidator-threads", 0));
>
> + ofproto_set_no_flush_on_bridge_delete(
> + smap_get_bool(&ovs_cfg->other_config,
> "no-flush-on-bridge-delete", false));
> +
> ofproto_set_explicit_sampled_drops(
>
> smap_get_bool(&ovs_cfg->other_config, "explicit-sampled-drops",
>
> OFPROTO_EXPLICIT_SAMPLED_DROPS_DEFAULT));
> @@ -938,7 +941,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
>
> VLOG_ERR("failed to create bridge %s: %s", br->name,
>
> ovs_strerror(error));
>
> shash_destroy(&br->wanted_ports);
> - bridge_destroy(br,
> true);
> + bridge_destroy(br,
> true, true);
> } else {
> /* Trigger
> storing datapath version. */
>
> seq_change(connectivity_seq_get());
> @@ -2137,7 +2140,7 @@ add_del_bridges(const struct ovsrec_open_vswitch *cfg)
> br->cfg = shash_find_data(&new_br,
> br->name);
> if (!br->cfg || strcmp(br->type,
> ofproto_normalize_type(
>
>
> br->cfg->datapath_type))) {
> - bridge_destroy(br, true);
> + bridge_destroy(br, true, false);
> }
> }
>
> @@ -3381,7 +3384,7 @@ bridge_run(void)
>
> (long int) getpid());
>
> HMAP_FOR_EACH_SAFE (br, node,
> &all_bridges) {
> - bridge_destroy(br, false);
> + bridge_destroy(br, false, false);
> }
> /* Since we will not be running
> system_stats_run() in this process
> * with the current situation of multiple
> ovs-vswitchd daemons,
> @@ -3700,7 +3703,7 @@ bridge_create(const struct ovsrec_bridge *br_cfg)
> }
>
> static void
> -bridge_destroy(struct bridge *br, bool del)
> +bridge_destroy(struct bridge *br, bool del, bool force_flush)
> {
> if (br) {
> struct mirror *mirror;
> @@ -3714,7 +3717,7 @@ bridge_destroy(struct bridge *br, bool del)
> }
>
> hmap_remove(&all_bridges,
> &br->node);
> - ofproto_destroy(br->ofproto, del);
> + ofproto_destroy(br->ofproto, del,
> force_flush);
> hmap_destroy(&br->ifaces);
> hmap_destroy(&br->ports);
> hmap_destroy(&br->iface_by_name);
> diff --git a/ovs/vswitchd/vswitch.xml b/ovs/vswitchd/vswitch.xml
> index 76df9aa..1d25b3e 100644
> --- a/ovs/vswitchd/vswitch.xml
> +++ b/ovs/vswitchd/vswitch.xml
> @@ -662,6 +662,24 @@
> </p>
> </column>
>
> + <column name="other_config"
> key="no-flush-on-bridege-delete"
> + type='{"type":
> "boolean"}'>
> + <p>
> + Set this value to
> <code>true</code> to keep the datapath from being flushed
> + whenever a bridge is deleted.
> + </p>
> + <p>
> + The default value is
> <code>false</code>
> + </p>
> + <p>
> + If this configuration is
> <code>true</code>, it protects the datapath
> + which resides in kernel when an arbitrary
> bridge is deleted.
> + Otherwise, bridge delete operation causes
> datapath flush, requiring
> + rebuilding the kernel datapath. The
> default value is <code>false</code> as it is
> + the default behavior until now.
> + </p>
> + </column>
> +
> <column name="other_config"
> key="emc-insert-inv-prob"
> type='{"type":
> "integer", "minInteger": 0, "maxInteger": 4294967295}'>
> <p>
>
> _______________________________________________
> 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
