On Fri, Sep 15, 2023 at 12:02:13PM +0900, Nobuhiro MIKI wrote:
> A conjunctive flow consists of two or more multiple flows with
> conjunction actions. When input to the ofproto/trace command
> matches a conjunctive flow, it outputs flows of all dimensions.
>
> Signed-off-by: Nobuhiro MIKI <[email protected]>
Hi Miki-san,
some feedback from my side is provided inline.
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 18dbfc83ad44..e43bd6e39d80 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -853,6 +853,20 @@ trie_ctx_init(struct trie_ctx *ctx, const struct
> cls_trie *trie)
> ctx->lookup_done = false;
> }
>
> +static void
> +append_conj_flows(struct ovs_list *conj_flows,
> + struct cls_conjunction_set **soft, size_t n_soft)
> +{
> + struct flow *f;
> + int i;
I wonder if it is nicer to return here if conj_flows is NULL,
avoiding the need for callers to check this condition.
> +
> + for (i = 0; i < n_soft; i++) {
> + f = xmalloc(sizeof *f);
> + miniflow_expand(&soft[i]->match->flow, f);
> + ovs_list_push_back(conj_flows, &f->list_node);
> + }
> +}
> +
> struct conjunctive_match {
> struct hmap_node hmap_node;
> uint32_t id;
...
> diff --git a/lib/classifier.h b/lib/classifier.h
> index f646a8f7429b..d398f42ae310 100644
> --- a/lib/classifier.h
> +++ b/lib/classifier.h
> @@ -299,6 +299,7 @@
> * parallel to the rule's removal. */
>
> #include "cmap.h"
> +#include "openvswitch/list.h"
> #include "openvswitch/match.h"
> #include "openvswitch/meta-flow.h"
> #include "pvector.h"
> @@ -398,7 +399,8 @@ static inline void classifier_publish(struct classifier
> *);
> * and each other. */
> const struct cls_rule *classifier_lookup(const struct classifier *,
> ovs_version_t, struct flow *,
> - struct flow_wildcards *);
> + struct flow_wildcards *,
> + struct ovs_list *conj_flows);
Perhaps for consistency perhaps this should be written as:
const struct cls_rule *classifier_lookup(const struct classifier *,
ovs_version_t, struct flow *,
struct flow_wildcards *,
struct ovs_list *);
> 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/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9cdddb1ff8cb..11bf48adbf09 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -866,6 +866,33 @@ xlate_report_action_set(const struct xlate_ctx *ctx,
> const char *verb)
> }
> }
>
> +static void
> +xlate_report_conj_matches(const struct xlate_ctx *ctx)
> +{
> + struct match match;
> + struct flow *f;
> + int count;
> + int i = 0;
> +
> + count = ovs_list_size(&ctx->xout->conj_flows);
> +
> + LIST_FOR_EACH (f, list_node, &ctx->xout->conj_flows) {
> + match_init(&match, f, ctx->xin->wc);
> +
> + /* Hide unnecessary fields. */
> + match.wc.masks.conj_id = 0;
> + match.wc.masks.recirc_id = 0;
> + match.wc.masks.in_port.ofp_port = 0;
If I understand things, this effectively alters ctx->xin->wc.
Is it safe to do so without subsequently restoring the original values?
> +
> + struct ds s = DS_EMPTY_INITIALIZER;
> +
> + match_format(&match, NULL, &s, OFP_DEFAULT_PRIORITY);
> + xlate_report_debug(ctx, OFT_DETAIL, "conj(%d/%d). %s",
> + ++i, count, ds_cstr(&s));
> +
> + ds_destroy(&s);
> + }
> +}
>
> /* If tracing is enabled in 'ctx', appends a node representing 'rule' (in
> * OpenFlow table 'table_id') to the trace and makes this node the parent for
...
> @@ -7967,10 +7997,9 @@ xlate_optimize_odp_actions(struct xlate_in *xin)
> enum xlate_error
> xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> {
> - *xout = (struct xlate_out) {
> - .slow = 0,
> - .recircs = RECIRC_REFS_EMPTY_INITIALIZER,
> - };
I think that the above had the side effect of zeroing all other fields
of xout. If so, it seems that the avoid_caching field is no longer
initialised. Is that ok?
> + xout->slow = 0;
> + xout->recircs = RECIRC_REFS_EMPTY_INITIALIZER;
> + ovs_list_init(&xout->conj_flows);
>
> struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> struct xbridge *xbridge = xbridge_lookup(xcfg, xin->ofproto);
...
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d8e0cd37ac5b..32e9048e7167 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -103,7 +103,8 @@ struct rule_dpif *rule_dpif_lookup_from_table(struct
> ofproto_dpif *,
> ofp_port_t in_port,
> bool may_packet_in,
> bool honor_table_miss,
> - struct xlate_cache *);
> + struct xlate_cache *,
> + struct ovs_list *conj_flows);
Perhaps for consistency this should be written as: struct ovs_list *);
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
