On 2023/10/02 20:41, Simon Horman wrote: > + Mike Pattrick <mpatt...@redhat.com> > > On Mon, Sep 25, 2023 at 06:09:00PM +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 <nm...@yahoo-corp.jp> >> --- >> v3: >> * Remove struct flow changes. >> * Use struct 'cls_rule' instead of struct 'flow'. >> * Add priority and id conditionals for 'soft' arrays. >> * Use 'minimask' in struct 'cls_rule' as mask. > > Hi Miki-san, >
Hi Simon-san, Thanks for your review. > I am not seeing minimask used in this patch. Sorry, it was a mistake. By the way, the test results seem to indicate that the mask is working correctly in the trace output. > >> * Use hmapx instead of ovs_list to store conj flows. >> * Passe 'conj_flows' as an argument only when tracing. > > Other than the minimask question above, it seems to me that > Ilya's review of v2 has been addressed. > > ... > >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > ... > >> @@ -918,6 +941,8 @@ xlate_report_table(const struct xlate_ctx *ctx, struct >> rule_dpif *rule, >> ctx->xin->trace = &oftrace_report(ctx->xin->trace, OFT_TABLE, >> ds_cstr(&s))->subs; >> ds_destroy(&s); >> + >> + xlate_report_conj_matches(ctx); >> } >> >> /* If tracing is enabled in 'ctx', adds an OFT_DETAIL trace node to 'ctx' > >> @@ -4653,7 +4678,8 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t >> in_port, uint8_t table_id, >> ctx->xin->resubmit_stats, >> &ctx->table_id, in_port, >> may_packet_in, honor_table_miss, >> - ctx->xin->xcache); >> + ctx->xin->xcache, >> + &ctx->xout->conj_flows); > > As per my comment here [1] on another patch, I'm concerned about functions > with too many (say more than 6 arguments). For one thing. AFAIK, there is > the overhead of needing to pass arguments the stack rather than purely in > registers. For another, I think there are questions of readability and > maintainability. > > It is already the case before this patch, although this patch does > make it slightly worse. > > [1] Re: [PATCH v3] ofproto-dpif-mirror: Add support for pre-selection filter > https://mail.openvswitch.org/pipermail/ovs-dev/2023-October/408228.html > > ... Agreed. It seems to me that there are approaches such as grouping arguments together as some kind of structure, or separating responsibilities by dividing one function into multiple functions. I will do some digging. But, any refactoring requires knowledge of existing data structures, so if you have any insights, I would appreciate your comments. Best Regards, Nobuhiro Miki _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
