| Message ID | 1600228064-129210-1-git-send-email-hzhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] lflow.c: Rename function convert_acts_to_expr to convert_match_to_expr. | expand |
On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org> wrote: > The name was misleading because it in fact parses lflow match instead > of actions. > > Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr matches.") > Cc: Numan Siddique <numans@ovn.org> > Signed-off-by: Han Zhou <hzhou@ovn.org> > Thanks for the fix. The name was misleading. Acked-by: Numan Siddique <numans@ovn.org> Numan --- > controller/lflow.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index e785866..9a96aac 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct > sbrec_logical_flow *lflow, > ofpbuf_uninit(&ofpacts); > } > > -/* Converts the actions and returns the simplified expre tree. > +/* Converts the match and returns the simplified expre tree. > * The caller should evaluate the conditions and normalize the > * expr tree. */ > static struct expr * > -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, > - struct expr *prereqs, > - struct lflow_ctx_in *l_ctx_in, > - struct lflow_ctx_out *l_ctx_out, > - bool *pg_addr_set_ref, char **errorp) > +convert_match_to_expr(const struct sbrec_logical_flow *lflow, > + struct expr *prereqs, > + struct lflow_ctx_in *l_ctx_in, > + struct lflow_ctx_out *l_ctx_out, > + bool *pg_addr_set_ref, char **errorp) > { > struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > @@ -861,7 +861,7 @@ consider_logical_flow(const struct sbrec_logical_flow > *lflow, > struct expr *expr = NULL; > if (!l_ctx_out->lflow_cache_map) { > /* Caching is disabled. */ > - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, NULL, &error); > if (error) { > expr_destroy(prereqs); > @@ -924,7 +924,7 @@ consider_logical_flow(const struct sbrec_logical_flow > *lflow, > > bool pg_addr_set_ref = false; > if (!expr) { > - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, > + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, > &pg_addr_set_ref, &error); > if (error) { > expr_destroy(prereqs); > -- > 2.1.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 9/16/20 8:11 AM, Numan Siddique wrote: > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org> wrote: > >> The name was misleading because it in fact parses lflow match instead >> of actions. >> >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr matches.") >> Cc: Numan Siddique <numans@ovn.org> >> Signed-off-by: Han Zhou <hzhou@ovn.org> >> > > Thanks for the fix. The name was misleading. > > Acked-by: Numan Siddique <numans@ovn.org> > > Numan > > --- >> controller/lflow.c | 16 ++++++++-------- >> 1 file changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/controller/lflow.c b/controller/lflow.c >> index e785866..9a96aac 100644 >> --- a/controller/lflow.c >> +++ b/controller/lflow.c >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct >> sbrec_logical_flow *lflow, >> ofpbuf_uninit(&ofpacts); >> } >> >> -/* Converts the actions and returns the simplified expre tree. >> +/* Converts the match and returns the simplified expre tree. >> * The caller should evaluate the conditions and normalize the >> * expr tree. */ >> static struct expr * >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, >> - struct expr *prereqs, >> - struct lflow_ctx_in *l_ctx_in, >> - struct lflow_ctx_out *l_ctx_out, >> - bool *pg_addr_set_ref, char **errorp) >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow, >> + struct expr *prereqs, >> + struct lflow_ctx_in *l_ctx_in, >> + struct lflow_ctx_out *l_ctx_out, >> + bool *pg_addr_set_ref, char **errorp) Hi Han, Numan, To be honest, we might be able to do better. We still have to look at the implementation of convert_match_to_expr() to see what it uses from l_ctx_in and what side effects it has on l_ctx_out. Right now it: - parses lflow->match - uses lflow->header_.uuid - uses l_ctx_in->port_groups/address_sets - it modifies l_ctx_out->lfrr - it also sets pg_addr_set_ref and errorp I'm finding it hard to track all the side effects without scanning every line of the function. Can't we refactor the function and just pass it the in/out arguments it needs? I know there was an effort a while ago to put all "input"/"output" parameters in in/out "context" structures, but passing the large contexts in a lot of functions is almost the same thing as having the contexts stored as global variables. Also, now that we're fixing this function we should also remove this line: https://github.com/ovn-org/ovn/blob/7c6009af2c0766b38c56147c21d0b9cb887389e3/controller/lflow.c#L757 >> { >> struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); >> struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); >> @@ -861,7 +861,7 @@ consider_logical_flow(const struct sbrec_logical_flow >> *lflow, >> struct expr *expr = NULL; >> if (!l_ctx_out->lflow_cache_map) { >> /* Caching is disabled. */ >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, >> l_ctx_out, NULL, &error); Indentation is wrong here. >> if (error) { >> expr_destroy(prereqs); >> @@ -924,7 +924,7 @@ consider_logical_flow(const struct sbrec_logical_flow >> *lflow, >> >> bool pg_addr_set_ref = false; >> if (!expr) { >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, >> &pg_addr_set_ref, &error); Indentation is wrong here. Regards, Dumitru >> if (error) { >> expr_destroy(prereqs); >> -- >> 2.1.0 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <dceara@redhat.com> wrote: > On 9/16/20 8:11 AM, Numan Siddique wrote: > > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org> wrote: > > > >> The name was misleading because it in fact parses lflow match instead > >> of actions. > >> > >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr matches.") > >> Cc: Numan Siddique <numans@ovn.org> > >> Signed-off-by: Han Zhou <hzhou@ovn.org> > >> > > > > Thanks for the fix. The name was misleading. > > > > Acked-by: Numan Siddique <numans@ovn.org> > > > > Numan > > > > --- > >> controller/lflow.c | 16 ++++++++-------- > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/controller/lflow.c b/controller/lflow.c > >> index e785866..9a96aac 100644 > >> --- a/controller/lflow.c > >> +++ b/controller/lflow.c > >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct > >> sbrec_logical_flow *lflow, > >> ofpbuf_uninit(&ofpacts); > >> } > >> > >> -/* Converts the actions and returns the simplified expre tree. > >> +/* Converts the match and returns the simplified expre tree. > >> * The caller should evaluate the conditions and normalize the > >> * expr tree. */ > >> static struct expr * > >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, > >> - struct expr *prereqs, > >> - struct lflow_ctx_in *l_ctx_in, > >> - struct lflow_ctx_out *l_ctx_out, > >> - bool *pg_addr_set_ref, char **errorp) > >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow, > >> + struct expr *prereqs, > >> + struct lflow_ctx_in *l_ctx_in, > >> + struct lflow_ctx_out *l_ctx_out, > >> + bool *pg_addr_set_ref, char **errorp) > > Hi Han, Numan, > > To be honest, we might be able to do better. We still have to look at > the implementation of convert_match_to_expr() to see what it uses from > l_ctx_in and what side effects it has on l_ctx_out. Right now it: > > - parses lflow->match > - uses lflow->header_.uuid > - uses l_ctx_in->port_groups/address_sets > - it modifies l_ctx_out->lfrr > - it also sets pg_addr_set_ref and errorp > > I'm finding it hard to track all the side effects without scanning every > line of the function. > > Can't we refactor the function and just pass it the in/out arguments it > needs? > > I know there was an effort a while ago to put all "input"/"output" > parameters in in/out "context" structures, but passing the large > contexts in a lot of functions is almost the same thing as having the > contexts stored as global variables. > > Hi Dumitru, Well, I'm not really sure if it's a bad programming choice or not. I personally would like to see less arguments passed to the functions and hence prefer a context variable to be passed. We adopted the similar function arguments for the I-P improvement patch series. One such example - https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L509 where we just use the one variable in this function from the binding_ctx_out. I tried to see if there are any coding guidelines (in OVS/OVN) for passing arguments to the functions and what is the preferred way. I couldn't find any. If you think passing specific arguments would be better, I'm fine with it. But I think this patch is addressing a specific case and I think it's better to address your concerns in a separate patch. I'm fine with this patch AS IS. It's up to Han if he wants to spin up another patch to change the function arguments. Otherwise, I request you to submit a follow patch for this. Thanks Numan > Also, now that we're fixing this function we should also remove this line: > > > https://github.com/ovn-org/ovn/blob/7c6009af2c0766b38c56147c21d0b9cb887389e3/controller/lflow.c#L757 > > >> { > >> struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > >> struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); > >> @@ -861,7 +861,7 @@ consider_logical_flow(const struct > sbrec_logical_flow > >> *lflow, > >> struct expr *expr = NULL; > >> if (!l_ctx_out->lflow_cache_map) { > >> /* Caching is disabled. */ > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > >> l_ctx_out, NULL, &error); > > Indentation is wrong here. > > >> if (error) { > >> expr_destroy(prereqs); > >> @@ -924,7 +924,7 @@ consider_logical_flow(const struct > sbrec_logical_flow > >> *lflow, > >> > >> bool pg_addr_set_ref = false; > >> if (!expr) { > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, > >> &pg_addr_set_ref, &error); > > Indentation is wrong here. > > Regards, > Dumitru > > >> if (error) { > >> expr_destroy(prereqs); > >> -- > >> 2.1.0 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 9/16/20 4:04 PM, Numan Siddique wrote: > > > > On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > On 9/16/20 8:11 AM, Numan Siddique wrote: > > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org > <mailto:hzhou@ovn.org>> wrote: > > > >> The name was misleading because it in fact parses lflow match instead > >> of actions. > >> > >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr > matches.") > >> Cc: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > >> Signed-off-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > >> > > > > Thanks for the fix. The name was misleading. > > > > Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > Numan > > > > --- > >> controller/lflow.c | 16 ++++++++-------- > >> 1 file changed, 8 insertions(+), 8 deletions(-) > >> > >> diff --git a/controller/lflow.c b/controller/lflow.c > >> index e785866..9a96aac 100644 > >> --- a/controller/lflow.c > >> +++ b/controller/lflow.c > >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct > >> sbrec_logical_flow *lflow, > >> ofpbuf_uninit(&ofpacts); > >> } > >> > >> -/* Converts the actions and returns the simplified expre tree. > >> +/* Converts the match and returns the simplified expre tree. > >> * The caller should evaluate the conditions and normalize the > >> * expr tree. */ > >> static struct expr * > >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, > >> - struct expr *prereqs, > >> - struct lflow_ctx_in *l_ctx_in, > >> - struct lflow_ctx_out *l_ctx_out, > >> - bool *pg_addr_set_ref, char **errorp) > >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow, > >> + struct expr *prereqs, > >> + struct lflow_ctx_in *l_ctx_in, > >> + struct lflow_ctx_out *l_ctx_out, > >> + bool *pg_addr_set_ref, char **errorp) > > Hi Han, Numan, > > To be honest, we might be able to do better. We still have to look at > the implementation of convert_match_to_expr() to see what it uses from > l_ctx_in and what side effects it has on l_ctx_out. Right now it: > > - parses lflow->match > - uses lflow->header_.uuid > - uses l_ctx_in->port_groups/address_sets > - it modifies l_ctx_out->lfrr > - it also sets pg_addr_set_ref and errorp > > I'm finding it hard to track all the side effects without scanning every > line of the function. > > Can't we refactor the function and just pass it the in/out arguments it > needs? > > I know there was an effort a while ago to put all "input"/"output" > parameters in in/out "context" structures, but passing the large > contexts in a lot of functions is almost the same thing as having the > contexts stored as global variables. > > > > Hi Dumitru, > Hi Numan, > Well, I'm not really sure if it's a bad programming choice or not. > I personally would like to see less arguments passed to the functions > and hence prefer a context variable to be passed. We adopted the similar > function arguments for the I-P improvement patch series. > One such example - > https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L509 > where we just use the one variable in this function from the > binding_ctx_out. > I'm not sure either, but I do find it harder to get the gist of what a function does (or should do) without reading every line of the function. > I tried to see if there are any coding guidelines (in OVS/OVN) for > passing arguments > to the functions and what is the preferred way. I couldn't find any. > There are some general guidelines: https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions > If you think passing specific arguments would be better, I'm fine with it. > > But I think this patch is addressing a specific case and I think it's > better to > address your concerns in a separate patch. I'm fine with this patch AS IS. > There are a few other minor comments related to incorrect indentation that should be fixed before this gets merged. > It's up to Han if he wants to spin up another patch to change the > function arguments. > Otherwise, I request you to submit a follow patch for this. > I'll do my best :) > Thanks > Numan > Thanks, Dumitru > > Also, now that we're fixing this function we should also remove this > line: > > https://github.com/ovn-org/ovn/blob/7c6009af2c0766b38c56147c21d0b9cb887389e3/controller/lflow.c#L757 > > >> { > >> struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > >> struct sset port_groups_ref = > SSET_INITIALIZER(&port_groups_ref); > >> @@ -861,7 +861,7 @@ consider_logical_flow(const struct > sbrec_logical_flow > >> *lflow, > >> struct expr *expr = NULL; > >> if (!l_ctx_out->lflow_cache_map) { > >> /* Caching is disabled. */ > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > >> l_ctx_out, NULL, &error); > > Indentation is wrong here. > > >> if (error) { > >> expr_destroy(prereqs); > >> @@ -924,7 +924,7 @@ consider_logical_flow(const struct > sbrec_logical_flow > >> *lflow, > >> > >> bool pg_addr_set_ref = false; > >> if (!expr) { > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > l_ctx_out, > >> &pg_addr_set_ref, &error); > > Indentation is wrong here. > > Regards, > Dumitru > > >> if (error) { > >> expr_destroy(prereqs); > >> -- > >> 2.1.0 > >> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@openvswitch.org> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 9/16/20 4:04 PM, Numan Siddique wrote: > > > > > > > > On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <dceara@redhat.com > > <mailto:dceara@redhat.com>> wrote: > > > > On 9/16/20 8:11 AM, Numan Siddique wrote: > > > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org > > <mailto:hzhou@ovn.org>> wrote: > > > > > >> The name was misleading because it in fact parses lflow match instead > > >> of actions. > > >> > > >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr > > matches.") > > >> Cc: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > >> Signed-off-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > > >> > > > > > > Thanks for the fix. The name was misleading. > > > > > > Acked-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > > > > > > Numan > > > > > > --- > > >> controller/lflow.c | 16 ++++++++-------- > > >> 1 file changed, 8 insertions(+), 8 deletions(-) > > >> > > >> diff --git a/controller/lflow.c b/controller/lflow.c > > >> index e785866..9a96aac 100644 > > >> --- a/controller/lflow.c > > >> +++ b/controller/lflow.c > > >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct > > >> sbrec_logical_flow *lflow, > > >> ofpbuf_uninit(&ofpacts); > > >> } > > >> > > >> -/* Converts the actions and returns the simplified expre tree. > > >> +/* Converts the match and returns the simplified expre tree. > > >> * The caller should evaluate the conditions and normalize the > > >> * expr tree. */ > > >> static struct expr * > > >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, > > >> - struct expr *prereqs, > > >> - struct lflow_ctx_in *l_ctx_in, > > >> - struct lflow_ctx_out *l_ctx_out, > > >> - bool *pg_addr_set_ref, char **errorp) > > >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow, > > >> + struct expr *prereqs, > > >> + struct lflow_ctx_in *l_ctx_in, > > >> + struct lflow_ctx_out *l_ctx_out, > > >> + bool *pg_addr_set_ref, char **errorp) > > > > Hi Han, Numan, > > > > To be honest, we might be able to do better. We still have to look at > > the implementation of convert_match_to_expr() to see what it uses from > > l_ctx_in and what side effects it has on l_ctx_out. Right now it: > > > > - parses lflow->match > > - uses lflow->header_.uuid > > - uses l_ctx_in->port_groups/address_sets > > - it modifies l_ctx_out->lfrr > > - it also sets pg_addr_set_ref and errorp > > > > I'm finding it hard to track all the side effects without scanning every > > line of the function. > > > > Can't we refactor the function and just pass it the in/out arguments it > > needs? > > > > I know there was an effort a while ago to put all "input"/"output" > > parameters in in/out "context" structures, but passing the large > > contexts in a lot of functions is almost the same thing as having the > > contexts stored as global variables. > > > > > > > > Hi Dumitru, > > > > Hi Numan, > > > Well, I'm not really sure if it's a bad programming choice or not. > > I personally would like to see less arguments passed to the functions > > and hence prefer a context variable to be passed. We adopted the similar > > function arguments for the I-P improvement patch series. > > One such example - > > https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L509 > > where we just use the one variable in this function from the > > binding_ctx_out. > > > > I'm not sure either, but I do find it harder to get the gist of what a > function does (or should do) without reading every line of the function. > I think either approach has its pros and cons here. With ctx wrapping the args, the function prototypes are concise. However, it does make the dependency less obvious. Initially when the parameter was converted to ctx, it only replaces the places when all the members are used. Now it seems it is easily passed to a static function that only uses some of them, thus hiding the real dependency. So, I tend to agree with Dumitru in this specific case that it may be better to unwrap the ctx structure because it is helpful for tracking the I-P dependency. If we make this change, it should belong to a separate patch, though. > > I tried to see if there are any coding guidelines (in OVS/OVN) for > > passing arguments > > to the functions and what is the preferred way. I couldn't find any. > > > > There are some general guidelines: > > https://github.com/ovn-org/ovn/blob/master/Documentation/internals/contributing/coding-style.rst#functions > > > If you think passing specific arguments would be better, I'm fine with it. > > > > But I think this patch is addressing a specific case and I think it's > > better to > > address your concerns in a separate patch. I'm fine with this patch AS IS. > > > > There are a few other minor comments related to incorrect indentation > that should be fixed before this gets merged. Thanks for spotting this. I sent v2 for the indentation. Please take a look. > > > It's up to Han if he wants to spin up another patch to change the > > function arguments. > > Otherwise, I request you to submit a follow patch for this. > > > > I'll do my best :) > > > Thanks > > Numan > > > > Thanks, > Dumitru > > > > > Also, now that we're fixing this function we should also remove this > > line: > > > > https://github.com/ovn-org/ovn/blob/7c6009af2c0766b38c56147c21d0b9cb887389e3/controller/lflow.c#L757 > > > > >> { > > >> struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); > > >> struct sset port_groups_ref = > > SSET_INITIALIZER(&port_groups_ref); > > >> @@ -861,7 +861,7 @@ consider_logical_flow(const struct > > sbrec_logical_flow > > >> *lflow, > > >> struct expr *expr = NULL; > > >> if (!l_ctx_out->lflow_cache_map) { > > >> /* Caching is disabled. */ > > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > > >> l_ctx_out, NULL, &error); > > > > Indentation is wrong here. > > > > >> if (error) { > > >> expr_destroy(prereqs); > > >> @@ -924,7 +924,7 @@ consider_logical_flow(const struct > > sbrec_logical_flow > > >> *lflow, > > >> > > >> bool pg_addr_set_ref = false; > > >> if (!expr) { > > >> - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, > > l_ctx_out, > > >> + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, > > l_ctx_out, > > >> &pg_addr_set_ref, &error); > > > > Indentation is wrong here. > > > > Regards, > > Dumitru > > > > >> if (error) { > > >> expr_destroy(prereqs); > > >> -- > > >> 2.1.0 > > >> > > >> _______________________________________________ > > >> dev mailing list > > >> dev@openvswitch.org <mailto:dev@openvswitch.org> > > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >> > > >> > > > _______________________________________________ > > > dev mailing list > > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
On 9/16/20 6:14 PM, Han Zhou wrote: > > > On Wed, Sep 16, 2020 at 7:28 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: >> >> On 9/16/20 4:04 PM, Numan Siddique wrote: >> > >> > >> > >> > On Wed, Sep 16, 2020 at 3:56 PM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com> >> > <mailto:dceara@redhat.com <mailto:dceara@redhat.com>>> wrote: >> > >> > On 9/16/20 8:11 AM, Numan Siddique wrote: >> > > On Wed, Sep 16, 2020 at 9:18 AM Han Zhou <hzhou@ovn.org > <mailto:hzhou@ovn.org> >> > <mailto:hzhou@ovn.org <mailto:hzhou@ovn.org>>> wrote: >> > > >> > >> The name was misleading because it in fact parses lflow match > instead >> > >> of actions. >> > >> >> > >> Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr >> > matches.") >> > >> Cc: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org> > <mailto:numans@ovn.org <mailto:numans@ovn.org>>> >> > >> Signed-off-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org> > <mailto:hzhou@ovn.org <mailto:hzhou@ovn.org>>> >> > >> >> > > >> > > Thanks for the fix. The name was misleading. >> > > >> > > Acked-by: Numan Siddique <numans@ovn.org > <mailto:numans@ovn.org> <mailto:numans@ovn.org <mailto:numans@ovn.org>>> >> > > >> > > Numan >> > > >> > > --- >> > >> controller/lflow.c | 16 ++++++++-------- >> > >> 1 file changed, 8 insertions(+), 8 deletions(-) >> > >> >> > >> diff --git a/controller/lflow.c b/controller/lflow.c >> > >> index e785866..9a96aac 100644 >> > >> --- a/controller/lflow.c >> > >> +++ b/controller/lflow.c >> > >> @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct >> > >> sbrec_logical_flow *lflow, >> > >> ofpbuf_uninit(&ofpacts); >> > >> } >> > >> >> > >> -/* Converts the actions and returns the simplified expre tree. >> > >> +/* Converts the match and returns the simplified expre tree. >> > >> * The caller should evaluate the conditions and normalize the >> > >> * expr tree. */ >> > >> static struct expr * >> > >> -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, >> > >> - struct expr *prereqs, >> > >> - struct lflow_ctx_in *l_ctx_in, >> > >> - struct lflow_ctx_out *l_ctx_out, >> > >> - bool *pg_addr_set_ref, char **errorp) >> > >> +convert_match_to_expr(const struct sbrec_logical_flow *lflow, >> > >> + struct expr *prereqs, >> > >> + struct lflow_ctx_in *l_ctx_in, >> > >> + struct lflow_ctx_out *l_ctx_out, >> > >> + bool *pg_addr_set_ref, char **errorp) >> > >> > Hi Han, Numan, >> > >> > To be honest, we might be able to do better. We still have to > look at >> > the implementation of convert_match_to_expr() to see what it > uses from >> > l_ctx_in and what side effects it has on l_ctx_out. Right now it: >> > >> > - parses lflow->match >> > - uses lflow->header_.uuid >> > - uses l_ctx_in->port_groups/address_sets >> > - it modifies l_ctx_out->lfrr >> > - it also sets pg_addr_set_ref and errorp >> > >> > I'm finding it hard to track all the side effects without > scanning every >> > line of the function. >> > >> > Can't we refactor the function and just pass it the in/out > arguments it >> > needs? >> > >> > I know there was an effort a while ago to put all "input"/"output" >> > parameters in in/out "context" structures, but passing the large >> > contexts in a lot of functions is almost the same thing as > having the >> > contexts stored as global variables. >> > >> > >> > >> > Hi Dumitru, >> > >> >> Hi Numan, >> >> > Well, I'm not really sure if it's a bad programming choice or not. >> > I personally would like to see less arguments passed to the functions >> > and hence prefer a context variable to be passed. We adopted the similar >> > function arguments for the I-P improvement patch series. >> > One such example - >> > https://github.com/ovn-org/ovn/blob/master/controller/binding.c#L509 >> > where we just use the one variable in this function from the >> > binding_ctx_out. >> > >> >> I'm not sure either, but I do find it harder to get the gist of what a >> function does (or should do) without reading every line of the function. >> > I think either approach has its pros and cons here. With ctx wrapping > the args, the function prototypes are concise. However, it does make the > dependency less obvious. Initially when the parameter was converted to > ctx, it only replaces the places when all the members are used. Now it > seems it is easily passed to a static function that only uses some of > them, thus hiding the real dependency. So, I tend to agree with Dumitru > in this specific case that it may be better to unwrap the ctx structure > because it is helpful for tracking the I-P dependency. If we make this > change, it should belong to a separate patch, though. > Hi Han, I acked v2 and I'll try to follow up on this part in the coming weeks. Thanks, Dumitru
diff --git a/controller/lflow.c b/controller/lflow.c index e785866..9a96aac 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -720,15 +720,15 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow, ofpbuf_uninit(&ofpacts); } -/* Converts the actions and returns the simplified expre tree. +/* Converts the match and returns the simplified expre tree. * The caller should evaluate the conditions and normalize the * expr tree. */ static struct expr * -convert_acts_to_expr(const struct sbrec_logical_flow *lflow, - struct expr *prereqs, - struct lflow_ctx_in *l_ctx_in, - struct lflow_ctx_out *l_ctx_out, - bool *pg_addr_set_ref, char **errorp) +convert_match_to_expr(const struct sbrec_logical_flow *lflow, + struct expr *prereqs, + struct lflow_ctx_in *l_ctx_in, + struct lflow_ctx_out *l_ctx_out, + bool *pg_addr_set_ref, char **errorp) { struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); @@ -861,7 +861,7 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, struct expr *expr = NULL; if (!l_ctx_out->lflow_cache_map) { /* Caching is disabled. */ - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, NULL, &error); if (error) { expr_destroy(prereqs); @@ -924,7 +924,7 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow, bool pg_addr_set_ref = false; if (!expr) { - expr = convert_acts_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, + expr = convert_match_to_expr(lflow, prereqs, l_ctx_in, l_ctx_out, &pg_addr_set_ref, &error); if (error) { expr_destroy(prereqs);
The name was misleading because it in fact parses lflow match instead of actions. Fixes: 1213bc8270 ("ovn-controller: Cache logical flow expr matches.") Cc: Numan Siddique <numans@ovn.org> Signed-off-by: Han Zhou <hzhou@ovn.org> --- controller/lflow.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)