On Fri, Mar 16, 2018 at 03:05:18PM -0700, Jakub Kicinski wrote:
> CC: David Ahern <dsah...@gmail.com>
>
> On Fri, 16 Mar 2018 16:23:08 -0300, Marcelo Ricardo Leitner wrote:
> > Currently we have the limitation that warnings cannot be reported though
> > extack. For example, when tc flower failed to get offloaded but got
> > installed on software datapath. The hardware failure is not fatal and
> > thus extack is not even shared with the driver, so the error is simply
> > omitted from any logging.
> >
> > The idea here is to allow such kind of warnings to get through and be
> > available for the sysadmin or the tool managing such commands (like Open
> > vSwitch), so that if this happens, we will have such log message in a
> > file later.
> >
> > The first patch extends extack to support more than one message and with
> > different log level (currently only error and warning). The second
> > shares extack with the drivers regardless of skip_sw.
> >
> > The iproute patch also follows.
> >
> > This kernel change is backward compatible with older iproute because
> > iproute will only process the last message, which should be the error
> > one in case of failure, or a warning if it suceeded.
> >
> > The iproute change is compatible with older kernels because it will find
> > only one message to be processed and will handle it properly.
> >
> > With this patches, this is now possible:
> > # tc qdisc add dev p7p1 ingress
> > # tc filter add dev p7p1 parent ffff: protocol ip prio 1 flower \
> > src_mac ec:13:db:00:00:00 dst_mac ec:14:c2:00:00:00 \
> > src_ip 56.0.0.0 dst_ip 55.0.0.0 action drop
> > Warning: TC offload is disabled on net device.
> > # echo $?
> > 0
>
> IMHO this set does more and less than is required to solve the
> problem.
>
> The way I understand it is we don't want HW offload errors/warnings to
> be printed to unsuspecting users who didn't specify any skip_* flags.
> What carries the message and whether it's explicitly marked as warning
> or error does not change the fact that user of the SW fwd path may not
> want to not be bothered by offload warnings.
Fair enough. We can then have a 'tc -v' option to enable this more
verbose logging.
>
> There maybe well be value in ability to report multiple messages. But
> for opt-in warning messages I would be leaning towards:
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index
> e828d31be5dae0ae8c69016dfde50379296484aa..7cec393bb47974b48a6d510b8aa84534a7a98594
> 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -705,8 +705,7 @@ tc_cls_common_offload_init(struct tc_cls_common_offload
> *cls_common,
> cls_common->chain_index = tp->chain->index;
> cls_common->protocol = tp->protocol;
> cls_common->prio = tp->prio;
> - if (tc_skip_sw(flags))
> + if (tc_skip_sw(flags) || flags & TCA_CLS_FLAGS_OFFLOAD_VERBOSE)
> cls_common->extack = extack;
> }
>
> enum tc_fl_command {
>
> That is admittedly quite conservative. Esp. in case of flower, cls_bpf
> is used in SW far more than HW, not to mention qdisc offload (although
> flag would be different there)!
Yeah, or something more generic, as a general -v / --verbose option.
M.
