On 27/04/2025 10:51, Roi Dayan wrote:
> From: Eli Britstein <el...@nvidia.com>
>
> According to [1], transport port masks are optimized using a prefix tree,
> to get less specific datapath flows. However, this might be not optimal
> for some cases. HW offload that is based on templates and has a table per
> template (all the fields that are matched) will create many such tables
> and yield poor performance. Furthermore, such templates are limited resources.
>
> In such cases, it is preferred to have more specific datapath flows that
> share the same match template rather then less flows distributed across
> multiple templates.
>
> To achieve that, introduce other_config:disable-ports-trie (true/false)
> configuration (default as false).
>
> [1] 69d6040e861a ("lib/classifier: Use a prefix tree to optimize ports
> wildcarding.")
>
> Acked-by: Roi Dayan <r...@nvidia.com>
> Signed-off-by: Eli Britstein <el...@nvidia.com>
> ---
Hi,
Any feedkback about this optional config?
Thanks,
Roi
> lib/classifier.c | 21 +++++++++++++--------
> lib/classifier.h | 3 ++-
> lib/ovs-router.c | 4 ++--
> lib/tnl-ports.c | 7 ++++---
> ofproto/ofproto-dpif.c | 9 ++++++---
> ofproto/ofproto-provider.h | 2 ++
> ofproto/ofproto.c | 5 ++++-
> ofproto/ofproto.h | 2 +-
> tests/classifier.at | 25 +++++++++++++++++++++++++
> tests/test-classifier.c | 8 ++++----
> vswitchd/bridge.c | 3 ++-
> vswitchd/vswitch.xml | 18 ++++++++++++++++++
> 12 files changed, 83 insertions(+), 24 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 55f23b976f30..37684a5aa026 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -116,7 +116,8 @@ static const struct cls_match *find_match_wc(const struct
> cls_subtable *,
> const struct flow *,
> struct trie_ctx *,
> unsigned int n_tries,
> - struct flow_wildcards *);
> + struct flow_wildcards *,
> + bool disable_ports_trie);
> static struct cls_match *find_equal(const struct cls_subtable *,
> const struct miniflow *, uint32_t hash);
>
> @@ -971,7 +972,8 @@ static const struct cls_rule *
> classifier_lookup__(const struct classifier *cls, ovs_version_t version,
> struct flow *flow, struct flow_wildcards *wc,
> bool allow_conjunctive_matches,
> - struct hmapx *conj_flows)
> + struct hmapx *conj_flows,
> + bool disable_ports_trie)
> {
> struct trie_ctx trie_ctx[CLS_MAX_TRIES];
> const struct cls_match *match;
> @@ -1007,7 +1009,7 @@ classifier_lookup__(const struct classifier *cls,
> ovs_version_t version,
> /* Skip subtables with no match, or where the match is lower-priority
> * than some certain match we've already found. */
> match = find_match_wc(subtable, version, flow, trie_ctx,
> cls->n_tries,
> - wc);
> + wc, disable_ports_trie);
> if (!match || match->priority <= hard_pri) {
> continue;
> }
> @@ -1132,7 +1134,7 @@ classifier_lookup__(const struct classifier *cls,
> ovs_version_t version,
>
> flow->conj_id = id;
> rule = classifier_lookup__(cls, version, flow, wc, false,
> - NULL);
> + NULL, disable_ports_trie);
> flow->conj_id = saved_conj_id;
>
> if (rule) {
> @@ -1207,9 +1209,11 @@ classifier_lookup__(const struct classifier *cls,
> ovs_version_t version,
> const struct cls_rule *
> classifier_lookup(const struct classifier *cls, ovs_version_t version,
> struct flow *flow, struct flow_wildcards *wc,
> - struct hmapx *conj_flows)
> + struct hmapx *conj_flows,
> + bool disable_ports_trie)
> {
> - return classifier_lookup__(cls, version, flow, wc, true, conj_flows);
> + return classifier_lookup__(cls, version, flow, wc, true, conj_flows,
> + disable_ports_trie);
> }
>
> /* Finds and returns a rule in 'cls' with exactly the same priority and
> @@ -1727,7 +1731,8 @@ find_match(const struct cls_subtable *subtable,
> ovs_version_t version,
> static const struct cls_match *
> find_match_wc(const struct cls_subtable *subtable, ovs_version_t version,
> const struct flow *flow, struct trie_ctx *trie_ctx,
> - unsigned int n_tries, struct flow_wildcards *wc)
> + unsigned int n_tries, struct flow_wildcards *wc,
> + bool disable_ports_trie)
> {
> if (OVS_UNLIKELY(!wc)) {
> return find_match(subtable, version, flow,
> @@ -1774,7 +1779,7 @@ find_match_wc(const struct cls_subtable *subtable,
> ovs_version_t version,
> subtable->index_maps[i],
> &mask_offset, &basis);
> rule = find_match(subtable, version, flow, hash);
> - if (!rule && subtable->ports_mask_len) {
> + if (!rule && subtable->ports_mask_len && !disable_ports_trie) {
> /* The final stage had ports, but there was no match. Instead of
> * unwildcarding all the ports bits, use the ports trie to figure
> out a
> * smaller set of bits to unwildcard. */
> diff --git a/lib/classifier.h b/lib/classifier.h
> index 7928601e0f59..38eb92f45267 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -399,7 +399,8 @@ static inline void classifier_publish(struct classifier
> *);
> const struct cls_rule *classifier_lookup(const struct classifier *,
> ovs_version_t, struct flow *,
> struct flow_wildcards *,
> - struct hmapx *conj_flows);
> + struct hmapx *conj_flows,
> + bool disable_ports_trie);
> bool classifier_rule_overlaps(const struct classifier *,
> const struct cls_rule *, ovs_version_t);
> const struct cls_rule *classifier_find_rule_exactly(const struct classifier
> *,
> diff --git a/lib/ovs-router.c b/lib/ovs-router.c
> index d955a3a543b8..81fe3382a287 100644
> --- a/lib/ovs-router.c
> +++ b/lib/ovs-router.c
> @@ -117,7 +117,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> struct flow flow_src = {.ipv6_dst = *src, .pkt_mark = mark};
>
> cr_src = classifier_lookup(&cls, OVS_VERSION_MAX, &flow_src, NULL,
> - NULL);
> + NULL, false);
> if (cr_src) {
> struct ovs_router_entry *p_src = ovs_router_entry_cast(cr_src);
> if (!p_src->local) {
> @@ -128,7 +128,7 @@ ovs_router_lookup(uint32_t mark, const struct in6_addr
> *ip6_dst,
> }
> }
>
> - cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL);
> + cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL, false);
> if (cr) {
> struct ovs_router_entry *p = ovs_router_entry_cast(cr);
>
> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
> index 56119b30010e..3b3545a11194 100644
> --- a/lib/tnl-ports.c
> +++ b/lib/tnl-ports.c
> @@ -112,7 +112,8 @@ map_insert(odp_port_t port, struct eth_addr mac, struct
> in6_addr *addr,
> tnl_port_init_flow(&match.flow, mac, addr, nw_proto, tp_port);
>
> do {
> - cr = classifier_lookup(&cls, OVS_VERSION_MAX, &match.flow, NULL,
> NULL);
> + cr = classifier_lookup(&cls, OVS_VERSION_MAX, &match.flow, NULL,
> NULL,
> + false);
> p = tnl_port_cast(cr);
> /* Try again if the rule was released before we get the reference. */
> } while (p && !ovs_refcount_try_ref_rcu(&p->ref_cnt));
> @@ -245,7 +246,7 @@ map_delete(struct eth_addr mac, struct in6_addr *addr,
>
> tnl_port_init_flow(&flow, mac, addr, nw_proto, tp_port);
>
> - cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL);
> + cr = classifier_lookup(&cls, OVS_VERSION_MAX, &flow, NULL, NULL, false);
> tnl_port_unref(cr);
> }
>
> @@ -303,7 +304,7 @@ odp_port_t
> tnl_port_map_lookup(struct flow *flow, struct flow_wildcards *wc)
> {
> const struct cls_rule *cr = classifier_lookup(&cls, OVS_VERSION_MAX,
> flow,
> - wc, NULL);
> + wc, NULL, false);
>
> return (cr) ? tnl_port_cast(cr)->portno : ODPP_NONE;
> }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ed9e44ce2b03..5b9b1e70ba92 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4587,9 +4587,12 @@ rule_dpif_lookup_in_table(struct ofproto_dpif
> *ofproto, ovs_version_t version,
> struct hmapx *conj_flows)
> {
> struct classifier *cls = &ofproto->up.tables[table_id].cls;
> - return rule_dpif_cast(rule_from_cls_rule(classifier_lookup(cls, version,
> - flow, wc,
> - conj_flows)));
> + bool disable_ports_trie = ofproto->up.disable_ports_trie;
> + const struct cls_rule *cr;
> +
> + cr = classifier_lookup(cls, version, flow, wc, conj_flows,
> + disable_ports_trie);
> + return rule_dpif_cast(rule_from_cls_rule(cr));
> }
>
> void
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 7df3f5246912..4527100c2d65 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -146,6 +146,8 @@ struct ofproto {
> struct vl_mff_map vl_mff_map;
> /* refcount to this ofproto, held by rule/group/xlate_caches */
> struct ovs_refcount refcount;
> +
> + bool disable_ports_trie;
> };
>
> void ofproto_init_tables(struct ofproto *, int n_tables);
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ef615e59c354..8bd47c68c16b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -481,7 +481,7 @@ ofproto_bump_tables_version(struct ofproto *ofproto)
>
> int
> ofproto_create(const char *datapath_name, const char *datapath_type,
> - struct ofproto **ofprotop)
> + const struct smap *other_config, struct ofproto **ofprotop)
> OVS_EXCLUDED(ofproto_mutex)
> {
> const struct ofproto_class *class;
> @@ -589,6 +589,9 @@ ofproto_create(const char *datapath_name, const char
> *datapath_type,
> ofproto->slowpath_meter_id = UINT32_MAX;
> ofproto->controller_meter_id = UINT32_MAX;
>
> + ofproto->disable_ports_trie = smap_get_bool(other_config,
> + "disable-ports-trie", false);
> +
> /* Set the initial tables version. */
> ofproto_bump_tables_version(ofproto);
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 3f85509a1add..ea0be0a4a19c 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -275,7 +275,7 @@ int ofproto_type_run(const char *datapath_type);
> void ofproto_type_wait(const char *datapath_type);
>
> int ofproto_create(const char *datapath, const char *datapath_type,
> - struct ofproto **ofprotop)
> + const struct smap *other_config, struct ofproto
> **ofprotop)
> OVS_EXCLUDED(ofproto_mutex);
> void ofproto_destroy(struct ofproto *, bool del);
> int ofproto_delete(const char *name, const char *type);
> diff --git a/tests/classifier.at b/tests/classifier.at
> index dfadf5e5a9ce..b83b76b3eacf 100644
> --- a/tests/classifier.at
> +++ b/tests/classifier.at
> @@ -282,6 +282,31 @@ Datapath actions: 3
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +AT_SETUP([flow classifier - disable ports trie])
> +OVS_VSWITCHD_START([set Open_vSwitch . other_config:disable-ports-trie=true])
> +add_of_ports br0 1 2
> +
> +AT_DATA([flows.txt], [dnl
> + priority=1,ip,tcp,tp_dst=80 action=drop
> + priority=0 actions=2
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0
> 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=10.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=80'],
> [0], [stdout])
> +AT_CHECK([tail -2 stdout], [0],
> + [Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_frag=no,tp_dst=80
> +Datapath actions: drop
> +])
> +
> +AT_CHECK([ovs-appctl ofproto/trace br0
> 'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,dl_type=0x0800,nw_src=192.168.0.1,nw_dst=10.1.2.15,nw_proto=6,nw_tos=0,nw_ttl=128,tp_src=8,tp_dst=79'],
> [0], [stdout])
> +AT_CHECK([tail -2 stdout], [0],
> + [Megaflow: recirc_id=0,eth,tcp,in_port=1,nw_frag=no,tp_dst=79
> +Datapath actions: 2
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> AT_SETUP([flow classifier - ipv6 ND dependency])
> OVS_VSWITCHD_START
> add_of_ports br0 1 2
> diff --git a/tests/test-classifier.c b/tests/test-classifier.c
> index 2c1604a01e2e..445b6206697f 100644
> --- a/tests/test-classifier.c
> +++ b/tests/test-classifier.c
> @@ -441,7 +441,7 @@ compare_classifiers(struct classifier *cls, size_t
> n_invisible_rules,
> /* This assertion is here to suppress a GCC 4.9 array-bounds warning
> */
> ovs_assert(cls->n_tries <= CLS_MAX_TRIES);
>
> - cr0 = classifier_lookup(cls, version, &flow, &wc, NULL);
> + cr0 = classifier_lookup(cls, version, &flow, &wc, NULL, false);
> cr1 = tcls_lookup(tcls, &flow);
> assert((cr0 == NULL) == (cr1 == NULL));
> if (cr0 != NULL) {
> @@ -454,7 +454,7 @@ compare_classifiers(struct classifier *cls, size_t
> n_invisible_rules,
> /* Make sure the rule should have been visible. */
> assert(cls_rule_visible_in_version(cr0, version));
> }
> - cr2 = classifier_lookup(cls, version, &flow, NULL, NULL);
> + cr2 = classifier_lookup(cls, version, &flow, NULL, NULL, false);
> assert(cr2 == cr0);
> }
> }
> @@ -1370,10 +1370,10 @@ lookup_classifier(void *aux_)
> if (aux->use_wc) {
> flow_wildcards_init_catchall(&wc);
> cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x],
> - &wc, NULL);
> + &wc, NULL, false);
> } else {
> cr = classifier_lookup(aux->cls, version, &aux->lookup_flows[x],
> - NULL, NULL);
> + NULL, NULL, false);
> }
> if (cr) {
> hits++;
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 456b784c01d0..1a2236c77719 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -933,7 +933,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch
> *ovs_cfg)
> if (!br->ofproto) {
> int error;
>
> - error = ofproto_create(br->name, br->type, &br->ofproto);
> + error = ofproto_create(br->name, br->type,
> &ovs_cfg->other_config,
> + &br->ofproto);
> if (error) {
> VLOG_ERR("failed to create bridge %s: %s", br->name,
> ovs_strerror(error));
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 76df9aab055a..737e4c07adf5 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -176,6 +176,24 @@
> </p>
> </column>
>
> + <column name="other_config" key="disable-ports-trie"
> + type='{"type": "boolean"}'>
> + <p>
> + Set this value to <code>true</code> to disable transport ports
> prefix
> + tree optimization.
> + </p>
> + <p>
> + The default value is <code>false</code>. Changing this value
> requires
> + restarting the daemon.
> + </p>
> + <p>
> + The default behavior tries to optimize transport ports wildcards,
> + to get less datapath flows. The down side of it that those flows
> + are distributed accross multiple match templates, which is
> + problematic for HW offloads.
> + </p>
> + </column>
> +
> <column name="other_config" key="max-idle"
> type='{"type": "integer", "minInteger": 500}'>
> <p>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
