On 2023/09/20 20:06, Simon Horman wrote:
> 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.
>
Hi Simon-san,
Thanks for the review.
>> 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.
>
I agree, I'll fix it.
>> +
>> + 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 *);
>
As commented in Ilya's email, I will leave the name
'conj_flows' here for descriptive purposes.
>> 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?
>
In struct match, wc is stored as a value, not a pointer.
Also, match_init() appears to copy the value of wc with
const qualifier. So I think we can change match.wc.masks
here without needing to alter ctx->xin->wc.
Please correct me if I've misunderstood something.
>> +
>> + 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?
>
This variable is not initialized anywhere.
It could potentially lead to a bug, so initialize it here.
>> + 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 *);
>
> ...
Here as well. I will leave the name 'conj_flows' here for descriptive purposes.
Best Regards,
Nobuhiro Miki
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
