[ovs-dev] [PATCH ovn v5 4/9] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed.
Numan Siddique
numans at ovn.org
Fri May 15 12:23:45 UTC 2020
On Wed, May 13, 2020 at 7:24 PM Dumitru Ceara <dceara at redhat.com> wrote:
> On 5/11/20 11:46 AM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > 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 <numans at ovn.org>
>
> 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.
Thanks
Numan
> I'm not sure if that makes the code harder to read though. What do you
> think?
>
> Thanks,
> Dumitru
>
> > }
> > ovn_flow_destroy(f);
> > return;
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
More information about the dev
mailing list