| Message ID | cover.1521226621.git.marcelo.leitner@gmail.com |
|---|---|
| Headers | show |
| Series | Add support for warnings to extack | expand |
On Fri, Mar 16, 2018 at 04:23:08PM -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 Will have more and better examples once we actually have more than one message being added. > > Marcelo Ricardo Leitner (2): > netlink: extend extack so it can carry more than one message > sched: pass extack through even if skip_sw is not set > > include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- > include/net/pkt_cls.h | 3 +-- > net/netlink/af_netlink.c | 12 +++++++----- > 3 files changed, 38 insertions(+), 27 deletions(-) > > -- > 2.14.3 >
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. 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)!
On 3/16/18 1:23 PM, 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. If this set ends up moving forward, the above statement needs to be corrected: extack allows non-error messages to be sent back to the user, so the above must be talking about some other limitation local to tc.
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.
On Sun, Mar 18, 2018 at 10:11:52AM -0600, David Ahern wrote: > On 3/16/18 1:23 PM, 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. > > If this set ends up moving forward, the above statement needs to be > corrected: extack allows non-error messages to be sent back to the user, > so the above must be talking about some other limitation local to tc. Right. Thanks, Marcelo
On Sun, Mar 18, 2018 at 10:11:52AM -0600, David Ahern wrote: > On 3/16/18 1:23 PM, 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. > > If this set ends up moving forward, the above statement needs to be > corrected: extack allows non-error messages to be sent back to the user, > so the above must be talking about some other limitation local to tc. I'll split this patchset into two: - pass extack to drivers when doing tc offload (such as flower and others) and allow reporting of warnings in such cases. (according to a opt-in flag, as discussed in the other subthread) - allow more than one message The 1st may lead to the 2nd but right now it's more as a supposition, as there is no actual user for it yet.