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

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

[Marcelo Ricardo Leitner]

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

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

[PATCH RFC iproute2] libnetlink: allow reading more than one message from extack

[Marcelo Ricardo Leitner]

This patch introduces support for reading more than one message from
extack's and to adjust their level (warning/error) accordingly.

Yes, there is a FIXME tag in the callback call for now.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 15 deletions(-)

diff --git a/lib/libnetlink.c b/lib/libnetlink.c
index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644
--- a/lib/libnetlink.c
+++ b/lib/libnetlink.c
@@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = {
 	[NLMSGERR_ATTR_OFFS]	= MNL_TYPE_U32,
 };
 
+#define NETLINK_MAX_EXTACK_MSGS 8
+struct extack_args {
+	const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS];
+	const struct nlattr *offs;
+	int msg_count;
+};
+
 static int err_attr_cb(const struct nlattr *attr, void *data)
 {
-	const struct nlattr **tb = data;
+	struct extack_args *tb = data;
 	uint16_t type;
 
 	if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) {
@@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data)
 		return MNL_CB_ERROR;
 	}
 
-	tb[type] = attr;
+	if (type == NLMSGERR_ATTR_OFFS)
+		tb->offs = attr;
+	else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS)
+		tb->msg[tb->msg_count++] = attr;
 	return MNL_CB_OK;
 }
 
 /* dump netlink extended ack error message */
 int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
 {
-	struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {};
+	struct extack_args tb = {};
 	const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh);
 	const struct nlmsghdr *err_nlh = NULL;
 	unsigned int hlen = sizeof(*err);
-	const char *msg = NULL;
+	const char *msg[NETLINK_MAX_EXTACK_MSGS] = {};
 	uint32_t off = 0;
+	int ret, i;
 
 	/* no TLVs, nothing to do here */
 	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS))
@@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
 	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
 		hlen += mnl_nlmsg_get_payload_len(&err->msg);
 
-	if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK)
+	if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK)
 		return 0;
 
-	if (tb[NLMSGERR_ATTR_MSG])
-		msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]);
+	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++)
+		msg[i] = mnl_attr_get_str(tb.msg[i]);
 
-	if (tb[NLMSGERR_ATTR_OFFS]) {
-		off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
+	if (tb.offs) {
+		off = mnl_attr_get_u32(tb.offs);
 
 		if (off > nlh->nlmsg_len) {
 			fprintf(stderr,
@@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
 	}
 
 	if (errfn)
-		return errfn(msg, off, err_nlh);
+		return errfn(*msg, off, err_nlh); /* FIXME */
 
-	if (msg && *msg != '\0') {
+	ret = 0;
+	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) {
 		bool is_err = !!err->error;
+		const char *_msg = msg[i];
+
+		/* Message tagging has precedence.
+		 * KERN_WARNING = ASCII Start Of Header ('\001') + '4'
+		 */
+		if (!strncmp(_msg, "\0014", 2)) {
+			is_err = false;
+			_msg += 2;
+		}
+		/* But we can't have Error if it didn't fail. */
+		if (is_err && !err->error)
+			is_err = 0;
 
 		fprintf(stderr, "%s: %s",
-			is_err ? "Error" : "Warning", msg);
-		if (msg[strlen(msg) - 1] != '.')
+			is_err ? "Error" : "Warning", _msg);
+		if (_msg[strlen(_msg) - 1] != '.')
 			fprintf(stderr, ".");
 		fprintf(stderr, "\n");
 
-		return is_err ? 1 : 0;
+		if (is_err)
+			ret = 1;
 	}
 
-	return 0;
+	return ret;
 }
 #else
 #warning "libmnl required for error support"
-- 
2.14.3

Re: [PATCH RFC iproute2] libnetlink: allow reading more than one message from extack

[Stephen Hemminger]

On Fri, 16 Mar 2018 16:23:09 -0300
Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:

> This patch introduces support for reading more than one message from
> extack's and to adjust their level (warning/error) accordingly.
> 
> Yes, there is a FIXME tag in the callback call for now.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Make sense, can hold off until kernel supports warnings.

> ---
>  lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644
> --- a/lib/libnetlink.c
> +++ b/lib/libnetlink.c
> @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = {
>  	[NLMSGERR_ATTR_OFFS]	= MNL_TYPE_U32,
>  };
>  
> +#define NETLINK_MAX_EXTACK_MSGS 8

Would rather not have fixed maximums

> +struct extack_args {
> +	const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS];
> +	const struct nlattr *offs;
> +	int msg_count;
> +};

