[ovs-dev] [PATCH ovn v5 4/9] ofctrl_check_and_add_flow: Replace the actions of an existing flow if actions have changed.

[Dumitru Ceara]

On 5/15/20 2:23 PM, Numan Siddique wrote:
> 
> 
> On Wed, May 13, 2020 at 7:24 PM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
> 
>     On 5/11/20 11:46 AM, numans at ovn.org <mailto:numans at ovn.org> wrote:
>     > From: Numan Siddique <numans at ovn.org <mailto: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 <mailto: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.
> 

You're right, we can't do it in ofctrl_add_or_append_flow(), thanks for
double checking.

Regards,
Dumitru