[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/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.

I'm not sure if that makes the code harder to read though. What do you
think?

Thanks,
Dumitru

>          }
>          ovn_flow_destroy(f);
>          return;
>