On 5/15/20 2:23 PM, Numan Siddique wrote: > > > On Wed, May 13, 2020 at 7:24 PM Dumitru Ceara <[email protected] > <mailto:[email protected]>> wrote: > > On 5/11/20 11:46 AM, [email protected] <mailto:[email protected]> wrote: > > From: Numan Siddique <[email protected] <mailto:[email protected]>> > > > > If ofctrl_check_and_add_flow(F') is called where flow F' has > match-actions (M, A2) > > and if there already exists a flow F with match-actions (M, A1) in > the desired flow > > table, then this function doesn't update the existing flow F with > new actions > > actions A2. > > > > This patch is required for the upcoming patch in this series which > > adds incremental processing for OVS interface in the flow output > stage. > > Since we will be not be clearing the flow output data if these changes > > are handled incrementally, some of the existing flows will be updated > > with new actions. One such example would be flows in physical > > table OFTABLE_LOCAL_OUTPUT (table 33). And this patch is required to > > update such flows. Otherwise we will have incomplete actions > installed. > > > > Signed-off-by: Numan Siddique <[email protected] <mailto:[email protected]>> > > Hi Numan, > > I think the title of the commit should be "ofctrl: Replace the actions > of an existing flow if actions have changed." > > > --- > > controller/ofctrl.c | 23 ++++++++++++++++------- > > 1 file changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 4b51cd86e..8f2f13767 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -667,14 +667,23 @@ ofctrl_check_and_add_flow(struct > ovn_desired_flow_table *flow_table, > > > > ovn_flow_log(f, "ofctrl_add_flow"); > > > > - if (ovn_flow_lookup(&flow_table->match_flow_table, f, true)) { > > - if (log_duplicate_flow) { > > - static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > - if (!VLOG_DROP_DBG(&rl)) { > > - char *s = ovn_flow_to_string(f); > > - VLOG_DBG("dropping duplicate flow: %s", s); > > - free(s); > > + struct ovn_flow *existing_f = > > + ovn_flow_lookup(&flow_table->match_flow_table, f, true); > > + if (existing_f) { > > + if (ofpacts_equal(f->ofpacts, f->ofpacts_len, > > + existing_f->ofpacts, > existing_f->ofpacts_len)) { > > + if (log_duplicate_flow) { > > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 5); > > + if (!VLOG_DROP_DBG(&rl)) { > > + char *s = ovn_flow_to_string(f); > > + VLOG_DBG("dropping duplicate flow: %s", s); > > + free(s); > > + } > > } > > + } else { > > + free(existing_f->ofpacts); > > + existing_f->ofpacts = xmemdup(f->ofpacts, > f->ofpacts_len); > > + existing_f->ofpacts_len = f->ofpacts_len; > > We could avoid the free(existing_f->ofpacts) followed by xmemdup by > swapping f->ofpacts with existing_f->ofpacts (same for ofpacts_len). > We'd probably need a function for that and we'd have to call it in > ofctrl_add_or_append_flow() too. > > > Good idea. I'll do as suggested. But I don't think we can do the same for > ofctrl_add_or_append_flow() as it appends the actions. >
You're right, we can't do it in ofctrl_add_or_append_flow(), thanks for double checking. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
