[ovs-dev,2/2] tc: Support new terse dump kernel API

From: Vlad Buslov <vladbu@mellanox.com>

When dumping flows in terse mode set TCA_DUMP_FLAGS attribute to
TCA_DUMP_FLAGS_TERSE flag to prevent unnecessary copying of data between
kernel and user spaces. Only expect kernel to provide cookie, stats and
flags when dumping filters in terse mode.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
 lib/netdev-offload-tc.c |  4 ++--
 lib/tc.c                | 59 ++++++++++++++++++++++++++++++++++++++-----------
 lib/tc.h                |  5 +++--
 3 files changed, 51 insertions(+), 17 deletions(-)

On 2020-06-01 3:54 PM, Roi Dayan wrote:
> From: Vlad Buslov <vladbu@mellanox.com>
> 
> When dumping flows in terse mode set TCA_DUMP_FLAGS attribute to
> TCA_DUMP_FLAGS_TERSE flag to prevent unnecessary copying of data between
> kernel and user spaces. Only expect kernel to provide cookie, stats and
> flags when dumping filters in terse mode.
> 
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
>  lib/netdev-offload-tc.c |  4 ++--
>  lib/tc.c                | 59 ++++++++++++++++++++++++++++++++++++++-----------
>  lib/tc.h                |  5 +++--
>  3 files changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 3ba70db4690b..0ad86c448bdf 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -390,7 +390,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
>      dump->terse = terse;
>  
>      id = tc_make_tcf_id(ifindex, block_id, prio, hook);
> -    tc_dump_flower_start(&id, dump->nl_dump);
> +    tc_dump_flower_start(&id, dump->nl_dump, terse);
>  
>      *dump_out = dump;
>  
> @@ -937,7 +937,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>      while (nl_dump_next(dump->nl_dump, &nl_flow, rbuffer)) {
>          struct tc_flower flower;
>  
> -        if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower)) {
> +        if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower, dump->terse)) {
>              continue;
>          }
>  
> diff --git a/lib/tc.c b/lib/tc.c
> index 12af0192b614..d0d9d0b047f7 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -51,9 +51,14 @@
>  #define TCM_IFINDEX_MAGIC_BLOCK (0xFFFFFFFFU)
>  #endif
>  
> -#if TCA_MAX < 14
> +#ifndef TCA_DUMP_FLAGS_TERSE
> +#define TCA_DUMP_FLAGS_TERSE (1 << 0)
> +#endif
> +
> +#if TCA_MAX < 15
>  #define TCA_CHAIN 11
>  #define TCA_INGRESS_BLOCK 13
> +#define TCA_DUMP_FLAGS 15
>  #endif
>  
>  VLOG_DEFINE_THIS_MODULE(tc);
> @@ -411,6 +416,11 @@ static const struct nl_policy tca_flower_policy[] = {
>                                          .optional = true, },
>  };
>  
> +static const struct nl_policy tca_flower_terse_policy[] = {
> +    [TCA_FLOWER_FLAGS] = { .type = NL_A_U32, .optional = false, },
> +    [TCA_FLOWER_ACT] = { .type = NL_A_NESTED, .optional = false, },
> +};
> +
>  static void
>  nl_parse_flower_eth(struct nlattr **attrs, struct tc_flower *flower)
>  {
> @@ -1573,7 +1583,7 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower)
>  static const struct nl_policy act_policy[] = {
>      [TCA_ACT_KIND] = { .type = NL_A_STRING, .optional = false, },
>      [TCA_ACT_COOKIE] = { .type = NL_A_UNSPEC, .optional = true, },
> -    [TCA_ACT_OPTIONS] = { .type = NL_A_NESTED, .optional = false, },
> +    [TCA_ACT_OPTIONS] = { .type = NL_A_NESTED, .optional = true, },
>      [TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, },
>  };
>  
> @@ -1584,7 +1594,8 @@ static const struct nl_policy stats_policy[] = {
>  };
>  
>  static int
> -nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
> +nl_parse_single_action(struct nlattr *action, struct tc_flower *flower,
> +                       bool terse)
>  {
>      struct nlattr *act_options;
>      struct nlattr *act_stats;
> @@ -1597,7 +1608,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>      int err = 0;
>  
>      if (!nl_parse_nested(action, act_policy, action_attrs,
> -                         ARRAY_SIZE(act_policy))) {
> +                         ARRAY_SIZE(act_policy)) ||
> +        (!terse && !action_attrs[TCA_ACT_OPTIONS])) {
>          VLOG_ERR_RL(&error_rl, "failed to parse single action options");
>          return EPROTO;
>      }
> @@ -1606,7 +1618,9 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>      act_options = action_attrs[TCA_ACT_OPTIONS];
>      act_cookie = action_attrs[TCA_ACT_COOKIE];
>  
> -    if (!strcmp(act_kind, "gact")) {
> +    if (terse) {
> +        /* Terse dump doesn't provide act options attribute. */
> +    } else if (!strcmp(act_kind, "gact")) {
>          err = nl_parse_act_gact(act_options, flower);
>      } else if (!strcmp(act_kind, "mirred")) {
>          err = nl_parse_act_mirred(act_options, flower);
> @@ -1656,7 +1670,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower)
>  #define TCA_ACT_MIN_PRIO 1
>  
>  static int
> -nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower)
> +nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower,
> +                        bool terse)
>  {
>      const struct nlattr *actions = attrs[TCA_FLOWER_ACT];
>      static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {};
> @@ -1682,7 +1697,7 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower)
>                  VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM);
>                  return EOPNOTSUPP;
>              }
> -            err = nl_parse_single_action(actions_orders[i], flower);
> +            err = nl_parse_single_action(actions_orders[i], flower, terse);
>  
>              if (err) {
>                  return err;
> @@ -1701,11 +1716,21 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower)
>  }
>  
>  static int
> -nl_parse_flower_options(struct nlattr *nl_options, struct tc_flower *flower)
> +nl_parse_flower_options(struct nlattr *nl_options, struct tc_flower *flower,
> +                        bool terse)
>  {
>      struct nlattr *attrs[ARRAY_SIZE(tca_flower_policy)];
>      int err;
>  
> +    if (terse) {
> +        if (!nl_parse_nested(nl_options, tca_flower_terse_policy,
> +                             attrs, ARRAY_SIZE(tca_flower_terse_policy))) {
> +            VLOG_ERR_RL(&error_rl, "failed to parse flower classifier terse options");
> +            return EPROTO;
> +        }
> +        goto skip_flower_opts;
> +    }
> +
>      if (!nl_parse_nested(nl_options, tca_flower_policy,
>                           attrs, ARRAY_SIZE(tca_flower_policy))) {
>          VLOG_ERR_RL(&error_rl, "failed to parse flower classifier options");
> @@ -1721,13 +1746,14 @@ nl_parse_flower_options(struct nlattr *nl_options, struct tc_flower *flower)
>          return err;
>      }
>  
> +skip_flower_opts:
>      nl_parse_flower_flags(attrs, flower);
> -    return nl_parse_flower_actions(attrs, flower);
> +    return nl_parse_flower_actions(attrs, flower, terse);
>  }
>  
>  int
>  parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tcf_id *id,
> -                           struct tc_flower *flower)
> +                           struct tc_flower *flower, bool terse)
>  {
>      struct tcmsg *tc;
>      struct nlattr *ta[ARRAY_SIZE(tca_policy)];
> @@ -1770,15 +1796,22 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tcf_id *id,
>          return EPROTO;
>      }
>  
> -    return nl_parse_flower_options(ta[TCA_OPTIONS], flower);
> +    return nl_parse_flower_options(ta[TCA_OPTIONS], flower, terse);
>  }
>  
>  int
> -tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump)
> +tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse)
>  {
>      struct ofpbuf request;
>  
>      request_from_tcf_id(id, 0, RTM_GETTFILTER, NLM_F_DUMP, &request);
> +    if (terse) {
> +        struct nla_bitfield32 dump_flags = { TCA_DUMP_FLAGS_TERSE,
> +                                             TCA_DUMP_FLAGS_TERSE };
> +
> +        nl_msg_put_unspec(&request, TCA_DUMP_FLAGS, &dump_flags,
> +                          sizeof dump_flags);
> +    }
>      nl_dump_start(dump, NETLINK_ROUTE, &request);
>      ofpbuf_uninit(&request);
>  
> @@ -1807,7 +1840,7 @@ tc_get_flower(struct tcf_id *id, struct tc_flower *flower)
>          return error;
>      }
>  
> -    error = parse_netlink_to_tc_flower(reply, id, flower);
> +    error = parse_netlink_to_tc_flower(reply, id, flower, false);
>      ofpbuf_delete(reply);
>      return error;
>  }
> diff --git a/lib/tc.h b/lib/tc.h
> index 24a4994fd17e..11f3231f9dfe 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -341,10 +341,11 @@ BUILD_ASSERT_DECL(offsetof(struct tc_flower, rewrite)
>  int tc_replace_flower(struct tcf_id *id, struct tc_flower *flower);
>  int tc_del_filter(struct tcf_id *id);
>  int tc_get_flower(struct tcf_id *id, struct tc_flower *flower);
> -int tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump);
> +int tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse);
>  int parse_netlink_to_tc_flower(struct ofpbuf *reply,
>                                 struct tcf_id *id,
> -                               struct tc_flower *flower);
> +                               struct tc_flower *flower,
> +                               bool terse);
>  void tc_set_policy(const char *policy);
>  
>  #endif /* tc.h */
> 

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

Bleep bloop.  Greetings Roi Dayan, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 86 characters long (recommended limit is 79)
#146 FILE: lib/tc.c:1728:
            VLOG_ERR_RL(&error_rl, "failed to parse flower classifier terse options");

Lines checked: 226, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot