Sun, Dec 24, 2017 at 03:20:45AM CET, kubak...@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..
