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

[Jiri Pirko <jiri@resnulli.us>]

Sun, Dec 24, 2017 at 03:20:45AM CET, kubakici@wp.pl wrote:
>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?

You undestand it correctly. Original plan was to ignore thore devices
that does not support offloading. But thinking about it a bit more,
you are probably right that they should be taken into consideration
when user explicitly says "skip_sw".

Will include the accounting you suggest below. Thanks!


>
>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..