[PATCH net-next] sched: cls: enable verbose logging

[PATCH net-next] sched: cls: enable verbose logging

[Marcelo Ricardo Leitner]

Currently, when the rule is not to be exclusively executed by the
hardware, extack is not passed along and offloading failures don't
get logged. The idea was that hardware failures are okay because the
rule will get executed in software then and this way it doesn't confuse
unware users.

But this is not helpful in case one needs to understand why a certain
rule failed to get offloaded. Considering it may have been a temporary
failure, like resources exceeded or so, reproducing it later and knowing
that it is triggering the same reason may be challenging.

The ultimate goal is to improve Open vSwitch debuggability when using
flower offloading.

This patch adds a new flag to enable verbose logging. With the flag set,
extack will be passed to the driver, which will be able to log the
error. As the operation itself probably won't fail (not because of this,
at least), current iproute will already log it as a Warning.

The flag is generic, so it can be reused later. No need to restrict it
just for HW offloading. The command line will follow the syntax that
tc-ebpf already uses, tc ... [ verbose ] ... , and extend its meaning.

For example:
# ./tc qdisc add dev p7p1 ingress
# ./tc filter add dev p7p1 parent ffff: protocol ip prio 1 \
	flower verbose \
	src_mac ed:13:db:00:00:00 dst_mac 01:80:c2:00:00:d0 \
	src_ip 56.0.0.0 dst_ip 55.0.0.0 action drop
Warning: TC offload is disabled on net device.
# echo $?
0
# ./tc filter add dev p7p1 parent ffff: protocol ip prio 1 \
	flower \
	src_mac ff:13:db:00:00:00 dst_mac 01:80:c2:00:00:d0 \
	src_ip 56.0.0.0 dst_ip 55.0.0.0 action drop
# echo $?
0

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---

Initial RFC was posted with subject "[PATCH RFC 0/2] Add support for
warnings to extack".

Changes since it:
- Use only a single error/warning message (instead of adding support to
  multiple messages)
