Re: [PATCH net-next v4 6/8] net: sched: create tc_can_offload_extack() wrapper

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Sat, Jan 20, 2018 at 2:54 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Sat, Jan 20, 2018 at 11:33:31AM CET, jakub.kicinski@netronome.com wrote:
>>On Sat, Jan 20, 2018 at 2:22 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> Sat, Jan 20, 2018 at 11:12:25AM CET, jakub.kicinski@netronome.com wrote:
>>>>net/sched/sch_prio.c:   if (!tc_can_offload(dev) ||
>>>>!dev->netdev_ops->ndo_setup_tc)
>>>>net/sched/sch_prio.c:   if (!tc_can_offload(dev) ||
>>>>!dev->netdev_ops->ndo_setup_tc)
>>>>net/sched/sch_red.c:    if (!tc_can_offload(dev) ||
>>>>!dev->netdev_ops->ndo_setup_tc)
>>>>net/sched/sch_red.c:    if (!tc_can_offload(dev) ||
>>>>!dev->netdev_ops->ndo_setup_tc)
>>>>
>>>>Do you mean the qdisc offloads too?  The whole lot?
>>>
>>> Yes.
>>
>>Actually looking at the qdisc code and destroy callbacks, if we plumb
>>it through everywhere won't that mean user will see error messages on
>>destroy of qdiscs/filters which were never offloaded?
>>
>>Just looking at prio_offload() as a simple example.  prio_destroy()
>>will always call tc_can_offload().
>
> Hmmm. You are right.
> Either we pass null from there (NL_SET_ERR_MSG can cope), or we leave
> tc_can_offload helper as is and let the caller to set the extack.

I'm afraid it doesn't stop there though :/  Even now if one install a
filter on a netdev with offload abilities and no skip_* flag there is
this:

# tc filter add dev eth0 ingress bpf obj drop.o da
Warning: TC offload is disabled on net device.

I'm not sure we should warn the user if there are no skip_* flags,
just because the device has *some* offload capabilities?   Perhaps we
should put the flags into tc_cls_common_offload and add a helper to
only set extack if skip_sw is set..

> I see that tc_can_offload_extack is probably good idea then.
>
> But please use it in all drivers that are calling tc_can_offload so the
> user experience is consistent for all.

Certainly, will do.