Re: [PATCH RFC 0/2] Add support for warnings to extack

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Fri, Mar 16, 2018 at 03:05:18PM -0700, Jakub Kicinski wrote:
> CC: David Ahern <dsahern@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.