- Make it opt-in, as suggested by Jakub Kicinski.
- Enhanced changelog, as per David Ahern comment.

 include/net/pkt_cls.h        | 6 ++++--
 include/uapi/linux/pkt_cls.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index e828d31be5dae0ae8c69016dfde50379296484aa..0005f0b40fe9310d8160018b3baaccf2cc098c4d 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -683,9 +683,11 @@ static inline bool tc_skip_sw(u32 flags)
 /* SKIP_HW and SKIP_SW are mutually exclusive flags. */
 static inline bool tc_flags_valid(u32 flags)
 {
-	if (flags & ~(TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW))
+	if (flags & ~(TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW |
+		      TCA_CLS_FLAGS_VERBOSE))
 		return false;

+	flags &= TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW;
 	if (!(flags ^ (TCA_CLS_FLAGS_SKIP_HW | TCA_CLS_FLAGS_SKIP_SW)))
 		return false;

@@ -705,7 +707,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_VERBOSE)
 		cls_common->extack = extack;
 }

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index be05e66c167b12a70db409242519a9b1958b1000..84e4c1d0f874afec5891fcf95def286c121f71ed 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -129,6 +129,7 @@ enum {
 #define TCA_CLS_FLAGS_SKIP_SW	(1 << 1) /* don't use filter in SW */
 #define TCA_CLS_FLAGS_IN_HW	(1 << 2) /* filter is offloaded to HW */
 #define TCA_CLS_FLAGS_NOT_IN_HW (1 << 3) /* filter isn't offloaded to HW */
+#define TCA_CLS_FLAGS_VERBOSE	(1 << 4) /* verbose logging */

 /* U32 filters */

[PATCH iproute2-next] tc: flower: add support for verbose logging

[Marcelo Ricardo Leitner]

From: Marcelo Ricardo Leitner <mleitner@redhat.com>

Currently there is no way to log offloading errors if the rule is not
explicitly marked as skip_sw, making it hard for other applications such
as Open vSwitch to log why a given could not be offloaded.

This patch adds support for signaling the kernel that more verbose
logging is wanted, which now will include such messages.

Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
---
 include/uapi/linux/pkt_cls.h | 1 +
 man/man8/tc-flower.8         | 7 +++++++
 tc/f_flower.c                | 4 +++-
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index be05e66c167b12a70db409242519a9b1958b1000..84e4c1d0f874afec5891fcf95def286c121f71ed 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -129,6 +129,7 @@ enum {
 #define TCA_CLS_FLAGS_SKIP_SW	(1 << 1) /* don't use filter in SW */
 #define TCA_CLS_FLAGS_IN_HW	(1 << 2) /* filter is offloaded to HW */
 #define TCA_CLS_FLAGS_NOT_IN_HW (1 << 3) /* filter isn't offloaded to HW */
+#define TCA_CLS_FLAGS_VERBOSE	(1 << 4) /* verbose logging */
 
 /* U32 filters */
 
diff --git a/man/man8/tc-flower.8 b/man/man8/tc-flower.8
index a561443b9978bee55142f7dfed6b46106f7b9baf..4f3714b79544b33f198d711fa9006806685d43a8 100644
--- a/man/man8/tc-flower.8
+++ b/man/man8/tc-flower.8
@@ -22,6 +22,8 @@ flower \- flow based traffic control filter
 .IR MATCH " := { "
 .B indev
 .IR ifname " | "
+.BR verbose
+.RI " | "
 .BR skip_sw " | " skip_hw
 .RI " | { "
 .BR dst_mac " | " src_mac " } "
@@ -100,6 +102,11 @@ is the name of an interface which must exist at the time of
 .B tc
 invocation.
 .TP
+.BI verbose
+Enable verbose logging, including offloading errors when not using
+.B skip_sw
+flag.
+.TP
 .BI skip_sw
 Do not process filter by software. If hardware has no offload support for this
 filter, or TC offload is not enabled for the interface, operation will fail.
diff --git a/tc/f_flower.c b/tc/f_flower.c
index ba8eb66cdd111adf4f68f041fb6c4acc48fd1a90..c710765179fb08ac94e04ec664fac0d30cf04931 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -41,7 +41,7 @@ enum flower_icmp_field {
 static void explain(void)
 {
 	fprintf(stderr,
-		"Usage: ... flower [ MATCH-LIST ]\n"
+		"Usage: ... flower [ MATCH-LIST ] [ verbose ]\n"
 		"                  [ skip_sw | skip_hw ]\n"
 		"                  [ action ACTION-SPEC ] [ classid CLASSID ]\n"
 		"\n"
@@ -648,6 +648,8 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
 				fprintf(stderr, "Illegal \"ip_flags\"\n");
 				return -1;
 			}
+		} else if (matches(*argv, "verbose") == 0) {
+			flags |= TCA_CLS_FLAGS_VERBOSE;
 		} else if (matches(*argv, "skip_hw") == 0) {
 			flags |= TCA_CLS_FLAGS_SKIP_HW;
 		} else if (matches(*argv, "skip_sw") == 0) {
-- 
2.14.3

Re: [PATCH iproute2-next] tc: flower: add support for verbose logging

[David Ahern]

On 5/13/18 2:44 PM, Marcelo Ricardo Leitner wrote:
> From: Marcelo Ricardo Leitner <mleitner@redhat.com>
> 
> Currently there is no way to log offloading errors if the rule is not
> explicitly marked as skip_sw, making it hard for other applications such
> as Open vSwitch to log why a given could not be offloaded.
> 
> This patch adds support for signaling the kernel that more verbose
> logging is wanted, which now will include such messages.
> 
> Signed-off-by: Marcelo Ricardo Leitner <mleitner@redhat.com>
> ---
>  include/uapi/linux/pkt_cls.h | 1 +
>  man/man8/tc-flower.8         | 7 +++++++
>  tc/f_flower.c                | 4 +++-
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 

applied to iproute2-next. Thanks

Re: [PATCH net-next] sched: cls: enable verbose logging

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Sun, 13 May 2018 17:44:27 -0300

> Currently, when the rule is not to be exclusively executed by the
> hardware, extack is not passed along and offloading failures don't
> get logged. The idea was that hardware failures are okay because the
> rule will get executed in software then and this way it doesn't confuse
> unware users.
> 
> But this is not helpful in case one needs to understand why a certain
> rule failed to get offloaded. Considering it may have been a temporary
> failure, like resources exceeded or so, reproducing it later and knowing
> that it is triggering the same reason may be challenging.
> 
> The ultimate goal is to improve Open vSwitch debuggability when using
> flower offloading.
> 
> This patch adds a new flag to enable verbose logging. With the flag set,
> extack will be passed to the driver, which will be able to log the
> error. As the operation itself probably won't fail (not because of this,
> at least), current iproute will already log it as a Warning.
> 
> The flag is generic, so it can be reused later. No need to restrict it
> just for HW offloading. The command line will follow the syntax that
> tc-ebpf already uses, tc ... [ verbose ] ... , and extend its meaning.
> 
> For example:
> # ./tc qdisc add dev p7p1 ingress
> # ./tc filter add dev p7p1 parent ffff: protocol ip prio 1 \
> 	flower verbose \
> 	src_mac ed:13:db:00:00:00 dst_mac 01:80:c2:00:00:d0 \
> 	src_ip 56.0.0.0 dst_ip 55.0.0.0 action drop
> Warning: TC offload is disabled on net device.
> # echo $?
> 0
> # ./tc filter add dev p7p1 parent ffff: protocol ip prio 1 \
> 	flower \
> 	src_mac ff:13:db:00:00:00 dst_mac 01:80:c2:00:00:d0 \
> 	src_ip 56.0.0.0 dst_ip 55.0.0.0 action drop
> # echo $?
> 0
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Applied, thank you.

Re: [PATCH net-next] sched: cls: enable verbose logging

[Cong Wang]

On Sun, May 13, 2018 at 1:44 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> Currently, when the rule is not to be exclusively executed by the
> hardware, extack is not passed along and offloading failures don't
> get logged. The idea was that hardware failures are okay because the
> rule will get executed in software then and this way it doesn't confuse
> unware users.
>
> But this is not helpful in case one needs to understand why a certain
> rule failed to get offloaded. Considering it may have been a temporary
> failure, like resources exceeded or so, reproducing it later and knowing
> that it is triggering the same reason may be challenging.

I fail to understand why you need a flag here, IOW, why not just pass
extack unconditionally?

Re: [PATCH net-next] sched: cls: enable verbose logging

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Mon, 14 May 2018 13:30:53 -0700

> I fail to understand why you need a flag here, IOW, why not just pass
> extack unconditionally?

It will confuse users, so isn't passed up by default.

Re: [PATCH net-next] sched: cls: enable verbose logging

[Marcelo Ricardo Leitner]

On Mon, May 14, 2018 at 01:30:53PM -0700, Cong Wang wrote:
> On Sun, May 13, 2018 at 1:44 PM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > Currently, when the rule is not to be exclusively executed by the
> > hardware, extack is not passed along and offloading failures don't
> > get logged. The idea was that hardware failures are okay because the
> > rule will get executed in software then and this way it doesn't confuse
> > unware users.
> >
> > But this is not helpful in case one needs to understand why a certain
> > rule failed to get offloaded. Considering it may have been a temporary
> > failure, like resources exceeded or so, reproducing it later and knowing
> > that it is triggering the same reason may be challenging.
>
> I fail to understand why you need a flag here, IOW, why not just pass
> extack unconditionally?

Because (as discussed in the RFC[1], should have linked it here) it
could confuse users that are not aware of offloading and, in other
cases, it can be just noise (like it would be right now for ebpf,
which is mostly used in sw-path).

1.https://www.mail-archive.com/netdev@vger.kernel.org/msg223016.html

Re: [PATCH net-next] sched: cls: enable verbose logging

[Cong Wang]

On Mon, May 14, 2018 at 1:47 PM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Mon, May 14, 2018 at 01:30:53PM -0700, Cong Wang wrote:
>> On Sun, May 13, 2018 at 1:44 PM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>> > Currently, when the rule is not to be exclusively executed by the
>> > hardware, extack is not passed along and offloading failures don't
>> > get logged. The idea was that hardware failures are okay because the
>> > rule will get executed in software then and this way it doesn't confuse
>> > unware users.
>> >
>> > But this is not helpful in case one needs to understand why a certain
>> > rule failed to get offloaded. Considering it may have been a temporary
>> > failure, like resources exceeded or so, reproducing it later and knowing
>> > that it is triggering the same reason may be challenging.
>>
>> I fail to understand why you need a flag here, IOW, why not just pass
>> extack unconditionally?
>
> Because (as discussed in the RFC[1], should have linked it here) it
> could confuse users that are not aware of offloading and, in other
> cases, it can be just noise (like it would be right now for ebpf,
> which is mostly used in sw-path).
>
> 1.https://www.mail-archive.com/netdev@vger.kernel.org/msg223016.html

My point is that a TC filter flag should be used for a filter attribute,
logging is apparently not a part of filter. At least, put it into HW offloading,
not in TC filter.

I know DaveM hates module parameters, but a module parameter here
is more suitable than a TC filter flag.

Re: [PATCH net-next] sched: cls: enable verbose logging

[Jakub Kicinski]

On Mon, 14 May 2018 22:31:46 -0700, Cong Wang wrote:
> On Mon, May 14, 2018 at 1:47 PM, Marcelo Ricardo Leitner wrote:
> > On Mon, May 14, 2018 at 01:30:53PM -0700, Cong Wang wrote:  
> >> On Sun, May 13, 2018 at 1:44 PM, Marcelo Ricardo Leitner wrote:  
> >> > Currently, when the rule is not to be exclusively executed by the
> >> > hardware, extack is not passed along and offloading failures don't
> >> > get logged. The idea was that hardware failures are okay because the
> >> > rule will get executed in software then and this way it doesn't confuse
> >> > unware users.
> >> >
> >> > But this is not helpful in case one needs to understand why a certain
> >> > rule failed to get offloaded. Considering it may have been a temporary
> >> > failure, like resources exceeded or so, reproducing it later and knowing
> >> > that it is triggering the same reason may be challenging.  
> >>
> >> I fail to understand why you need a flag here, IOW, why not just pass
> >> extack unconditionally?  
> >
> > Because (as discussed in the RFC[1], should have linked it here) it
> > could confuse users that are not aware of offloading and, in other
> > cases, it can be just noise (like it would be right now for ebpf,
> > which is mostly used in sw-path).
> >
> > 1.https://www.mail-archive.com/netdev@vger.kernel.org/msg223016.html  
> 
> My point is that a TC filter flag should be used for a filter attribute,
> logging is apparently not a part of filter. At least, put it into HW offloading,
> not in TC filter.
> 
> I know DaveM hates module parameters, but a module parameter here
> is more suitable than a TC filter flag.

Do you mean we should add a global cls_flower parameter to enable
verbose HW offload messages?  I'm not sure where "HW offloading" is.

I agree with you in principle, this could be made a "per application
context" flag.  Perhaps to be set on the socket.  But our existing
flags are per-request so it makes sense to do the same here IMHO.