Re: [patch net-next v4 05/10] net: sched: keep track of offloaded filters and check tc offload feature

[Jakub Kicinski]

On Sat, 23 Dec 2017 16:54:31 +0100, Jiri Pirko wrote:
> -static void tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> -                                struct tcf_block_ext_info *ei)
> +static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
> +                               struct tcf_block_ext_info *ei)
>  {
> -     tcf_block_offload_cmd(block, q, ei, TC_BLOCK_BIND);
> +     struct net_device *dev = q->dev_queue->dev;
> +     int err;
> +
> +     if (!dev->netdev_ops->ndo_setup_tc)
> +             return 0;
> +
> +     /* If tc offload feature is disabled and the block we try to bind
> +      * to already has some offloaded filters, forbid to bind.
> +      */
> +     if (!tc_can_offload(dev) && tcf_block_offload_in_use(block))
> +             return -EOPNOTSUPP;
> +
> +     err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND);
> +     if (err == -EOPNOTSUPP)
> +             /* Driver does not support binding. */
> +             return 0;
> +     return err;
>  }

Would you mind explaining why those return 0s are safe?

Say I have 2 netdevs, one dumb NIC without ndo_setup_tc (dnic) and one
NIC that can offload everything (enic).  I can share a block between
them (before or after adding any filters) and adding filters with
skip_sw will succeed, even though dnic will not see them ever.  There
is only one callback for enic, "all callbacks" != "all devices". 
It's fine to share the block in such case, but that block can never
accept a skip_sw filter.  Don't we need something like (reverse) patch 3
for keeping track of netdevs sharing the block which are not OK with
offloads?

Am I misunderstanding how this is supposed to work?  Or simply too nit
picky about providing predictable behaviour?

Quick hack to illustrate the idea (untested):

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 22a3a1d22ffa..e61e59161243 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -289,6 +289,7 @@ struct tcf_block {
        struct list_head cb_list;
        struct list_head owner_list;
        bool keep_dst;
+       bool nonoffload_taint;
        unsigned int offloadcnt;
 };
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 37eea70d1d72..4e017cbbf356 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -290,7 +290,7 @@ static int tcf_block_offload_bind(struct tcf_block *block, 
struct Qdisc *q,
        int err;
 
        if (!dev->netdev_ops->ndo_setup_tc)
-               return 0;
+               goto mark_no_offload;
 
        /* If tc offload feature is disabled and the block we try to bind
         * to already has some offloaded filters, forbid to bind.
@@ -300,9 +300,14 @@ static int tcf_block_offload_bind(struct tcf_block *block, 
struct Qdisc *q,
 
        err = tcf_block_offload_cmd(block, dev, ei, TC_BLOCK_BIND);
        if (err == -EOPNOTSUPP)
-               /* Driver does not support binding. */
-               return 0;
+               goto mark_no_offload;
        return err;
+
+mark_no_offload:
+       if (tcf_block_offload_in_use(block))
+               return -EOPNOTSUPP;
+       block->nonoffload_taint = true;
+       return 0;
 }
 
 static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
@@ -1492,6 +1497,10 @@ int tc_setup_cb_call(struct tcf_block *block, struct 
tcf_exts *exts,
        int ok_count;
        int ret;
 
+       /* Make sure all netdevs sharing this block are offload-capable */
+       if (block->nonoffload_taint && err_stop)
+               return -EOPNOTSUPP;
+
        ret = tcf_block_cb_call(block, type, type_data, err_stop);
        if (ret < 0)
                return ret;


Here a block once tainted with a bad netdev will never be offloadable
again, so tracking a'la patch 3 would be nicer..