[ovs-dev] dpif-netlink: avoid netlink modify flow put op failed after tc modify flow put op failed.

From: wenxu <wenxu@ucloud.cn>

The tc modify flow put always delete the original flow first and
then add the new flow. If the modfiy flow put operation failed,
the flow put operation will change from modify to create if success
to delete the original flow in tc (which will be always failed with
ENOENT, the flow is already be deleted before add the new flow in tc).
Finally, the modify flow put will failed to add in kernel datapath.

Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 lib/dpif-netlink.c      | 7 ++++++-
 lib/netdev-offload-tc.c | 2 +-
 lib/netdev-offload.h    | 3 +++
 3 files changed, 10 insertions(+), 2 deletions(-)

On 2020-03-11 7:39 AM, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> The tc modify flow put always delete the original flow first and
> then add the new flow. If the modfiy flow put operation failed,
> the flow put operation will change from modify to create if success
> to delete the original flow in tc (which will be always failed with
> ENOENT, the flow is already be deleted before add the new flow in tc).
> Finally, the modify flow put will failed to add in kernel datapath.
> 
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  lib/dpif-netlink.c      | 7 ++++++-
>  lib/netdev-offload-tc.c | 2 +-
>  lib/netdev-offload.h    | 3 +++
>  3 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 5b5c96d..b8d08a6 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2091,6 +2091,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>      info.tunnel_csum_on = csum_on;
>      info.recirc_id_shared_with_tc = (dpif->user_features
>                                       & OVS_DP_F_TC_RECIRC_SHARING);
> +    info.tc_modify_flow_deleted = false;
>      err = netdev_flow_put(dev, &match,
>                            CONST_CAST(struct nlattr *, put->actions),
>                            put->actions_len,
> @@ -2141,7 +2142,11 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>  out:
>      if (err && err != EEXIST && (put->flags & DPIF_FP_MODIFY)) {
>          /* Modified rule can't be offloaded, try and delete from HW */
> -        int del_err = netdev_flow_del(dev, put->ufid, put->stats);
> +        int del_err = 0;
> +
> +        if (!info.tc_modify_flow_deleted) {
> +            del_err = netdev_flow_del(dev, put->ufid, put->stats);
> +        }
>  
>          if (!del_err) {
>              /* Delete from hw success, so old flow was offloaded.
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 550e440..5e7b873 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1727,7 +1727,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      if (get_ufid_tc_mapping(ufid, &id) == 0) {
>          VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
>                      id.handle, id.prio);
> -        del_filter_and_ufid_mapping(&id, ufid);
> +        info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(&id, ufid);
>      }
>  
>      prio = get_prio_for_tc_flower(&flower);
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index cd6dfdf..b4b882a 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
> @@ -74,6 +74,9 @@ struct offload_info {
>       * it will be in the pkt meta data.
>       */
>      uint32_t flow_mark;
> +
> +    bool tc_modify_flow_deleted; /* Indicate the tc modify flow put success
> +                                  * to delete the original flow. */
>  };
>  
>  int netdev_flow_flush(struct netdev *);
> 

Acked-by: Roi Dayan <roid@mellanox.com>

On Thu, Mar 19, 2020 at 11:28:00AM +0200, Roi Dayan wrote:
> 
> 
> On 2020-03-11 7:39 AM, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> > 
> > The tc modify flow put always delete the original flow first and
> > then add the new flow. If the modfiy flow put operation failed,
> > the flow put operation will change from modify to create if success
> > to delete the original flow in tc (which will be always failed with
> > ENOENT, the flow is already be deleted before add the new flow in tc).
> > Finally, the modify flow put will failed to add in kernel datapath.
> > 
> > Signed-off-by: wenxu <wenxu@ucloud.cn>

...

> Acked-by: Roi Dayan <roid@mellanox.com>

Thanks,

this looks good to me.

I am exercising the patch applied on top of master and branch-2.13
using Travis CI. And I plan to push the patch to those branches if
that succeeds.

The patch does not apply cleanly on branch-2.12. Please consider
posting a backport to that and earlier branches if the change
is appropriate there.

On Thu, Mar 19, 2020 at 12:04:40PM +0100, Simon Horman wrote:
> On Thu, Mar 19, 2020 at 11:28:00AM +0200, Roi Dayan wrote:
> > 
> > 
> > On 2020-03-11 7:39 AM, wenxu@ucloud.cn wrote:
> > > From: wenxu <wenxu@ucloud.cn>
> > > 
> > > The tc modify flow put always delete the original flow first and
> > > then add the new flow. If the modfiy flow put operation failed,
> > > the flow put operation will change from modify to create if success
> > > to delete the original flow in tc (which will be always failed with
> > > ENOENT, the flow is already be deleted before add the new flow in tc).
> > > Finally, the modify flow put will failed to add in kernel datapath.
> > > 
> > > Signed-off-by: wenxu <wenxu@ucloud.cn>
> 
> ...
> 
> > Acked-by: Roi Dayan <roid@mellanox.com>
> 
> Thanks,
> 
> this looks good to me.
> 
> I am exercising the patch applied on top of master and branch-2.13
> using Travis CI. And I plan to push the patch to those branches if
> that succeeds.
> 
> The patch does not apply cleanly on branch-2.12. Please consider
> posting a backport to that and earlier branches if the change
> is appropriate there.

Thanks again, pushed to master and branch-2.13.