If you put msg[] last in structure, it could be variable length.

>  static int err_attr_cb(const struct nlattr *attr, void *data)
>  {
> -	const struct nlattr **tb = data;
> +	struct extack_args *tb = data;
>  	uint16_t type;
>  
>  	if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) {
> @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data)
>  		return MNL_CB_ERROR;
>  	}
>  
> -	tb[type] = attr;
> +	if (type == NLMSGERR_ATTR_OFFS)
> +		tb->offs = attr;
> +	else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS)
> +		tb->msg[tb->msg_count++] = attr;
>  	return MNL_CB_OK;
>  }
>  
>  /* dump netlink extended ack error message */
>  int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
>  {
> -	struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {};
> +	struct extack_args tb = {};
>  	const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh);
>  	const struct nlmsghdr *err_nlh = NULL;
>  	unsigned int hlen = sizeof(*err);
> -	const char *msg = NULL;
> +	const char *msg[NETLINK_MAX_EXTACK_MSGS] = {};
>  	uint32_t off = 0;
> +	int ret, i;
>  
>  	/* no TLVs, nothing to do here */
>  	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS))
> @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
>  	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
>  		hlen += mnl_nlmsg_get_payload_len(&err->msg);
>  
> -	if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK)
> +	if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK)
>  		return 0;
>  
> -	if (tb[NLMSGERR_ATTR_MSG])
> -		msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]);
> +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++)
> +		msg[i] = mnl_attr_get_str(tb.msg[i]);
>  
> -	if (tb[NLMSGERR_ATTR_OFFS]) {
> -		off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
> +	if (tb.offs) {
> +		off = mnl_attr_get_u32(tb.offs);
>  
>  		if (off > nlh->nlmsg_len) {
>  			fprintf(stderr,
> @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
>  	}
>  
>  	if (errfn)
> -		return errfn(msg, off, err_nlh);
> +		return errfn(*msg, off, err_nlh); /* FIXME */
>  
> -	if (msg && *msg != '\0') {
> +	ret = 0;
> +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) {
>  		bool is_err = !!err->error;
> +		const char *_msg = msg[i];
> +
> +		/* Message tagging has precedence.
> +		 * KERN_WARNING = ASCII Start Of Header ('\001') + '4'
> +		 */
> +		if (!strncmp(_msg, "\0014", 2)) {
> +			is_err = false;
> +			_msg += 2;
> +		}

If you are going to have an API that has levels, it must be the same
as existing syslog kernel log format and maybe even get some code reuse.

> +		/* But we can't have Error if it didn't fail. */
> +		if (is_err && !err->error)
> +			is_err = 0;
>  
>  		fprintf(stderr, "%s: %s",
> -			is_err ? "Error" : "Warning", msg);
> -		if (msg[strlen(msg) - 1] != '.')
> +			is_err ? "Error" : "Warning", _msg);
> +		if (_msg[strlen(_msg) - 1] != '.')
>  			fprintf(stderr, ".");
>  		fprintf(stderr, "\n");
>  
> -		return is_err ? 1 : 0;
> +		if (is_err)
> +			ret = 1;
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  #else
>  #warning "libmnl required for error support"

Re: [PATCH RFC iproute2] libnetlink: allow reading more than one message from extack

[Marcelo Ricardo Leitner]

On Mon, Mar 19, 2018 at 09:09:50AM -0700, Stephen Hemminger wrote:
> On Fri, 16 Mar 2018 16:23:09 -0300
> Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> 
> > This patch introduces support for reading more than one message from
> > extack's and to adjust their level (warning/error) accordingly.
> > 
> > Yes, there is a FIXME tag in the callback call for now.
> > 
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> Make sense, can hold off until kernel supports warnings.

Okay.

> 
> > ---
> >  lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644
> > --- a/lib/libnetlink.c
> > +++ b/lib/libnetlink.c
> > @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = {
> >  	[NLMSGERR_ATTR_OFFS]	= MNL_TYPE_U32,
> >  };
> >  
> > +#define NETLINK_MAX_EXTACK_MSGS 8
> 
> Would rather not have fixed maximums

Makes sense. Then it is more forward-compatible in case we increase
that on kernel side later.

> 
> > +struct extack_args {
> > +	const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS];
> > +	const struct nlattr *offs;
> > +	int msg_count;
> > +};
> 
> If you put msg[] last in structure, it could be variable length.

It will also require a two-times parsing then because we don't have an
attribute with the number of messages in there and we have to allocate
it with a certain length.

> 
> >  static int err_attr_cb(const struct nlattr *attr, void *data)
> >  {
> > -	const struct nlattr **tb = data;
> > +	struct extack_args *tb = data;
> >  	uint16_t type;
> >  
> >  	if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) {
> > @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data)
> >  		return MNL_CB_ERROR;
> >  	}
> >  
> > -	tb[type] = attr;
> > +	if (type == NLMSGERR_ATTR_OFFS)
> > +		tb->offs = attr;
> > +	else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS)
> > +		tb->msg[tb->msg_count++] = attr;
> >  	return MNL_CB_OK;
> >  }
> >  
> >  /* dump netlink extended ack error message */
> >  int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  {
> > -	struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {};
> > +	struct extack_args tb = {};
> >  	const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh);
> >  	const struct nlmsghdr *err_nlh = NULL;
> >  	unsigned int hlen = sizeof(*err);
> > -	const char *msg = NULL;
> > +	const char *msg[NETLINK_MAX_EXTACK_MSGS] = {};
> >  	uint32_t off = 0;
> > +	int ret, i;
> >  
> >  	/* no TLVs, nothing to do here */
> >  	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS))
> > @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
> >  		hlen += mnl_nlmsg_get_payload_len(&err->msg);
> >  
> > -	if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK)
> > +	if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK)
> >  		return 0;
> >  
> > -	if (tb[NLMSGERR_ATTR_MSG])
> > -		msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]);
> > +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++)
> > +		msg[i] = mnl_attr_get_str(tb.msg[i]);
> >  
> > -	if (tb[NLMSGERR_ATTR_OFFS]) {
> > -		off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
> > +	if (tb.offs) {
> > +		off = mnl_attr_get_u32(tb.offs);
> >  
> >  		if (off > nlh->nlmsg_len) {
> >  			fprintf(stderr,
> > @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  	}
> >  
> >  	if (errfn)
> > -		return errfn(msg, off, err_nlh);
> > +		return errfn(*msg, off, err_nlh); /* FIXME */
> >  
> > -	if (msg && *msg != '\0') {
> > +	ret = 0;
> > +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) {
> >  		bool is_err = !!err->error;
> > +		const char *_msg = msg[i];
> > +
> > +		/* Message tagging has precedence.
> > +		 * KERN_WARNING = ASCII Start Of Header ('\001') + '4'
> > +		 */
> > +		if (!strncmp(_msg, "\0014", 2)) {
> > +			is_err = false;
> > +			_msg += 2;
> > +		}
> 
> If you are going to have an API that has levels, it must be the same
> as existing syslog kernel log format and maybe even get some code reuse.

It is the same representation already. The magic "\0014" in there is
the same as KERN_WARNING. I'll check how other userspace applications
are dealing with this, as I don't see KERN_WARNING being exported to
userspace.

Thanks,
  Marcelo

> 
> > +		/* But we can't have Error if it didn't fail. */
> > +		if (is_err && !err->error)
> > +			is_err = 0;
> >  
> >  		fprintf(stderr, "%s: %s",
> > -			is_err ? "Error" : "Warning", msg);
> > -		if (msg[strlen(msg) - 1] != '.')
> > +			is_err ? "Error" : "Warning", _msg);
> > +		if (_msg[strlen(_msg) - 1] != '.')
> >  			fprintf(stderr, ".");
> >  		fprintf(stderr, "\n");
> >  
> > -		return is_err ? 1 : 0;
> > +		if (is_err)
> > +			ret = 1;
> >  	}
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  #else
> >  #warning "libmnl required for error support"
> 

[PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[Marcelo Ricardo Leitner]

Currently extack can carry only a single message, which is usually the
error message.

This imposes a limitation on a more verbose error reporting. For
example, it's not able to carry warning messages together with the error
message, or 2 warning messages.

One use case is when dealing with tc offloading. If it failed to
offload, and also failed to install on software, it will report only
regarding the error about the software datapath, but the error from the
hardware path would also have been welcomed.

This patch extends extack so it now can carry up to 8 messages and these
messages may be prefixed similarly to printk/pr_warning, so thus they
can be tagged either was warning or error.

Fixed number of messages because supporting a dynamic limit seem to be
an overkill for the moment. Remember that this is not meant to be a
trace tool, but an error reporting one.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/linux/netlink.h  | 50 +++++++++++++++++++++++++++++-------------------
 net/netlink/af_netlink.c | 12 +++++++-----
 2 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
  * @cookie: cookie data to return to userspace (for success)
  * @cookie_len: actual cookie data length
  */
+#define NETLINK_MAX_EXTACK_MSGS 8
 struct netlink_ext_ack {
-	const char *_msg;
+	const char *_msg[NETLINK_MAX_EXTACK_MSGS];
 	const struct nlattr *bad_attr;
 	u8 cookie[NETLINK_MAX_COOKIE_LEN];
 	u8 cookie_len;
+	u8 _msg_count;
 };
 
-/* Always use this macro, this allows later putting the
- * message into a separate section or such for things
- * like translation or listing all possible messages.
- * Currently string formatting is not supported (due
- * to the lack of an output buffer.)
+/* Always use these macros, this allows later putting
+ * the message into a separate section or such for
+ * things like translation or listing all possible
+ * messages.  Currently string formatting is not
+ * supported (due to the lack of an output buffer.)
  */
-#define NL_SET_ERR_MSG(extack, msg) do {		\
-	static const char __msg[] = msg;		\
-	struct netlink_ext_ack *__extack = (extack);	\
-							\
-	if (__extack)					\
-		__extack->_msg = __msg;			\
+#define NL_SET_MSG(extack, msg) do {					\
+	static const char __msg[] = msg;				\
+	struct netlink_ext_ack *__extack = (extack);			\
+									\
+	if (__extack &&							\
+	    !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS))	\
+		__extack->_msg[__extack->_msg_count++] = __msg;		\
 } while (0)
 
+#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
+#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
+
 #define NL_SET_ERR_MSG_MOD(extack, msg)			\
 	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
+#define NL_SET_WARN_MSG_MOD(extack, msg)		\
+	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
+
 
 #define NL_SET_BAD_ATTR(extack, attr) do {		\
 	if ((extack))					\
 		(extack)->bad_attr = (attr);		\
 } while (0)
 
