On 11 May 2025, at 15:47, Roi Dayan via dev wrote:
> 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?
Hi Roi,
It’s on Ilya’s queue, see here;
https://patchwork.ozlabs.org/project/openvswitch/list/
>> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
