Add support for warnings to extack
Subsystems (4)
cover Add support for warnings to extack
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(-)
Signature · 1 line
2.14.3
RFC PATCH 1/1 libnetlink: allow reading more than one message from extack
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.
--- lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 15 deletions(-)
lib/libnetlink.c +40 -15
@@ -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"
Signature · 1 line
2.14.3
Re: [PATCH RFC iproute2] libnetlink: allow reading more than one message from extack
Quoted 6 lines · On Fri, 16 Mar 2018 16:23:09 -0300 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 <[email protected]>
Make sense, can hold off until kernel supports warnings.
lib/libnetlink.c static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { · line 46
Quoted 13 lines
--- lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 15 deletions(-)lib/libnetlink.c +1
-0@@ -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
lib/libnetlink.c static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { · line 51
Quoted 5 lines
lib/libnetlink.c +5
-0+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.
Quoted 64 lines
lib/libnetlink.c +16
-11static 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++) {
lib/libnetlink.c int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) · line 127
Quoted 10 lines
lib/libnetlink.c +9
-0bool 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.
Quoted 22 lines
lib/libnetlink.c +8
-4+/* 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
Quoted 11 lines · Stephen Hemminger
Quoted 6 lines · On Fri, 16 Mar 2018 16:23:09 -0300 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 <[email protected]>Make sense, can hold off until kernel supports warnings.
Okay.
Quoted 13 lines
Quoted 13 lines
--- lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 15 deletions(-)lib/libnetlink.c +0
-0@@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { [NLMSGERR_ATTR_OFFS] = MNL_TYPE_U32, };
Quoted 3 lines
lib/libnetlink.c static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { · line 46
Quoted 1 line
lib/libnetlink.c +1
-0+#define NETLINK_MAX_EXTACK_MSGS 8Would rather not have fixed maximums
Makes sense. Then it is more forward-compatible in case we increase that on kernel side later.
Quoted 8 lines
lib/libnetlink.c static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = { · line 51
Quoted 6 lines
lib/libnetlink.c +5
-0+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.
Quoted 62 lines
Quoted 62 lines
lib/libnetlink.c +14
-10static 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 */
Quoted 16 lines
lib/libnetlink.c int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn) · line 127
Quoted 13 lines
lib/libnetlink.c +11
-1-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
Quoted 24 lines
Quoted 23 lines
lib/libnetlink.c +8
-4+/* 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"
RFC PATCH 1/2 netlink: extend extack so it can carry more than one message
In reply to Marcelo Ricardo LeitnerCurrently 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.
--- include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- net/netlink/af_netlink.c | 12 +++++++----- 2 files changed, 37 insertions(+), 25 deletions(-)
include/linux/netlink.h +30 -20
@@ -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 8struct 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);
net/netlink/af_netlink.c +7 -5
@@ -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 ||
Signature · 1 line
2.14.3
Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message
Quoted 6 lines · 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.
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.
Quoted 25 lines
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 <[email protected]>--- include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- net/netlink/af_netlink.c | 12 +++++++----- 2 files changed, 37 insertions(+), 25 deletions(-)include/linux/netlink.h +0
-0
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 74
Quoted 4 lines
include/linux/netlink.h +1
-0@@ -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)
Quoted 33 lines
include/linux/netlink.h +14
-12struct 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; \
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 105
Quoted 10 lines
include/linux/netlink.h +6
-0} 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
Quoted 4 lines · David Ahern
Quoted 3 lines · Marcelo Ricardo Leitner
Currently extack can carry only a single message, which is usually the error message.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 22 lines
Quoted 22 lines
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 <[email protected]>--- include/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- net/netlink/af_netlink.c | 12 +++++++----- 2 files changed, 37 insertions(+), 25 deletions(-)
Quoted 11 lines
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 74
Quoted 9 lines
include/linux/netlink.h +1
-0@@ -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 88 is way too many. If some change fails because of an error, why would a
Ok. I'm fine with 4 for now.
Quoted 3 lines
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.
Quoted 37 lines
Quoted 37 lines
include/linux/netlink.h +14
-12struct 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)
Quoted 12 lines
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 105
Quoted 8 lines
include/linux/netlink.h +6
-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
Quoted 14 lines · Marcelo Ricardo Leitner
Quoted 4 lines · David Ahern
Quoted 3 lines · Marcelo Ricardo Leitner
Currently extack can carry only a single message, which is usually the error message.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
I don't see your point here.
The proposed patch extends what you said to:
- include warnings on error reports
- allow more than 1 messageWith 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 ...
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
include/linux/netlink.h +3
-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) \Quoted 8 lines
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 105
Quoted 4 lines
include/linux/netlink.h +3
-0NL_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.
Quoted 3 lines
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
Quoted 40 lines · David Ahern
Quoted 14 lines · Marcelo Ricardo Leitner
Quoted 4 lines · David Ahern
Quoted 3 lines · Marcelo Ricardo Leitner
Currently extack can carry only a single message, which is usually the error message.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 8 lines
I don't see your point here.
The proposed patch extends what you said to:
- include warnings on error reports
- allow more than 1 messageWith 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 ...
Quoted 13 lines
Quoted 12 lines
include/linux/netlink.h netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg) · line 105
Quoted 8 lines
include/linux/netlink.h +6
-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.
Quoted 8 lines
Quoted 3 lines
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_WARNINabove. That means that warning messages are prefixed by0x1(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).
Quoted 7 lines
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.
RFC PATCH 2/2 sched: pass extack through even if skip_sw is not set
In reply to Marcelo Ricardo LeitnerCurrently, 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.
--- include/net/pkt_cls.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
include/net/pkt_cls.h +1 -2
@@ -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 {
Signature · 1 line
2.14.3
Quoted 24 lines · 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.
Quoted 8 lines
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.
Quoted 13 lines
Marcelo Ricardo Leitner (2):
netlink: extend extack so it can carry more than one message
sched: pass extack through even ifskip_swis not setinclude/linux/netlink.h | 50 +++++++++++++++++++++++++++++------------------- include/net/pkt_cls.h | 3 +-- net/netlink/af_netlink.c | 12 +++++++----- 3 files changed, 38 insertions(+), 27 deletions(-)Signature · 1 line
2.14.3
Quoted 24 lines · 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.
Quoted 8 lines
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:
include/net/pkt_cls.h +1 -1
@@ -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)!
Quoted 36 lines · Jakub Kicinski
CC David Ahern <[email protected]>Quoted 24 lines · 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.
Quoted 8 lines
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
Quoted 8 lines
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.
Quoted 19 lines
There maybe well be value in ability to report multiple messages. But for opt-in warning messages I would be leaning towards:
include/net/pkt_cls.h +1
-1@@ -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 {
Quoted 3 lines
That is admittedly quite conservative. Esp. in case of flower,
cls_bpfis 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.
Quoted 5 lines · 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.
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.
Quoted 10 lines · David Ahern
Quoted 5 lines · 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.
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
Quoted 10 lines · David Ahern
Quoted 5 lines · 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.
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.