-#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {	\
-	static const char __msg[] = msg;		\
-	struct netlink_ext_ack *__extack = (extack);	\
-							\
-	if (__extack) {					\
-		__extack->_msg = __msg;			\
-		__extack->bad_attr = (attr);		\
-	}						\
+#define NL_SET_ERR_MSG_ATTR(extack, attr, msg) do {			\
+	static const char __msg[] = msg;				\
+	struct netlink_ext_ack *__extack = (extack);			\
+									\
+	if (__extack) {							\
+		if (!WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS)) \
+			__extack->_msg[__extack->_msg_count++] = __msg;	\
+		__extack->bad_attr = (attr);				\
+	}								\
 } while (0)
 
 extern void netlink_kernel_release(struct sock *sk);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 5d10dcfe6411e745e2abcda925fe7b6fabdac0fc..71b4ece5c760979e082b0c644af9d2f222e1b721 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2350,13 +2350,15 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	struct netlink_sock *nlk = nlk_sk(NETLINK_CB(in_skb).sk);
 	unsigned int flags = 0;
 	bool nlk_has_extack = nlk->flags & NETLINK_F_EXT_ACK;
+	int i;
 
 	/* Error messages get the original request appened, unless the user
 	 * requests to cap the error message, and get extra error data if
 	 * requested.
 	 */
