* [RFC 1/4] netlink: make extended ACK setting NULL-friendly
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
@ 2017-04-25 8:06 ` Jakub Kicinski
2017-04-25 8:13 ` Johannes Berg
2017-04-25 9:12 ` Daniel Borkmann
2017-04-25 8:06 ` [RFC 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 8:06 UTC (permalink / raw)
To: netdev
Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers, Jakub Kicinski
As we propagate extended ack reporting throughout various paths in
the kernel it may happen that the same function is called with the
extended ack parameter passed as NULL. Make the NL_SET_ERR_MSG()
macro simply print the message to the logs if that happens.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
include/linux/netlink.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 8d2a8924705c..b59cfbf2e2c7 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -86,10 +86,14 @@ struct netlink_ext_ack {
* 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); \
- \
- (extack)->_msg = _msg; \
+#define NL_SET_ERR_MSG(extack, msg) do { \
+ struct netlink_ext_ack *_extack = (extack); \
+ static const char _msg[] = (msg); \
+ \
+ if (_extack) \
+ _extack->_msg = _msg; \
+ else \
+ pr_info("%s\n", _msg); \
} while (0)
extern void netlink_kernel_release(struct sock *sk);
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 1/4] netlink: make extended ACK setting NULL-friendly
2017-04-25 8:06 ` [RFC 1/4] netlink: make extended ACK setting NULL-friendly Jakub Kicinski
@ 2017-04-25 8:13 ` Johannes Berg
2017-04-25 20:53 ` Jakub Kicinski
2017-04-25 9:12 ` Daniel Borkmann
1 sibling, 1 reply; 21+ messages in thread
From: Johannes Berg @ 2017-04-25 8:13 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, dsa, daniel, alexei.starovoitov, bblanco, john.fastabend,
kubakici, oss-drivers
On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote:
> +#define NL_SET_ERR_MSG(extack, msg) do { \
> + struct netlink_ext_ack *_extack = (extack); \
> + static const char _msg[] = (msg); \
> + \
> + if (_extack) \
> + _extack->_msg = _msg; \
> + else \
> + pr_info("%s\n", _msg); \
> } while (0)
That's a good point, I used it only for genetlink so far where it was
guaranteed non-NULL.
I'm not really sure about the printing though - I'd rather not people
start relying on that and then we convert to have non-NULL and the
message disappears as a result ...
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 1/4] netlink: make extended ACK setting NULL-friendly
2017-04-25 8:13 ` Johannes Berg
@ 2017-04-25 20:53 ` Jakub Kicinski
2017-04-26 7:17 ` Johannes Berg
0 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 20:53 UTC (permalink / raw)
To: Johannes Berg, daniel
Cc: netdev, davem, dsa, alexei.starovoitov, bblanco, john.fastabend,
kubakici, oss-drivers
On Tue, 25 Apr 2017 10:13:34 +0200, Johannes Berg wrote:
> On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote:
>
> > +#define NL_SET_ERR_MSG(extack, msg) do { \
> > + struct netlink_ext_ack *_extack = (extack); \
> > + static const char _msg[] = (msg); \
> > + \
> > + if (_extack) \
> > + _extack->_msg = _msg; \
> > + else \
> > + pr_info("%s\n", _msg); \
> > } while (0)
>
> That's a good point, I used it only for genetlink so far where it was
> guaranteed non-NULL.
>
> I'm not really sure about the printing though - I'd rather not people
> start relying on that and then we convert to have non-NULL and the
> message disappears as a result ...
Yes, agreed. I don't really know what to do about that one though :|
One could argue people may already be depending on the messages which
I'm converting in this series... On the other hand, that would
be considering logs as part of the ABI which we don't want to do.
I'm leaning towards dropping the else clause and never printing, that
will add an incentive for people to convert more paths to provide the
ext ack. Any thoughts on that?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 1/4] netlink: make extended ACK setting NULL-friendly
2017-04-25 20:53 ` Jakub Kicinski
@ 2017-04-26 7:17 ` Johannes Berg
0 siblings, 0 replies; 21+ messages in thread
From: Johannes Berg @ 2017-04-26 7:17 UTC (permalink / raw)
To: Jakub Kicinski, daniel
Cc: netdev, davem, dsa, alexei.starovoitov, bblanco, john.fastabend,
oss-drivers
On Tue, 2017-04-25 at 13:53 -0700, Jakub Kicinski wrote:
> On Tue, 25 Apr 2017 10:13:34 +0200, Johannes Berg wrote:
> > On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote:
> >
> > > +#define NL_SET_ERR_MSG(extack, msg) do { \
> > > + struct netlink_ext_ack *_extack = (extack); \
> > > + static const char _msg[] = (msg); \
> > > + \
> > > + if (_extack) \
> > > + _extack->_msg = _msg; \
> > > + else \
> > > + pr_info("%s\n", _msg); \
> > > } while (0)
> I'm leaning towards dropping the else clause and never printing, that
> will add an incentive for people to convert more paths to provide the
> ext ack. Any thoughts on that?
Personally, I'm happy with that.
johannes
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 1/4] netlink: make extended ACK setting NULL-friendly
2017-04-25 8:06 ` [RFC 1/4] netlink: make extended ACK setting NULL-friendly Jakub Kicinski
2017-04-25 8:13 ` Johannes Berg
@ 2017-04-25 9:12 ` Daniel Borkmann
1 sibling, 0 replies; 21+ messages in thread
From: Daniel Borkmann @ 2017-04-25 9:12 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, dsa, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers
On 04/25/2017 10:06 AM, Jakub Kicinski wrote:
> As we propagate extended ack reporting throughout various paths in
> the kernel it may happen that the same function is called with the
> extended ack parameter passed as NULL. Make the NL_SET_ERR_MSG()
> macro simply print the message to the logs if that happens.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> include/linux/netlink.h | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 8d2a8924705c..b59cfbf2e2c7 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -86,10 +86,14 @@ struct netlink_ext_ack {
> * 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); \
> - \
> - (extack)->_msg = _msg; \
> +#define NL_SET_ERR_MSG(extack, msg) do { \
> + struct netlink_ext_ack *_extack = (extack); \
> + static const char _msg[] = (msg); \
> + \
> + if (_extack) \
> + _extack->_msg = _msg; \
> + else \
> + pr_info("%s\n", _msg); \
> } while (0)
>
> extern void netlink_kernel_release(struct sock *sk);
Probably makes sense to have a NL_MOD_SET_ERR_MSG(), which
then also prepends a KBUILD_MODNAME ": " string to the
message (similar to pr_*()), so that it's easier to identify
whether the error came from a specific driver or rather
common core code?
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 2/4] xdp: propagate extended ack to XDP setup
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
2017-04-25 8:06 ` [RFC 1/4] netlink: make extended ACK setting NULL-friendly Jakub Kicinski
@ 2017-04-25 8:06 ` Jakub Kicinski
2017-04-25 8:06 ` [RFC 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
` (3 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 8:06 UTC (permalink / raw)
To: netdev
Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers, Jakub Kicinski
Drivers usually have a number of restrictions for running XDP
- most common being buffer sizes, LRO and number of rings.
Even though some drivers try to be helpful and print error
messages experience shows that users don't often consult
kernel logs on netlink errors. Try to use the new extended
ack mechanism to carry the message back to user space.
For now the extack is only set for XDP_SETUP_PROG, adding it
to dump/XDP_QUERY_PROG didn't make much sense.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
include/linux/netdevice.h | 10 ++++++++--
net/core/dev.c | 5 ++++-
net/core/rtnetlink.c | 13 ++++++++-----
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5d5267febd56..41667f4238d2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -813,11 +813,16 @@ enum xdp_netdev_command {
XDP_QUERY_PROG,
};
+struct netlink_ext_ack;
+
struct netdev_xdp {
enum xdp_netdev_command command;
union {
/* XDP_SETUP_PROG */
- struct bpf_prog *prog;
+ struct {
+ struct bpf_prog *prog;
+ struct netlink_ext_ack *extack;
+ };
/* XDP_QUERY_PROG */
bool prog_attached;
};
@@ -3283,7 +3288,8 @@ int dev_get_phys_port_id(struct net_device *dev,
int dev_get_phys_port_name(struct net_device *dev,
char *name, size_t len);
int dev_change_proto_down(struct net_device *dev, bool proto_down);
-int dev_change_xdp_fd(struct net_device *dev, int fd, u32 flags);
+int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
+ int fd, u32 flags);
struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev);
struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
struct netdev_queue *txq, int *ret);
diff --git a/net/core/dev.c b/net/core/dev.c
index db6e31564d06..ca4633af5448 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6716,12 +6716,14 @@ EXPORT_SYMBOL(dev_change_proto_down);
/**
* dev_change_xdp_fd - set or clear a bpf program for a device rx path
* @dev: device
+ * @extact: netlink extended ack
* @fd: new program fd or negative value to clear
* @flags: xdp-related flags
*
* Set or clear a bpf program for a device
*/
-int dev_change_xdp_fd(struct net_device *dev, int fd, u32 flags)
+int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
+ int fd, u32 flags)
{
const struct net_device_ops *ops = dev->netdev_ops;
struct bpf_prog *prog = NULL;
@@ -6751,6 +6753,7 @@ int dev_change_xdp_fd(struct net_device *dev, int fd, u32 flags)
memset(&xdp, 0, sizeof(xdp));
xdp.command = XDP_SETUP_PROG;
+ xdp.extack = extack;
xdp.prog = prog;
err = ops->ndo_xdp(dev, &xdp);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 088f9c8b4196..1723dbb9e3dd 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1909,6 +1909,7 @@ static int do_set_master(struct net_device *dev, int ifindex)
#define DO_SETLINK_NOTIFY 0x03
static int do_setlink(const struct sk_buff *skb,
struct net_device *dev, struct ifinfomsg *ifm,
+ struct netlink_ext_ack *extack,
struct nlattr **tb, char *ifname, int status)
{
const struct net_device_ops *ops = dev->netdev_ops;
@@ -2191,7 +2192,7 @@ static int do_setlink(const struct sk_buff *skb,
}
if (xdp[IFLA_XDP_FD]) {
- err = dev_change_xdp_fd(dev,
+ err = dev_change_xdp_fd(dev, extack,
nla_get_s32(xdp[IFLA_XDP_FD]),
xdp_flags);
if (err)
@@ -2251,7 +2252,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
if (err < 0)
goto errout;
- err = do_setlink(skb, dev, ifm, tb, ifname, 0);
+ err = do_setlink(skb, dev, ifm, extack, tb, ifname, 0);
errout:
return err;
}
@@ -2413,6 +2414,7 @@ EXPORT_SYMBOL(rtnl_create_link);
static int rtnl_group_changelink(const struct sk_buff *skb,
struct net *net, int group,
struct ifinfomsg *ifm,
+ struct netlink_ext_ack *extack,
struct nlattr **tb)
{
struct net_device *dev, *aux;
@@ -2420,7 +2422,7 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
for_each_netdev_safe(net, dev, aux) {
if (dev->group == group) {
- err = do_setlink(skb, dev, ifm, tb, NULL, 0);
+ err = do_setlink(skb, dev, ifm, extack, tb, NULL, 0);
if (err < 0)
return err;
}
@@ -2566,14 +2568,15 @@ static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
status |= DO_SETLINK_NOTIFY;
}
- return do_setlink(skb, dev, ifm, tb, ifname, status);
+ return do_setlink(skb, dev, ifm, extack, tb, ifname,
+ status);
}
if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
if (ifm->ifi_index == 0 && tb[IFLA_GROUP])
return rtnl_group_changelink(skb, net,
nla_get_u32(tb[IFLA_GROUP]),
- ifm, tb);
+ ifm, extack, tb);
return -ENODEV;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
2017-04-25 8:06 ` [RFC 1/4] netlink: make extended ACK setting NULL-friendly Jakub Kicinski
2017-04-25 8:06 ` [RFC 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
@ 2017-04-25 8:06 ` Jakub Kicinski
2017-04-25 12:42 ` Jamal Hadi Salim
2017-04-25 8:06 ` [RFC 4/4] virtio_net: " Jakub Kicinski
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 8:06 UTC (permalink / raw)
To: netdev
Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers, Jakub Kicinski
Try to carry error messages to the user via the netlink extended
ack message attribute.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 ++-
.../net/ethernet/netronome/nfp/nfp_net_common.c | 22 +++++++++++++---------
.../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 4 ++--
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
index 8f20fdef0754..48b4a2742233 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -816,7 +816,8 @@ nfp_net_irqs_assign(struct nfp_net *nn, struct msix_entry *irq_entries,
unsigned int n);
struct nfp_net_dp *nfp_net_clone_dp(struct nfp_net *nn);
-int nfp_net_ring_reconfig(struct nfp_net *nn, struct nfp_net_dp *new);
+int nfp_net_ring_reconfig(struct nfp_net *nn, struct nfp_net_dp *new,
+ struct netlink_ext_ack *extack);
bool nfp_net_link_changed_read_clear(struct nfp_net *nn);
int nfp_net_refresh_eth_port(struct nfp_net *nn);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
index 8a9b74305493..ff66898375cc 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -2496,24 +2496,27 @@ struct nfp_net_dp *nfp_net_clone_dp(struct nfp_net *nn)
return new;
}
-static int nfp_net_check_config(struct nfp_net *nn, struct nfp_net_dp *dp)
+static int
+nfp_net_check_config(struct nfp_net *nn, struct nfp_net_dp *dp,
+ struct netlink_ext_ack *extack)
{
/* XDP-enabled tests */
if (!dp->xdp_prog)
return 0;
if (dp->fl_bufsz > PAGE_SIZE) {
- nn_warn(nn, "MTU too large w/ XDP enabled\n");
+ NL_SET_ERR_MSG(extack, "MTU too large w/ XDP enabled");
return -EINVAL;
}
if (dp->num_tx_rings > nn->max_tx_rings) {
- nn_warn(nn, "Insufficient number of TX rings w/ XDP enabled\n");
+ NL_SET_ERR_MSG(extack, "Insufficient number of TX rings w/ XDP enabled");
return -EINVAL;
}
return 0;
}
-int nfp_net_ring_reconfig(struct nfp_net *nn, struct nfp_net_dp *dp)
+int nfp_net_ring_reconfig(struct nfp_net *nn, struct nfp_net_dp *dp,
+ struct netlink_ext_ack *extack)
{
int r, err;
@@ -2525,7 +2528,7 @@ int nfp_net_ring_reconfig(struct nfp_net *nn, struct nfp_net_dp *dp)
dp->num_r_vecs = max(dp->num_rx_rings, dp->num_stack_tx_rings);
- err = nfp_net_check_config(nn, dp);
+ err = nfp_net_check_config(nn, dp, extack);
if (err)
goto exit_free_dp;
@@ -2600,7 +2603,7 @@ static int nfp_net_change_mtu(struct net_device *netdev, int new_mtu)
dp->mtu = new_mtu;
- return nfp_net_ring_reconfig(nn, dp);
+ return nfp_net_ring_reconfig(nn, dp, NULL);
}
static void nfp_net_stat64(struct net_device *netdev,
@@ -2916,9 +2919,10 @@ static int nfp_net_xdp_offload(struct nfp_net *nn, struct bpf_prog *prog)
return ret;
}
-static int nfp_net_xdp_setup(struct nfp_net *nn, struct bpf_prog *prog)
+static int nfp_net_xdp_setup(struct nfp_net *nn, struct netdev_xdp *xdp)
{
struct bpf_prog *old_prog = nn->dp.xdp_prog;
+ struct bpf_prog *prog = xdp->prog;
struct nfp_net_dp *dp;
int err;
@@ -2945,7 +2949,7 @@ static int nfp_net_xdp_setup(struct nfp_net *nn, struct bpf_prog *prog)
dp->rx_dma_off = 0;
/* We need RX reconfig to remap the buffers (BIDIR vs FROM_DEV) */
- err = nfp_net_ring_reconfig(nn, dp);
+ err = nfp_net_ring_reconfig(nn, dp, xdp->extack);
if (err)
return err;
@@ -2963,7 +2967,7 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_xdp *xdp)
switch (xdp->command) {
case XDP_SETUP_PROG:
- return nfp_net_xdp_setup(nn, xdp->prog);
+ return nfp_net_xdp_setup(nn, xdp);
case XDP_QUERY_PROG:
xdp->prog_attached = !!nn->dp.xdp_prog;
return 0;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index 6e27d1281425..4d41639b9b03 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -309,7 +309,7 @@ static int nfp_net_set_ring_size(struct nfp_net *nn, u32 rxd_cnt, u32 txd_cnt)
dp->rxd_cnt = rxd_cnt;
dp->txd_cnt = txd_cnt;
- return nfp_net_ring_reconfig(nn, dp);
+ return nfp_net_ring_reconfig(nn, dp, NULL);
}
static int nfp_net_set_ringparam(struct net_device *netdev,
@@ -880,7 +880,7 @@ static int nfp_net_set_num_rings(struct nfp_net *nn, unsigned int total_rx,
if (dp->xdp_prog)
dp->num_tx_rings += total_rx;
- return nfp_net_ring_reconfig(nn, dp);
+ return nfp_net_ring_reconfig(nn, dp, NULL);
}
static int nfp_net_set_channels(struct net_device *netdev,
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-25 8:06 ` [RFC 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
@ 2017-04-25 12:42 ` Jamal Hadi Salim
2017-04-25 14:20 ` David Miller
0 siblings, 1 reply; 21+ messages in thread
From: Jamal Hadi Salim @ 2017-04-25 12:42 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers
Good stuff. Question below:
On 17-04-25 04:06 AM, Jakub Kicinski wrote:
> Try to carry error messages to the user via the netlink extended
[..]
> +nfp_net_check_config(struct nfp_net *nn, struct nfp_net_dp *dp,
> + struct netlink_ext_ack *extack)
> {
> /* XDP-enabled tests */
> if (!dp->xdp_prog)
> return 0;
> if (dp->fl_bufsz > PAGE_SIZE) {
> - nn_warn(nn, "MTU too large w/ XDP enabled\n");
> + NL_SET_ERR_MSG(extack, "MTU too large w/ XDP enabled");
So are we going to standardize these strings?
i.e what if some user has written a bash script that depends on this
string and it gets changed later.
cheers,
jamal
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-25 12:42 ` Jamal Hadi Salim
@ 2017-04-25 14:20 ` David Miller
2017-04-26 11:13 ` [oss-drivers] " Simon Horman
0 siblings, 1 reply; 21+ messages in thread
From: David Miller @ 2017-04-25 14:20 UTC (permalink / raw)
To: jhs
Cc: jakub.kicinski, netdev, johannes, dsa, daniel,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Tue, 25 Apr 2017 08:42:32 -0400
> So are we going to standardize these strings?
No.
> i.e what if some user has written a bash script that depends on this
> string and it gets changed later.
They can't do that.
It's free form extra information an application may or not provide
to the user when the kernel emits it.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [oss-drivers] Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-25 14:20 ` David Miller
@ 2017-04-26 11:13 ` Simon Horman
2017-04-26 13:03 ` Jamal Hadi Salim
2017-04-26 14:44 ` David Miller
0 siblings, 2 replies; 21+ messages in thread
From: Simon Horman @ 2017-04-26 11:13 UTC (permalink / raw)
To: David Miller
Cc: jhs, jakub.kicinski, netdev, johannes, dsa, daniel,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
On Tue, Apr 25, 2017 at 10:20:22AM -0400, David Miller wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> Date: Tue, 25 Apr 2017 08:42:32 -0400
>
> > So are we going to standardize these strings?
>
> No.
>
> > i.e what if some user has written a bash script that depends on this
> > string and it gets changed later.
>
> They can't do that.
>
> It's free form extra information an application may or not provide
> to the user when the kernel emits it.
I don't feel strongly about this and perhaps it can be revisited at some
point but perhaps it would be worth documenting that he strings do not
form part of the UAPI as my expectation would have been that they do f.e. to
facilitate internationalisation.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [oss-drivers] Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-26 11:13 ` [oss-drivers] " Simon Horman
@ 2017-04-26 13:03 ` Jamal Hadi Salim
2017-04-26 13:31 ` Daniel Borkmann
2017-04-26 14:44 ` David Miller
1 sibling, 1 reply; 21+ messages in thread
From: Jamal Hadi Salim @ 2017-04-26 13:03 UTC (permalink / raw)
To: Simon Horman, David Miller
Cc: jakub.kicinski, netdev, johannes, dsa, daniel,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
On 17-04-26 07:13 AM, Simon Horman wrote:
> I don't feel strongly about this and perhaps it can be revisited at some
> point but perhaps it would be worth documenting that he strings do not
> form part of the UAPI as my expectation would have been that they do f.e. to
> facilitate internationalisation.
>
I thought people script against what iproute2 outputs today. We
may have to change habits.
cheers,
jamal
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [oss-drivers] Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-26 13:03 ` Jamal Hadi Salim
@ 2017-04-26 13:31 ` Daniel Borkmann
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Borkmann @ 2017-04-26 13:31 UTC (permalink / raw)
To: Jamal Hadi Salim, Simon Horman, David Miller
Cc: jakub.kicinski, netdev, johannes, dsa, alexei.starovoitov,
bblanco, john.fastabend, kubakici, oss-drivers
On 04/26/2017 03:03 PM, Jamal Hadi Salim wrote:
> On 17-04-26 07:13 AM, Simon Horman wrote:
>
>> I don't feel strongly about this and perhaps it can be revisited at some
>> point but perhaps it would be worth documenting that he strings do not
>> form part of the UAPI as my expectation would have been that they do f.e. to
>> facilitate internationalisation.
>
> I thought people script against what iproute2 outputs today. We
> may have to change habits.
I would strictly treat this kind of auxiliary information as
non-stable error message data, and it should be documented
as such, f.e. in the man page.
Every driver or subsystem using this could have different
restrictions/semantics and thus error messages will not be
the same everywhere anyway, so there's zero point in parsing
this text from an application in the first place, it's just
passing this through to the user to aide debugging.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [oss-drivers] Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-26 11:13 ` [oss-drivers] " Simon Horman
2017-04-26 13:03 ` Jamal Hadi Salim
@ 2017-04-26 14:44 ` David Miller
2017-04-26 18:44 ` Simon Horman
1 sibling, 1 reply; 21+ messages in thread
From: David Miller @ 2017-04-26 14:44 UTC (permalink / raw)
To: simon.horman
Cc: jhs, jakub.kicinski, netdev, johannes, dsa, daniel,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
From: Simon Horman <simon.horman@netronome.com>
Date: Wed, 26 Apr 2017 13:13:16 +0200
> On Tue, Apr 25, 2017 at 10:20:22AM -0400, David Miller wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>> Date: Tue, 25 Apr 2017 08:42:32 -0400
>>
>> > So are we going to standardize these strings?
>>
>> No.
>>
>> > i.e what if some user has written a bash script that depends on this
>> > string and it gets changed later.
>>
>> They can't do that.
>>
>> It's free form extra information an application may or not provide
>> to the user when the kernel emits it.
>
> I don't feel strongly about this and perhaps it can be revisited at some
> point but perhaps it would be worth documenting that he strings do not
> form part of the UAPI as my expectation would have been that they do f.e. to
> facilitate internationalisation.
These two things are entirely separate.
We can maintain uptodate translations of the strings, yet document that
they can change at any time and are thus not UAPI.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [oss-drivers] Re: [RFC 3/4] nfp: make use of extended ack message reporting
2017-04-26 14:44 ` David Miller
@ 2017-04-26 18:44 ` Simon Horman
0 siblings, 0 replies; 21+ messages in thread
From: Simon Horman @ 2017-04-26 18:44 UTC (permalink / raw)
To: David Miller
Cc: jhs, jakub.kicinski, netdev, johannes, dsa, daniel,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
On Wed, Apr 26, 2017 at 10:44:16AM -0400, David Miller wrote:
> From: Simon Horman <simon.horman@netronome.com>
> Date: Wed, 26 Apr 2017 13:13:16 +0200
>
> > On Tue, Apr 25, 2017 at 10:20:22AM -0400, David Miller wrote:
> >> From: Jamal Hadi Salim <jhs@mojatatu.com>
> >> Date: Tue, 25 Apr 2017 08:42:32 -0400
> >>
> >> > So are we going to standardize these strings?
> >>
> >> No.
> >>
> >> > i.e what if some user has written a bash script that depends on this
> >> > string and it gets changed later.
> >>
> >> They can't do that.
> >>
> >> It's free form extra information an application may or not provide
> >> to the user when the kernel emits it.
> >
> > I don't feel strongly about this and perhaps it can be revisited at some
> > point but perhaps it would be worth documenting that he strings do not
> > form part of the UAPI as my expectation would have been that they do f.e. to
> > facilitate internationalisation.
>
> These two things are entirely separate.
>
> We can maintain uptodate translations of the strings, yet document that
> they can change at any time and are thus not UAPI.
Thanks, I see that now.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC 4/4] virtio_net: make use of extended ack message reporting
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
` (2 preceding siblings ...)
2017-04-25 8:06 ` [RFC 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
@ 2017-04-25 8:06 ` Jakub Kicinski
2017-04-25 9:05 ` [RFC 0/4] xdp: use netlink extended ACK reporting Daniel Borkmann
2017-04-25 14:53 ` David Ahern
5 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 8:06 UTC (permalink / raw)
To: netdev
Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers, Jakub Kicinski
Try to carry error messages to the user via the netlink extended
ack message attribute.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/virtio_net.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 666ada6130ab..96c5bb31f0af 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1788,7 +1788,8 @@ static int virtnet_reset(struct virtnet_info *vi, int curr_qp, int xdp_qp)
return ret;
}
-static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
+static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+ struct netlink_ext_ack *extack)
{
unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr);
struct virtnet_info *vi = netdev_priv(dev);
@@ -1800,16 +1801,17 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
- netdev_warn(dev, "can't set XDP while host is implementing LRO, disable LRO first\n");
+ NL_SET_ERR_MSG(extack, "can't set XDP while host is implementing LRO, disable LRO first");
return -EOPNOTSUPP;
}
if (vi->mergeable_rx_bufs && !vi->any_header_sg) {
- netdev_warn(dev, "XDP expects header/data in single page, any_header_sg required\n");
+ NL_SET_ERR_MSG(extack, "XDP expects header/data in single page, any_header_sg required");
return -EINVAL;
}
if (dev->mtu > max_sz) {
+ NL_SET_ERR_MSG(extack, "MTU too large to enable XDP");
netdev_warn(dev, "XDP requires MTU less than %lu\n", max_sz);
return -EINVAL;
}
@@ -1820,6 +1822,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog)
/* XDP requires extra queues for XDP_TX */
if (curr_qp + xdp_qp > vi->max_queue_pairs) {
+ NL_SET_ERR_MSG(extack, "Too few free TX rings available");
netdev_warn(dev, "request %i queues but max is %i\n",
curr_qp + xdp_qp, vi->max_queue_pairs);
return -ENOMEM;
@@ -1881,7 +1884,7 @@ static int virtnet_xdp(struct net_device *dev, struct netdev_xdp *xdp)
{
switch (xdp->command) {
case XDP_SETUP_PROG:
- return virtnet_xdp_set(dev, xdp->prog);
+ return virtnet_xdp_set(dev, xdp->prog, xdp->extack);
case XDP_QUERY_PROG:
xdp->prog_attached = virtnet_xdp_query(dev);
return 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
` (3 preceding siblings ...)
2017-04-25 8:06 ` [RFC 4/4] virtio_net: " Jakub Kicinski
@ 2017-04-25 9:05 ` Daniel Borkmann
2017-04-25 16:54 ` Jesper Dangaard Brouer
2017-04-25 21:00 ` Jakub Kicinski
2017-04-25 14:53 ` David Ahern
5 siblings, 2 replies; 21+ messages in thread
From: Daniel Borkmann @ 2017-04-25 9:05 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, dsa, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers
On 04/25/2017 10:06 AM, Jakub Kicinski wrote:
> Hi!
>
> This series is an attempt to make XDP more user friendly by
> enabling exploiting the recently added netlink extended ACK
> reporting to carry messages to user space.
>
> I made iproute2 parse the extended messages and have it showing
> the errors like this:
>
> # ip link set dev p4p1 xdp obj ipip_prepend.o sec ".text"
> RTNETLINK answers: Invalid argument (MTU too large w/ XDP enabled)
>
> Where the message is coming directly from the driver. There could
> still be a bit of a leap for a complete novice from the message
> above to the right settings. I wonder if it would be worthwhile
But still 100x better than the current situation. ;) I really
like the series, thanks for working on this!
> adding #defines for the most common configuration conflicts?
> Sharing the messages verbatim between drivers could make them easier
> to google.
Makes sense, once more drivers adapt to this reporting, these
messages could be consolidated.
> Also - is anyone working on adding proper extack support to iproute2?
> The code I have right now is a bit of a hack...
>
> Jakub Kicinski (4):
> netlink: make extended ACK setting NULL-friendly
> xdp: propagate extended ack to XDP setup
> nfp: make use of extended ack message reporting
> virtio_net: make use of extended ack message reporting
>
> drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 ++-
> .../net/ethernet/netronome/nfp/nfp_net_common.c | 22 +++++++++++++---------
> .../net/ethernet/netronome/nfp/nfp_net_ethtool.c | 4 ++--
> drivers/net/virtio_net.c | 11 +++++++----
> include/linux/netdevice.h | 10 ++++++++--
> include/linux/netlink.h | 12 ++++++++----
> net/core/dev.c | 5 ++++-
> net/core/rtnetlink.c | 13 ++++++++-----
> 8 files changed, 52 insertions(+), 28 deletions(-)
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
2017-04-25 9:05 ` [RFC 0/4] xdp: use netlink extended ACK reporting Daniel Borkmann
@ 2017-04-25 16:54 ` Jesper Dangaard Brouer
2017-04-25 21:00 ` Jakub Kicinski
1 sibling, 0 replies; 21+ messages in thread
From: Jesper Dangaard Brouer @ 2017-04-25 16:54 UTC (permalink / raw)
To: Daniel Borkmann
Cc: brouer, Jakub Kicinski, netdev, davem, johannes, dsa,
alexei.starovoitov, bblanco, john.fastabend, kubakici,
oss-drivers
On Tue, 25 Apr 2017 11:05:27 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Where the message is coming directly from the driver. There could
> > still be a bit of a leap for a complete novice from the message
> > above to the right settings. I wonder if it would be worthwhile
>
> But still 100x better than the current situation. ;) I really
> like the series, thanks for working on this!
+1 thanks for working on this! :-)
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
2017-04-25 9:05 ` [RFC 0/4] xdp: use netlink extended ACK reporting Daniel Borkmann
2017-04-25 16:54 ` Jesper Dangaard Brouer
@ 2017-04-25 21:00 ` Jakub Kicinski
1 sibling, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 21:00 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, davem, johannes, dsa, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers
On Tue, 25 Apr 2017 11:05:27 +0200, Daniel Borkmann wrote:
> > adding #defines for the most common configuration conflicts?
> > Sharing the messages verbatim between drivers could make them easier
> > to google.
>
> Makes sense, once more drivers adapt to this reporting, these
> messages could be consolidated.
There seem to be concerns about standardizing/turning the strings into
ABI. I will leave it out for now, but we can revisit later :)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
2017-04-25 8:06 [RFC 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
` (4 preceding siblings ...)
2017-04-25 9:05 ` [RFC 0/4] xdp: use netlink extended ACK reporting Daniel Borkmann
@ 2017-04-25 14:53 ` David Ahern
2017-04-25 21:05 ` Jakub Kicinski
5 siblings, 1 reply; 21+ messages in thread
From: David Ahern @ 2017-04-25 14:53 UTC (permalink / raw)
To: Jakub Kicinski, netdev
Cc: davem, johannes, daniel, alexei.starovoitov, bblanco,
john.fastabend, kubakici, oss-drivers
On 4/25/17 2:06 AM, Jakub Kicinski wrote:
> Also - is anyone working on adding proper extack support to iproute2?
> The code I have right now is a bit of a hack...
This is what I have done:
https://github.com/dsahern/iproute2/commits/ext-ack
Basically, added the parsing code and then a new rtnl_talk_extack
function that takes a callback to invoke with the extack data. The last
patch (of 3) purposely breaks ip set link mtu -- sending the mtu as a
u16 rather than a u32 just to work on the plumbing for parsing the
returned message:
$ ip li set dummy1 mtu 1490
Error with rtnetlink attribute IFLA_MTU
If an errmsg is returned it is printed as well.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC 0/4] xdp: use netlink extended ACK reporting
2017-04-25 14:53 ` David Ahern
@ 2017-04-25 21:05 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2017-04-25 21:05 UTC (permalink / raw)
To: David Ahern
Cc: netdev, davem, johannes, daniel, alexei.starovoitov, bblanco,
john.fastabend, oss-drivers
On Tue, 25 Apr 2017 08:53:35 -0600, David Ahern wrote:
> On 4/25/17 2:06 AM, Jakub Kicinski wrote:
>
> > Also - is anyone working on adding proper extack support to iproute2?
> > The code I have right now is a bit of a hack...
>
> This is what I have done:
> https://github.com/dsahern/iproute2/commits/ext-ack
>
> Basically, added the parsing code and then a new rtnl_talk_extack
> function that takes a callback to invoke with the extack data. The last
> patch (of 3) purposely breaks ip set link mtu -- sending the mtu as a
> u16 rather than a u32 just to work on the plumbing for parsing the
> returned message:
>
> $ ip li set dummy1 mtu 1490
> Error with rtnetlink attribute IFLA_MTU
>
> If an errmsg is returned it is printed as well.
Great, that's much better than what I have. It will make the XDP patch
for iproute2 pretty trivial :)
^ permalink raw reply [flat|nested] 21+ messages in thread