netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] xdp: use netlink extended ACK reporting
@ 2017-05-01  4:46 Jakub Kicinski
  2017-05-01  4:46 ` [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message Jakub Kicinski
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Jakub Kicinski @ 2017-05-01  4:46 UTC (permalink / raw)
  To: netdev
  Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs,
	Jakub Kicinski

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.

David Ahern's iproute2 ext ack patches for ip link are sufficient
to show the errors like this:

# ip link set dev p4p1 xdp obj ipip_prepend.o sec ".text"
Error: nfp: 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, but it's a big improvement over the
standard "Invalid argument" message.

v1/non-rfc:
 - add a separate macro in patch 1;
 - add KBUILD_MODNAME as part of the message (Daniel);
 - don't print the error to logs in patch 1.

Jakub Kicinski (4):
  netlink: add NULL-friendly helper for setting extended ACK message
  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                            |  8 ++++++++
 net/core/dev.c                                     |  5 ++++-
 net/core/rtnetlink.c                               | 13 ++++++++-----
 8 files changed, 52 insertions(+), 24 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
@ 2017-05-01  4:46 ` Jakub Kicinski
  2017-05-01 10:45   ` Daniel Borkmann
  2017-05-01  4:46 ` [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2017-05-01  4:46 UTC (permalink / raw)
  To: netdev
  Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs,
	Jakub Kicinski

As we propagate extended ack reporting throughout various paths in
the kernel it may be that the same function is called with the
extended ack parameter passed as NULL.  One place where that happens
is in drivers which have a centralized reconfiguration function
called both from ndos and from ethtool_ops.  Add a new helper for
setting the error message in such conditions.

Existing helper is left as is to encourage propagating the ext act
fully wherever possible.  It also makes it clear in the code which
messages may be lost due to ext ack being NULL.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/netlink.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 8d2a8924705c..c20395edf2de 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -92,6 +92,14 @@ struct netlink_ext_ack {
 	(extack)->_msg = _msg;			\
 } while (0)
 
+#define NL_MOD_TRY_SET_ERR_MSG(extack, msg) do {		\
+	static const char _msg[] = KBUILD_MODNAME ": " msg;	\
+	struct netlink_ext_ack *_extack = (extack);		\
+								\
+	if (_extack)						\
+		_extack->_msg = _msg;				\
+} while (0)
+
 extern void netlink_kernel_release(struct sock *sk);
 extern int __netlink_change_ngroups(struct sock *sk, unsigned int groups);
 extern int netlink_change_ngroups(struct sock *sk, unsigned int groups);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
  2017-05-01  4:46 ` [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message Jakub Kicinski
@ 2017-05-01  4:46 ` Jakub Kicinski
  2017-05-01 10:46   ` Daniel Borkmann
  2017-05-01  4:46 ` [PATCH net-next 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2017-05-01  4:46 UTC (permalink / raw)
  To: netdev
  Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs,
	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.

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 6847714a5ae3..9c23bd2efb56 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;
 	};
@@ -3291,7 +3296,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 8371a01eee87..35a06cebb282 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6854,12 +6854,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)
 {
 	int (*xdp_op)(struct net_device *dev, struct netdev_xdp *xdp);
 	const struct net_device_ops *ops = dev->netdev_ops;
@@ -6892,6 +6894,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 = xdp_op(dev, &xdp);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9031a6c8bfa7..6e67315ec368 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1919,6 +1919,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;
@@ -2201,7 +2202,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)
@@ -2261,7 +2262,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;
 }
@@ -2423,6 +2424,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;
@@ -2430,7 +2432,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;
 		}
@@ -2576,14 +2578,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] 12+ messages in thread