-	if (nlk_has_extack && extack && extack->_msg)
-		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
+	if (nlk_has_extack && extack)
+		for (i = 0; i < extack->_msg_count; i++)
+			tlvlen += nla_total_size(strlen(extack->_msg[i]) + 1);
 
 	if (err) {
 		if (!(nlk->flags & NETLINK_F_CAP_ACK))
@@ -2389,10 +2391,10 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err,
 	memcpy(&errmsg->msg, nlh, payload > sizeof(*errmsg) ? nlh->nlmsg_len : sizeof(*nlh));
 
 	if (nlk_has_extack && extack) {
-		if (extack->_msg) {
+		for (i = 0; i < extack->_msg_count; i++)
 			WARN_ON(nla_put_string(skb, NLMSGERR_ATTR_MSG,
-					       extack->_msg));
-		}
+					       extack->_msg[i]));
+
 		if (err) {
 			if (extack->bad_attr &&
 			    !WARN_ON((u8 *)extack->bad_attr < in_skb->data ||
-- 
2.14.3

Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[David Ahern]

On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
> Currently extack can carry only a single message, which is usually the
> error message.
> 
> This imposes a limitation on a more verbose error reporting. For
> example, it's not able to carry warning messages together with the error
> message, or 2 warning messages.


The only means for userspace to separate an error message from info or
warnings is the error in nlmsgerr. If it is non-0, any extack message is
considered an error else it is a warning.


> 
> One use case is when dealing with tc offloading. If it failed to
> offload, and also failed to install on software, it will report only
> regarding the error about the software datapath, but the error from the
> hardware path would also have been welcomed.
> 
> This patch extends extack so it now can carry up to 8 messages and these
> messages may be prefixed similarly to printk/pr_warning, so thus they
> can be tagged either was warning or error.
> 
> Fixed number of messages because supporting a dynamic limit seem to be
> an overkill for the moment. Remember that this is not meant to be a
> trace tool, but an error reporting one.
> 
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
>  include/linux/netlink.h  | 50 +++++++++++++++++++++++++++++-------------------
>  net/netlink/af_netlink.c | 12 +++++++-----
>  2 files changed, 37 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
>   * @cookie: cookie data to return to userspace (for success)
>   * @cookie_len: actual cookie data length
>   */
> +#define NETLINK_MAX_EXTACK_MSGS 8

8 is way too many. If some change fails because of an error, why would a
single error message not be enough? If it is a not an error, why is more
than 1 warning message not enough? (I forget the details of the tc
'skip_sw' use case)


>  struct netlink_ext_ack {
> -	const char *_msg;
> +	const char *_msg[NETLINK_MAX_EXTACK_MSGS];
>  	const struct nlattr *bad_attr;
>  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
>  	u8 cookie_len;
> +	u8 _msg_count;
>  };
>  
> -/* Always use this macro, this allows later putting the
> - * message into a separate section or such for things
> - * like translation or listing all possible messages.
> - * Currently string formatting is not supported (due
> - * to the lack of an output buffer.)
> +/* Always use these macros, this allows later putting
> + * the message into a separate section or such for
> + * things like translation or listing all possible
> + * messages.  Currently string formatting is not
> + * supported (due to the lack of an output buffer.)
>   */
> -#define NL_SET_ERR_MSG(extack, msg) do {		\
> -	static const char __msg[] = msg;		\
> -	struct netlink_ext_ack *__extack = (extack);	\
> -							\
> -	if (__extack)					\
> -		__extack->_msg = __msg;			\
> +#define NL_SET_MSG(extack, msg) do {					\
> +	static const char __msg[] = msg;				\
> +	struct netlink_ext_ack *__extack = (extack);			\
> +									\
> +	if (__extack &&							\
> +	    !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS))	\
> +		__extack->_msg[__extack->_msg_count++] = __msg;		\
>  } while (0)
>  
> +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
> +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
> +
>  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
>  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
> +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
> +

Adding separate macros for error versus warning is confusing since from
an extack perspective a message is a message and there is no uapi to
separate them.

Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[Marcelo Ricardo Leitner]

On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote:
> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
> > Currently extack can carry only a single message, which is usually the
> > error message.
> > 
> > This imposes a limitation on a more verbose error reporting. For
> > example, it's not able to carry warning messages together with the error
> > message, or 2 warning messages.
> 
> 
> The only means for userspace to separate an error message from info or
> warnings is the error in nlmsgerr. If it is non-0, any extack message is
> considered an error else it is a warning.

I don't see your point here.

The proposed patch extends what you said to:
- include warnings on error reports
- allow more than 1 message

With the proposed patch, if nlmsgerr is 0 all messages are considered
as warnings. If it's non-zero, some may be marked as warnings.

AFAICT it is not far from what you described, and still honouring the
main knob, mlmsgerr.

> 
> 
> > 
> > One use case is when dealing with tc offloading. If it failed to
> > offload, and also failed to install on software, it will report only
> > regarding the error about the software datapath, but the error from the
> > hardware path would also have been welcomed.
> > 
> > This patch extends extack so it now can carry up to 8 messages and these
> > messages may be prefixed similarly to printk/pr_warning, so thus they
> > can be tagged either was warning or error.
> > 
> > Fixed number of messages because supporting a dynamic limit seem to be
> > an overkill for the moment. Remember that this is not meant to be a
> > trace tool, but an error reporting one.
> > 
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> >  include/linux/netlink.h  | 50 +++++++++++++++++++++++++++++-------------------
> >  net/netlink/af_netlink.c | 12 +++++++-----
> >  2 files changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644
> > --- a/include/linux/netlink.h
> > +++ b/include/linux/netlink.h
> > @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
> >   * @cookie: cookie data to return to userspace (for success)
> >   * @cookie_len: actual cookie data length
> >   */
> > +#define NETLINK_MAX_EXTACK_MSGS 8
> 
> 8 is way too many. If some change fails because of an error, why would a

Ok. I'm fine with 4 for now.

> single error message not be enough? If it is a not an error, why is more
> than 1 warning message not enough? (I forget the details of the tc
> 'skip_sw' use case)

Because 1 message assumes you have a simple and linear code path being
executed and that it aborts on the first error it encounters.

You could, for example, report several bad arguments at once instead
of having the user to fix & retry until he gets it all right.

You can also have situations like:
Warning: option foo is useless with option bar specified.
    (as in: the rule may not work as you intended)
Error: failed to allocate a new node.

The goal is to be able to deliver more information to the
user/software using netlink and be able to log it, for later analysis.

If you have a look at mlx5/core/en_tc.c, there are several
printk(KERN_WARNING or pr_warn calls that should be getting returned
via extack and not to kernel dmesg. That's just one domain and it is
not aware if there is already a message stored in extack or if there
will be another one later.

> 
> 
> >  struct netlink_ext_ack {
> > -	const char *_msg;
> > +	const char *_msg[NETLINK_MAX_EXTACK_MSGS];
> >  	const struct nlattr *bad_attr;
> >  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
> >  	u8 cookie_len;
> > +	u8 _msg_count;
> >  };
> >  
> > -/* Always use this macro, this allows later putting the
> > - * message into a separate section or such for things
> > - * like translation or listing all possible messages.
> > - * Currently string formatting is not supported (due
> > - * to the lack of an output buffer.)
> > +/* Always use these macros, this allows later putting
> > + * the message into a separate section or such for
> > + * things like translation or listing all possible
> > + * messages.  Currently string formatting is not
> > + * supported (due to the lack of an output buffer.)
> >   */
> > -#define NL_SET_ERR_MSG(extack, msg) do {		\
> > -	static const char __msg[] = msg;		\
> > -	struct netlink_ext_ack *__extack = (extack);	\
> > -							\
> > -	if (__extack)					\
> > -		__extack->_msg = __msg;			\
> > +#define NL_SET_MSG(extack, msg) do {					\
> > +	static const char __msg[] = msg;				\
> > +	struct netlink_ext_ack *__extack = (extack);			\
> > +									\
> > +	if (__extack &&							\
> > +	    !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS))	\
> > +		__extack->_msg[__extack->_msg_count++] = __msg;		\
> >  } while (0)
> >  
> > +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
> > +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
> > +
> >  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
> >  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> > +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
> > +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
> > +
> 
> Adding separate macros for error versus warning is confusing since from
> an extack perspective a message is a message and there is no uapi to
> separate them.

