[ovs-dev] [PATCH V3 02/19] netdev-offload-dpdk: Refactor action items freeing scheme

[Ilya Maximets]

On 08.12.2019 14:22, Eli Britstein wrote:
> Action item data structures are pointed by rte_flow_action items.
> Refactor the code to free the data structures when freeing the
> rte_flow_action items, allowing simpler future actions simpler to add to
> the code.
> 
> Signed-off-by: Eli Britstein <elibr at mellanox.com>
> ---
>  lib/netdev-offload-dpdk.c | 92 ++++++++++++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 40 deletions(-)
> 
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index a008e642f..c3b595a0a 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -374,40 +374,19 @@ add_flow_action(struct flow_actions *actions, enum rte_flow_action_type type,
>      actions->cnt++;
>  }
>  
> -struct action_rss_data {
> -    struct rte_flow_action_rss conf;
> -    uint16_t queue[0];
> -};
> -
> -static struct action_rss_data *
> -add_flow_rss_action(struct flow_actions *actions,
> -                    struct netdev *netdev)
> +static void
> +free_flow_actions(struct flow_actions *actions)
>  {
>      int i;
> -    struct action_rss_data *rss_data;
> -
> -    rss_data = xmalloc(sizeof *rss_data +
> -                       netdev_n_rxq(netdev) * sizeof rss_data->queue[0]);
> -    *rss_data = (struct action_rss_data) {
> -        .conf = (struct rte_flow_action_rss) {
> -            .func = RTE_ETH_HASH_FUNCTION_DEFAULT,
> -            .level = 0,
> -            .types = 0,
> -            .queue_num = netdev_n_rxq(netdev),
> -            .queue = rss_data->queue,
> -            .key_len = 0,
> -            .key  = NULL
> -        },
> -    };
>  
> -    /* Override queue array with default. */
> -    for (i = 0; i < netdev_n_rxq(netdev); i++) {
> -       rss_data->queue[i] = i;
> +    for (i = 0; i < actions->cnt; i++) {
> +        if (actions->actions[i].conf) {
> +            free((void *)actions->actions[i].conf);

Please, don't cast the argument.

> +        }
>      }
> -
> -    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RSS, &rss_data->conf);
> -
> -    return rss_data;
> +    free(actions->actions);
> +    actions->actions = NULL;
> +    actions->cnt = 0;
>  }
>  
>  struct flow_items {
> @@ -572,6 +551,47 @@ parse_flow_match(struct flow_patterns *patterns,
>      return 0;
>  }
>  
> +static void
> +add_flow_mark_rss_actions(struct flow_actions *actions,
> +                          uint32_t flow_mark,
> +                          struct netdev *netdev)

const struct netdev *netdev)

> +{
> +    struct rte_flow_action_mark *mark;
> +    struct action_rss_data {
> +        struct rte_flow_action_rss conf;
> +        uint16_t queue[0];
> +    } *rss_data;

It seems we need this:

BUILD_ASSERT_DECL(offsetof(struct action_rss_data, conf) == 0); 

> +    int i;
> +
> +    mark = xzalloc(sizeof *mark);
> +
> +    mark->id = flow_mark;
> +    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_MARK, mark);
> +
> +    rss_data = xmalloc(sizeof *rss_data +
> +                       netdev_n_rxq(netdev) * sizeof rss_data->queue[0]);
> +    *rss_data = (struct action_rss_data) {
> +        .conf = (struct rte_flow_action_rss) {
> +            .func = RTE_ETH_HASH_FUNCTION_DEFAULT,
> +            .level = 0,
> +            .types = 0,
> +            .queue_num = netdev_n_rxq(netdev),
> +            .queue = rss_data->queue,
> +            .key_len = 0,
> +            .key  = NULL
> +        },
> +    };
> +
> +    /* Override queue array with default. */
> +    for (i = 0; i < netdev_n_rxq(netdev); i++) {
> +       rss_data->queue[i] = i;
> +    }
> +
> +    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RSS, &rss_data->conf);
> +

This empty line doesn't seem to be necessary.

> +    add_flow_action(actions, RTE_FLOW_ACTION_TYPE_END, NULL);
> +}
> +
>  static int
>  netdev_offload_dpdk_add_flow(struct netdev *netdev,
>                               const struct match *match,
> @@ -599,20 +619,12 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>          goto out;
>      }
>  
> -    struct rte_flow_action_mark mark;
> -    struct action_rss_data *rss;
> -
> -    mark.id = info->flow_mark;
> -    add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_MARK, &mark);
> -
> -    rss = add_flow_rss_action(&actions, netdev);
> -    add_flow_action(&actions, RTE_FLOW_ACTION_TYPE_END, NULL);
> +    add_flow_mark_rss_actions(&actions, info->flow_mark, netdev);
>  
>      flow = netdev_dpdk_rte_flow_create(netdev, &flow_attr,
>                                         patterns.items,
>                                         actions.actions, &error);
>  
> -    free(rss);
>      if (!flow) {
>          VLOG_ERR("%s: rte flow creat error: %u : message : %s\n",
>                   netdev_get_name(netdev), error.type, error.message);
> @@ -625,7 +637,7 @@ netdev_offload_dpdk_add_flow(struct netdev *netdev,
>  
>  out:
>      free(patterns.items);
> -    free(actions.actions);
> +    free_flow_actions(&actions);
>      return ret;
>  }
>  
>