* [PATCH net-next 3/4] nfp: make use of extended ack message reporting
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
  2017-05-01  4:46 ` [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message Jakub Kicinski
  2017-05-01  4:46 ` [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
@ 2017-05-01  4:46 ` Jakub Kicinski
  2017-05-01 10:46   ` Daniel Borkmann
  2017-05-01  4:46 ` [PATCH net-next 4/4] virtio_net: " Jakub Kicinski
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2017-05-01  4:46 UTC (permalink / raw)
  To: netdev
  Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs,
	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 38b41fdeaa8f..fcf81b3be830 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
@@ -818,7 +818,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 b9f3548bb65f..db20376260f5 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
@@ -2524,24 +2524,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_MOD_TRY_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_MOD_TRY_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;
 
@@ -2553,7 +2556,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;
 
@@ -2628,7 +2631,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,
@@ -2944,9 +2947,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;
 
@@ -2969,7 +2973,7 @@ static int nfp_net_xdp_setup(struct nfp_net *nn, struct bpf_prog *prog)
 	dp->rx_dma_off = prog ? XDP_PACKET_HEADROOM - nn->dp.rx_offset : 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;
 
@@ -2987,7 +2991,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 a704efd4e314..abbb47e60cc3 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] 12+ messages in thread

* [PATCH net-next 4/4] virtio_net: make use of extended ack message reporting
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
                   ` (2 preceding siblings ...)
  2017-05-01  4:46 ` [PATCH net-next 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
@ 2017-05-01  4:46 ` Jakub Kicinski
  2017-05-01 10:50   ` Daniel Borkmann
  2017-05-01 10:32 ` [oss-drivers] [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Simon Horman
  2017-05-01 14:36 ` David Miller
  5 siblings, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2017-05-01  4:46 UTC (permalink / raw)
  To: netdev
  Cc: davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs,
	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 82f1c3a73345..046c60619c59 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1878,7 +1878,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);
@@ -1890,16 +1891,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;
 	}
@@ -1910,6 +1912,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;
@@ -1971,7 +1974,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] 12+ messages in thread

* Re: [oss-drivers] [PATCH net-next 0/4] xdp: use netlink extended ACK reporting
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
                   ` (3 preceding siblings ...)
  2017-05-01  4:46 ` [PATCH net-next 4/4] virtio_net: " Jakub Kicinski
@ 2017-05-01 10:32 ` Simon Horman
  2017-05-01 14:36 ` David Miller
  5 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2017-05-01 10:32 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, davem, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs

On Sun, Apr 30, 2017 at 09:46:44PM -0700, 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.
> 
> David Ahern's iproute2 ext ack patches for ip link are sufficient
> to show the errors like this:
> 
> # ip link set dev p4p1 xdp obj ipip_prepend.o sec ".text"
> Error: nfp: 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, but it's a big improvement over the
> standard "Invalid argument" message.
> 
> v1/non-rfc:
>  - add a separate macro in patch 1;
>  - add KBUILD_MODNAME as part of the message (Daniel);
>  - don't print the error to logs in patch 1.
> 
> Jakub Kicinski (4):
>   netlink: add NULL-friendly helper for setting extended ACK message
>   xdp: propagate extended ack to XDP setup
>   nfp: make use of extended ack message reporting
>   virtio_net: make use of extended ack message reporting

Reviewed-by: Simon Horman <simon.horman@netronome.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message
  2017-05-01  4:46 ` [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message Jakub Kicinski
@ 2017-05-01 10:45   ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2017-05-01 10:45 UTC (permalink / raw)
  To: Jakub Kicinski, netdev
  Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
	kubakici, oss-drivers, brouer, jhs

On 05/01/2017 06:46 AM, Jakub Kicinski wrote:
> As we propagate extended ack reporting throughout various paths in
> the kernel it may be that the same function is called with the
> extended ack parameter passed as NULL.  One place where that happens
> is in drivers which have a centralized reconfiguration function
> called both from ndos and from ethtool_ops.  Add a new helper for
> setting the error message in such conditions.
>
> Existing helper is left as is to encourage propagating the ext act
> fully wherever possible.  It also makes it clear in the code which
> messages may be lost due to ext ack being NULL.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup
  2017-05-01  4:46 ` [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
@ 2017-05-01 10:46   ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2017-05-01 10:46 UTC (permalink / raw)
  To: Jakub Kicinski, netdev
  Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
	kubakici, oss-drivers, brouer, jhs

On 05/01/2017 06:46 AM, Jakub Kicinski wrote:
> 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.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/4] nfp: make use of extended ack message reporting
  2017-05-01  4:46 ` [PATCH net-next 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
@ 2017-05-01 10:46   ` Daniel Borkmann
  0 siblings, 0 replies; 12+ messages in thread
From: Daniel Borkmann @ 2017-05-01 10:46 UTC (permalink / raw)
  To: Jakub Kicinski, netdev
  Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
	kubakici, oss-drivers, brouer, jhs

On 05/01/2017 06:46 AM, Jakub Kicinski wrote:
> Try to carry error messages to the user via the netlink extended
> ack message attribute.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 4/4] virtio_net: make use of extended ack message reporting
  2017-05-01  4:46 ` [PATCH net-next 4/4] virtio_net: " Jakub Kicinski
@ 2017-05-01 10:50   ` Daniel Borkmann
  2017-05-01 14:34     ` David Miller
  0 siblings, 1 reply; 12+ messages in thread
From: Daniel Borkmann @ 2017-05-01 10:50 UTC (permalink / raw)
  To: Jakub Kicinski, netdev
  Cc: davem, johannes, dsa, alexei.starovoitov, bblanco, john.fastabend,
	kubakici, oss-drivers, brouer, jhs

On 05/01/2017 06:46 AM, Jakub Kicinski wrote:
> Try to carry error messages to the user via the netlink extended
> ack message attribute.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

[...]
> @@ -1890,16 +1891,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");

Should this be NL_MOD_TRY_SET_ERR_MSG() as well like in nfp case
(otherwise the 'if (_extack)' check might be missing from the
macro)?

Thanks,
Daniel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 4/4] virtio_net: make use of extended ack message reporting
  2017-05-01 10:50   ` Daniel Borkmann
@ 2017-05-01 14:34     ` David Miller
  0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-05-01 14:34 UTC (permalink / raw)
  To: daniel
  Cc: jakub.kicinski, netdev, johannes, dsa, alexei.starovoitov,
	bblanco, john.fastabend, kubakici, oss-drivers, brouer, jhs

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Mon, 01 May 2017 12:50:55 +0200

> On 05/01/2017 06:46 AM, Jakub Kicinski wrote:
>> Try to carry error messages to the user via the netlink extended
>> ack message attribute.
>>
>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> 
> [...]
>> @@ -1890,16 +1891,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");
> 
> Should this be NL_MOD_TRY_SET_ERR_MSG() as well like in nfp case
> (otherwise the 'if (_extack)' check might be missing from the
> macro)?

I don't see a path in the doit callchain for setlink/newlink where the
extack pointer can be null.

I'm going to apply this series and we should either:

1) Guarantee the extack object is always available.

or

2) Make all macros including NL_SET_ERR_MSG() check the
   pointer.

Thanks.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/4] xdp: use netlink extended ACK reporting
  2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
                   ` (4 preceding siblings ...)
  2017-05-01 10:32 ` [oss-drivers] [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Simon Horman
@ 2017-05-01 14:36 ` David Miller
  5 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2017-05-01 14:36 UTC (permalink / raw)
  To: jakub.kicinski
  Cc: netdev, johannes, dsa, daniel, alexei.starovoitov, bblanco,
	john.fastabend, kubakici, oss-drivers, brouer, jhs

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Sun, 30 Apr 2017 21:46:44 -0700

> 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.
> 
> David Ahern's iproute2 ext ack patches for ip link are sufficient
> to show the errors like this:
> 
> # ip link set dev p4p1 xdp obj ipip_prepend.o sec ".text"
> Error: nfp: 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, but it's a big improvement over the
> standard "Invalid argument" message.
> 
> v1/non-rfc:
>  - add a separate macro in patch 1;
>  - add KBUILD_MODNAME as part of the message (Daniel);
>  - don't print the error to logs in patch 1.

Series applied.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-05-01 14:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-01  4:46 [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Jakub Kicinski
2017-05-01  4:46 ` [PATCH net-next 1/4] netlink: add NULL-friendly helper for setting extended ACK message Jakub Kicinski
2017-05-01 10:45   ` Daniel Borkmann
2017-05-01  4:46 ` [PATCH net-next 2/4] xdp: propagate extended ack to XDP setup Jakub Kicinski
2017-05-01 10:46   ` Daniel Borkmann
2017-05-01  4:46 ` [PATCH net-next 3/4] nfp: make use of extended ack message reporting Jakub Kicinski
2017-05-01 10:46   ` Daniel Borkmann
2017-05-01  4:46 ` [PATCH net-next 4/4] virtio_net: " Jakub Kicinski
2017-05-01 10:50   ` Daniel Borkmann
2017-05-01 14:34     ` David Miller
2017-05-01 10:32 ` [oss-drivers] [PATCH net-next 0/4] xdp: use netlink extended ACK reporting Simon Horman
2017-05-01 14:36 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).