Are you saying the markings at beginning of the messages are not
possible? If that's the case, we probably can think of something else,
as I see value in being able to deliver warnings together with errors.

  M.

Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[David Ahern]

On 3/18/18 12:19 PM, Marcelo Ricardo Leitner wrote:
> On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote:
>> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
>>> Currently extack can carry only a single message, which is usually the
>>> error message.
>>>
>>> This imposes a limitation on a more verbose error reporting. For
>>> example, it's not able to carry warning messages together with the error
>>> message, or 2 warning messages.
>>
>>
>> The only means for userspace to separate an error message from info or
>> warnings is the error in nlmsgerr. If it is non-0, any extack message is
>> considered an error else it is a warning.
> 
> I don't see your point here.
> 
> The proposed patch extends what you said to:
> - include warnings on error reports
> - allow more than 1 message
> 
> With the proposed patch, if nlmsgerr is 0 all messages are considered
> as warnings. If it's non-zero, some may be marked as warnings.

It's the 'some' that I was referring to, but ...


>>> +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
>>> +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
>>> +
>>>  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
>>>  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
>>> +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
>>> +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
>>> +
>>
>> Adding separate macros for error versus warning is confusing since from
>> an extack perspective a message is a message and there is no uapi to
>> separate them.
> 
> Are you saying the markings at beginning of the messages are not
> possible? If that's the case, we probably can think of something else,
> as I see value in being able to deliver warnings together with errors.

