[ovs-dev] netdev-offload-tc: Flush rules on all chains before attach ingress block

From: Jianbo Liu <jianbol@nvidia.com>

Previously, only chain 0 is deleted before attach the ingress block.
If there are rules on the chain other than 0, these rules are not flushed.
In this case, the recreation of qdisc also fails. To fix this issue, flush
rules from all chains.

Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 lib/netdev-offload-tc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++---
 lib/tc.c                | 39 +++++++++++++++++++++++
 lib/tc.h                |  2 ++
 3 files changed, 121 insertions(+), 5 deletions(-)

Hi,

Any comments or thoughts on this patch?

Thanks,
Roi


On 2021-01-26 9:17 AM, Roi Dayan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> Previously, only chain 0 is deleted before attach the ingress block.
> If there are rules on the chain other than 0, these rules are not flushed.
> In this case, the recreation of qdisc also fails. To fix this issue, flush
> rules from all chains.
> 
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> ---
>   lib/netdev-offload-tc.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++---
>   lib/tc.c                | 39 +++++++++++++++++++++++
>   lib/tc.h                |  2 ++
>   3 files changed, 121 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 717a987d14d8..bd7aa7725ad3 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -55,6 +55,11 @@ struct netlink_field {
>       int size;
>   };
>   
> +struct chain_node {
> +    struct hmap_node node;
> +    uint32_t chain;
> +};
> +
>   static bool
>   is_internal_port(const char *type)
>   {
> @@ -345,6 +350,69 @@ get_block_id_from_netdev(struct netdev *netdev)
>   }
>   
>   static int
> +get_chains_from_netdev(struct netdev *netdev, struct tcf_id *id,
> +                       struct hmap *map)
> +{
> +    struct netdev_flow_dump *dump;
> +    struct chain_node *chain_node;
> +    struct ofpbuf rbuffer, reply;
> +    uint32_t chain;
> +    size_t hash;
> +    int err;
> +
> +    dump = xzalloc(sizeof *dump);
> +    dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
> +    dump->netdev = netdev_ref(netdev);
> +
> +    ofpbuf_init(&rbuffer, NL_DUMP_BUFSIZE);
> +    tc_dump_tc_chain_start(id, dump->nl_dump);
> +
> +    while (nl_dump_next(dump->nl_dump, &reply, &rbuffer)) {
> +        if (parse_netlink_to_tc_chain(&reply, &chain)) {
> +            continue;
> +        }
> +
> +        chain_node = xzalloc(sizeof *chain_node);
> +        chain_node->chain = chain;
> +        hash = hash_int(chain, 0);
> +        hmap_insert(map, &chain_node->node, hash);
> +    }
> +
> +    err = nl_dump_done(dump->nl_dump);
> +    ofpbuf_uninit(&rbuffer);
> +    netdev_close(netdev);
> +    free(dump->nl_dump);
> +    free(dump);
> +
> +    return err;
> +}
> +
> +static int
> +delete_chains_from_netdev(struct netdev *netdev, struct tcf_id *id)
> +{
> +    struct chain_node *chain_node;
> +    struct hmap map;
> +    int error;
> +
> +    hmap_init(&map);
> +    error = get_chains_from_netdev(netdev, id, &map);
> +
> +    if (!error) {
> +        /* Flush rules explicitly needed when we work with ingress_block,
> +         * so we will not fail with reattaching block to bond iface, for ex.
> +         */
> +        HMAP_FOR_EACH_POP (chain_node, node, &map) {
> +            id->chain = chain_node->chain;
> +            tc_del_filter(id);
> +            free(chain_node);
> +        }
> +    }
> +
> +    hmap_destroy(&map);
> +    return error;
> +}
> +
> +static int
>   netdev_tc_flow_flush(struct netdev *netdev)
>   {
>       struct ufid_tc_data *data, *next;
> @@ -2008,6 +2076,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>   {
>       static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>       enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
> +    static bool get_chain_supported = true;
>       uint32_t block_id = 0;
>       struct tcf_id id;
>       int ifindex;
> @@ -2021,12 +2090,18 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>       }
>   
>       block_id = get_block_id_from_netdev(netdev);
> -
> -    /* Flush rules explicitly needed when we work with ingress_block,
> -     * so we will not fail with reattaching block to bond iface, for ex.
> -     */
>       id = tc_make_tcf_id(ifindex, block_id, 0, hook);
> -    tc_del_filter(&id);
> +
> +    if (get_chain_supported) {
> +        if (delete_chains_from_netdev(netdev, &id)) {
> +            get_chain_supported = false;
> +        }
> +    }
> +
> +    /* fallback here if delete chains fail */
> +    if (!get_chain_supported) {
> +        tc_del_filter(&id);
> +    }
>   
>       /* make sure there is no ingress/egress qdisc */
>       tc_add_del_qdisc(ifindex, false, 0, hook);
> diff --git a/lib/tc.c b/lib/tc.c
> index c2de78bfe347..74a8113eaa0f 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -61,6 +61,10 @@
>   #define TCA_DUMP_FLAGS 15
>   #endif
>   
> +#ifndef RTM_GETCHAIN
> +#define RTM_GETCHAIN 102
> +#endif
> +
>   VLOG_DEFINE_THIS_MODULE(tc);
>   
>   static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 5);
> @@ -297,6 +301,10 @@ static const struct nl_policy tca_policy[] = {
>       [TCA_STATS2] = { .type = NL_A_NESTED, .optional = true, },
>   };
>   
> +static const struct nl_policy tca_chain_policy[] = {
> +    [TCA_CHAIN] = { .type = NL_A_U32, .optional = false, },
> +};
> +
>   static const struct nl_policy tca_flower_policy[] = {
>       [TCA_FLOWER_CLASSID] = { .type = NL_A_U32, .optional = true, },
>       [TCA_FLOWER_INDEV] = { .type = NL_A_STRING, .max_len = IFNAMSIZ,
> @@ -1864,6 +1872,25 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tcf_id *id,
>   }
>   
>   int
> +parse_netlink_to_tc_chain(struct ofpbuf *reply, uint32_t *chain)
> +{
> +    struct nlattr *ta[ARRAY_SIZE(tca_chain_policy)];
> +    struct tcmsg *tc;
> +
> +    tc = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);
> +
> +    if (!nl_policy_parse(reply, NLMSG_HDRLEN + sizeof *tc,
> +                         tca_chain_policy, ta, ARRAY_SIZE(ta))) {
> +        VLOG_ERR_RL(&error_rl, "failed to parse tca chain policy");
> +        return EINVAL;
> +    }
> +
> +   *chain = nl_attr_get_u32(ta[TCA_CHAIN]);
> +
> +    return 0;
> +}
> +
> +int
>   tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse)
>   {
>       struct ofpbuf request;
> @@ -1883,6 +1910,18 @@ tc_dump_flower_start(struct tcf_id *id, struct nl_dump *dump, bool terse)
>   }
>   
>   int
> +tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump)
> +{
> +    struct ofpbuf request;
> +
> +    request_from_tcf_id(id, 0, RTM_GETCHAIN, NLM_F_DUMP, &request);
> +    nl_dump_start(dump, NETLINK_ROUTE, &request);
> +    ofpbuf_uninit(&request);
> +
> +    return 0;
> +}
> +
> +int
>   tc_del_filter(struct tcf_id *id)
>   {
>       struct ofpbuf request;
> diff --git a/lib/tc.h b/lib/tc.h
> index 281231c0d3f1..d4a304ea6faa 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -360,10 +360,12 @@ 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, bool terse);
> +int tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump);
>   int parse_netlink_to_tc_flower(struct ofpbuf *reply,
>                                  struct tcf_id *id,
>                                  struct tc_flower *flower,
>                                  bool terse);
> +int parse_netlink_to_tc_chain(struct ofpbuf *reply, uint32_t *chain);
>   void tc_set_policy(const char *policy);
>   
>   #endif /* tc.h */
>

On Tue, Feb 23, 2021 at 09:53:10AM +0200, Roi Dayan wrote:
> Hi,
> 
> Any comments or thoughts on this patch?
> 
> Thanks,
> Roi

Thanks, and sorry for the extended delay.

This looks good to me. And as there has been no other activity
around this patch I've taken the liberty of pushing it to master.