... I did miss the KERN_WARNIN above. That means that warning messages
are prefixed by 0x1 (KERN_SOH) and "4" (warning loglevel). There will be
cases missed by iproute2 as current versions won't catch the 2 new
characters.

Converting code to be able to continue generating error messages yet
ultimately fail seems overly complex to me. I get the intent of
returning as much info as possible, but most of that feels (e.g., in the
mlx5 example you referenced) like someone learning how to do something
the first time in which case 1 at a time errors seems reasonable - in
fact might drive home some lessons. ;-)

Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[Marcelo Ricardo Leitner]

On Sun, Mar 18, 2018 at 10:27:00PM -0600, David Ahern wrote:
> On 3/18/18 12:19 PM, Marcelo Ricardo Leitner wrote:
> > On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote:
> >> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
> >>> Currently extack can carry only a single message, which is usually the
> >>> error message.
> >>>
> >>> This imposes a limitation on a more verbose error reporting. For
> >>> example, it's not able to carry warning messages together with the error
> >>> message, or 2 warning messages.
> >>
> >>
> >> The only means for userspace to separate an error message from info or
> >> warnings is the error in nlmsgerr. If it is non-0, any extack message is
> >> considered an error else it is a warning.
> > 
> > I don't see your point here.
> > 
> > The proposed patch extends what you said to:
> > - include warnings on error reports
> > - allow more than 1 message
> > 
> > With the proposed patch, if nlmsgerr is 0 all messages are considered
> > as warnings. If it's non-zero, some may be marked as warnings.
> 
> It's the 'some' that I was referring to, but ...
> 
> 
> >>> +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
> >>> +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
> >>> +
> >>>  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
> >>>  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> >>> +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
> >>> +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
> >>> +
> >>
> >> Adding separate macros for error versus warning is confusing since from
> >> an extack perspective a message is a message and there is no uapi to
> >> separate them.
> > 
> > Are you saying the markings at beginning of the messages are not
> > possible? If that's the case, we probably can think of something else,
> > as I see value in being able to deliver warnings together with errors.
> 
> ... I did miss the KERN_WARNIN above. That means that warning messages
> are prefixed by 0x1 (KERN_SOH) and "4" (warning loglevel). There will be
> cases missed by iproute2 as current versions won't catch the 2 new
> characters.

The first one is not printable, so it would print a weird '4' at the
beginning of the message. But: only if it didn't have any error
message later, because old iproute will display only the last message
(and error messages are not tagged).

> 
> Converting code to be able to continue generating error messages yet
> ultimately fail seems overly complex to me. I get the intent of
> returning as much info as possible, but most of that feels (e.g., in the
> mlx5 example you referenced) like someone learning how to do something
> the first time in which case 1 at a time errors seems reasonable - in
> fact might drive home some lessons. ;-)

That is true.

Yep, I'm still lacking a real user for it. Maybe with the patchset
split it will come up.

  M.

[PATCH RFC 2/2] sched: pass extack through even if skip_sw is not set

[Marcelo Ricardo Leitner]

Currently, when the rule is not to be exclusively executed by the
hardware, extack is not passed along. The idea was that hardware
failures are okay because the rule will get executed in software then.

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
it is triggering the same reason may be challenging.

This patch thus removes the condition and makes it always pass extack
structure along.

Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/pkt_cls.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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))
-		cls_common->extack = extack;
+	cls_common->extack = extack;
 }
 
 enum tc_fl_command {
-- 
2.14.3

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

[Marcelo Ricardo Leitner]

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
> 

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

[Jakub Kicinski]

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)!

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

[Marcelo Ricardo Leitner]

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.

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

[David Ahern]

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.

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

[Marcelo Ricardo Leitner]

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

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

[Marcelo Ricardo Leitner]

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.