* Re: [PATCH] CDC NCM: release interfaces fix in unbind()
From: David Miller @ 2011-05-16 19:13 UTC (permalink / raw)
To: alexey.orishko; +Cc: netdev, linux-usb, gregkh, oliver, alexey.orishko
In-Reply-To: <1305491307-6548-1-git-send-email-alexey.orishko@stericsson.com>
From: Alexey Orishko <alexey.orishko@gmail.com>
Date: Sun, 15 May 2011 22:28:27 +0200
> Signed-off-by: Alexey Orishko <alexey.orishko@stericsson.com>
I don't know about this.
The usb_set_intfdata() calls at cdc_ncm_bind() time are unconditional,
and are performed regardless of whether ->data_claimed or
->control_claimed are set.
So conditionalizing the setting of them to NULL here doesn't seem
right at all.
I'm not applying this patch.
^ permalink raw reply
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
From: David Miller @ 2011-05-16 19:14 UTC (permalink / raw)
To: mirq-linux; +Cc: netdev, mst, herbert, bhutchings
In-Reply-To: <20110516131757.ED57E13A6A@rere.qmqm.pl>
From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
> Those reduced to DEBUG can possibly be triggered by unprivileged processes
> and are nothing exceptional. Illegal checksum combinations can only be
> caused by driver bug, so promote those messages to WARN.
>
> Since GSO without SG will now only cause DEBUG message from
> netdev_fix_features(), remove the workaround from register_netdevice().
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next-2.6 v2] ipv4: more compliant RFC 3168 support
From: Joe Perches @ 2011-05-16 19:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Stefanos Harhalakis, David Miller, netdev
In-Reply-To: <1305571057.2581.2.camel@edumazet-laptop>
On Mon, 2011-05-16 at 20:37 +0200, Eric Dumazet wrote:
> Commit 6623e3b24a5e (ipv4: IP defragmentation must be ECN aware) was an
> attempt to not lose "Congestion Experienced" (CE) indications when
> performing datagram defragmentation.
> Stefanos Harhalakis raised the point that RFC 3168 requirements were not
> completely met by this commit.
> In particular, we MUST detect invalid combinations and eventually drop
> illegal frames.
> Reported-by: Stefanos Harhalakis <v13@v13.gr>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> v2 : Use a table instead of a switch()
Just trivia:
bitmasks and hard coded constants can be a bit of a minefield.
> diff --git a/net/ipv4/ip_fragment.c b/net/ipv4/ip_fragment.c
> @@ -77,22 +77,42 @@ struct ipq {
[]
> +/* RFC 3168 support :
> + * We want to check ECN values of all fragments, do detect invalid combinations.
> + * In ipq->ecn, we store the OR value of each ip4_frag_ecn() fragment value.
> + */
> +enum {
> + IPFRAG_ECN_NOT_ECT = 0x01, /* one frag had ECN_NOT_ECT */
> + IPFRAG_ECN_ECT_1 = 0x02, /* one frag had ECN_ECT_1 */
> + IPFRAG_ECN_ECT_0 = 0x04, /* one frag had ECN_ECT_0 */
> + IPFRAG_ECN_CE = 0x08, /* one frag had ECN_CE */
Might be better to have a IPFRAG_ECN_TABLESIZE or some such.
IPFRAG_ECN_TABLESIZE = 0x10,
> +static const u8 ip4_frag_ecn_table[16] = {
static const u8 ip4_frag_ecn_table[IPFRAG_ECN_TABLESIZE] = {
^ permalink raw reply
* [PATCH V5 0/6 net-next] macvtap/vhost TX zero-copy support
From: Shirley Ma @ 2011-05-16 19:24 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
This patchset add supports for TX zero-copy between guest and host
kernel through vhost. It significantly reduces CPU utilization on the
local host on which the guest is located (It reduced 30-50% CPU usage
for vhost thread for single stream test). The patchset is based on
previous submission and comments from the community regarding when/how
to handle guest kernel buffers to be released. This is the simplest
approach I can think of after comparing with several other solutions.
This patchset has integrated V3 review comments from community:
1. Add more comments on how to use device ZEROCOPY flag;
2. Change device ZEROCOPY to available bit 31
3. Fix skb header linear allocation when virtio_net GSO is not enabled
It has integrated V4 review comments from MST and Sridhar:
1. In vhost, using socket poll wake up for outstanding DMAs
2. Add detailed comments for vhost_zerocopy_signal_used call
3. Add sleep in vhost shutting down instead of busy-wait for outstanding
DMAs.
4. Copy small packets, don't do zero-copy callback in mavtap, mark it's
DMA done in vhost
5. change zerocopy to bool in macvtap.
One comment doesn't address:
Should we count the whole page in sk_wmem_alloc for zerocopy buffer? If
so, sock_wfree needs to change too.
This patchset includes:
1/6: Add a new sock zero-copy flag, SOCK_ZEROCOPY;
2/6: Add a new device flag, NETIF_F_ZEROCOPY for lower level device
support zero-copy;
3/6: Add a new struct skb_ubuf_info in skb_share_info for userspace
buffers release callback when lower device DMA has done for that skb,
which is the last reference count gone;
4/6: Add vhost zero-copy callback in vhost when skb last refcnt is gone;
add vhost_zerocopy_signal_used to notify guest to release TX skb
buffers.
5/6: Add macvtap zero-copy in lower device when sending packet is
greater than 256 bytes to make sure there is enough room for expanding
skb head.
6/6: example: enable Intel 10Gb NIC zero-copy feature flag
The patchset is built against most recent net-next linux 2.6.39-rc7. It
has passed netperf/netserver multiple streams stress test.
Single TCP_STREAM 120 secs test results over ixgbe 10Gb NIC results:
Message BW(Gb/s)qemu-kvm (NumCPU)vhost-net(NumCPU) PerfTop irq/s
4K 7408.57 92.1% 22.6% 1229
4K(Orig)4913.17 118.1% 84.1% 2086
8K 9129.90 89.3% 23.3% 1141
8K(Orig)7094.55 115.9% 84.7% 2157
16K 9178.81 89.1% 23.3% 1139
16K(Orig)8927.1 118.7% 83.4% 2262
64K 9171.43 88.4% 24.9% 1253
64K(Orig)9085.85 115.9% 82.4% 2229
For message size less or equal than 2K, there is a known KVM guest TX
overrun issue. With this zero-copy patch, the issue becomes more severe,
guest io_exits has tripled than before, so the performance is not good.
Once the TX overrun problem has been addressed, I will retest the small
message size performance.
^ permalink raw reply
* [PATCH V5 1/6 net-next] sock.h: Add a new sock zero-copy flag
From: Shirley Ma @ 2011-05-16 19:26 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
include/net/sock.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index f2046e4..2229bd1 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -563,6 +563,7 @@ enum sock_flags {
SOCK_TIMESTAMPING_SYS_HARDWARE, /* %SOF_TIMESTAMPING_SYS_HARDWARE */
SOCK_FASYNC, /* fasync() active */
SOCK_RXQ_OVFL,
+ SOCK_ZEROCOPY, /* buffers from userspace */
};
static inline void sock_copy_flags(struct sock *nsk, struct sock *osk)
^ permalink raw reply related
* [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Shirley Ma @ 2011-05-16 19:28 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
include/linux/netdevice.h | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a134d80..2646251 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1066,6 +1066,16 @@ struct net_device {
#define NETIF_F_NOCACHE_COPY (1 << 30) /* Use no-cache copyfromuser */
#define NETIF_F_LOOPBACK (1 << 31) /* Enable loopback */
+/*
+ * Bit 31 is for device to map userspace buffers -- zerocopy
+ * Device can set this flag when it supports HIGHDMA.
+ * Device can't recycle this kind of skb buffers.
+ * There are 256 bytes copied, the rest of buffers are mapped.
+ * The userspace callback should only be called when last reference to this skb
+ * is gone.
+ */
+#define NETIF_F_ZEROCOPY (1 << 31)
+
/* Segmentation offload features */
#define NETIF_F_GSO_SHIFT 16
#define NETIF_F_GSO_MASK 0x00ff0000
^ permalink raw reply related
* [PATCH V5 3/6 net-next] skbuff: Add userspace zero-copy buffers in skb
From: Shirley Ma @ 2011-05-16 19:31 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
This patch adds userspace buffers support in skb shared info. A new
struct skb_ubuf_info is needed to maintain the userspace buffers
argument and index, a callback is used to notify userspace to release
the buffers once lower device has done DMA (Last reference to that skb
has gone).
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
include/linux/skbuff.h | 26 ++++++++++++++++++++++++++
net/core/skbuff.c | 13 +++++++++++++
2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 79aafbb..40faffe 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -189,6 +189,18 @@ enum {
SKBTX_DRV_NEEDS_SK_REF = 1 << 3,
};
+/*
+ * The callback notifies userspace to release buffers when skb DMA is
done in
+ * lower device, the skb last reference should be 0 when calling this.
+ * The desc is used to track userspace buffer index.
+ */
+struct skb_ubuf_info {
+ /* support buffers allocation from userspace */
+ void (*callback)(struct sk_buff *);
+ void *arg;
+ size_t desc;
+};
+
/* This data is invariant across clones and lives at
* the end of the header data, ie. at skb->end.
*/
@@ -211,6 +223,10 @@ struct skb_shared_info {
/* Intermediate layers must ensure that destructor_arg
* remains valid until skb destructor */
void * destructor_arg;
+
+ /* DMA mapping from/to userspace buffers */
+ struct skb_ubuf_info ubuf;
+
/* must be last field, see pskb_expand_head() */
skb_frag_t frags[MAX_SKB_FRAGS];
};
@@ -2261,5 +2277,15 @@ static inline void
skb_checksum_none_assert(struct sk_buff *skb)
}
bool skb_partial_csum_set(struct sk_buff *skb, u16 start, u16 off);
+
+/*
+ * skb_ubuf - is the buffer from userspace
+ * @skb: buffer to check
+ */
+static inline int skb_ubuf(const struct sk_buff *skb)
+{
+ return (skb_shinfo(skb)->ubuf.callback != NULL);
+}
+
#endif /* __KERNEL__ */
#endif /* _LINUX_SKBUFF_H */
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7ebeed0..9cbd3fc 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -210,6 +210,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
gfp_mask,
shinfo = skb_shinfo(skb);
memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
atomic_set(&shinfo->dataref, 1);
+ shinfo->ubuf.callback = NULL;
+ shinfo->ubuf.arg = NULL;
kmemcheck_annotate_variable(shinfo->destructor_arg);
if (fclone) {
@@ -328,6 +330,14 @@ static void skb_release_data(struct sk_buff *skb)
put_page(skb_shinfo(skb)->frags[i].page);
}
+ /*
+ * if skb buf is from userspace, we need to notify the caller
+ * the lower device DMA has done;
+ */
+ if (skb_shinfo(skb)->ubuf.callback) {
+ skb_shinfo(skb)->ubuf.callback(skb);
+ skb_shinfo(skb)->ubuf.callback = NULL;
+ }
if (skb_has_frag_list(skb))
skb_drop_fraglist(skb);
@@ -480,6 +490,9 @@ bool skb_recycle_check(struct sk_buff *skb, int
skb_size)
if (irqs_disabled())
return false;
+ if (skb_ubuf(skb))
+ return false;
+
if (skb_is_nonlinear(skb) || skb->fclone != SKB_FCLONE_UNAVAILABLE)
return false;
^ permalink raw reply related
* [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Shirley Ma @ 2011-05-16 19:34 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
This patch maintains the outstanding userspace buffers in the
sequence it is delivered to vhost. The outstanding userspace buffers
will be marked as done once the lower device buffers DMA has finished.
This is monitored through last reference of kfree_skb callback. Two
buffer index are used for this purpose.
The vhost passes the userspace buffers info to lower device skb
through message control. Since there will be some done DMAs when
entering vhost handle_tx. The worse case is all buffers in the vq are
in pending/done status, so we need to notify guest to release DMA done
buffers first before get any new buffers from the vq.
Signed-off-by: Shirley <xma@us.ibm.com>
---
drivers/vhost/net.c | 39 +++++++++++++++++++++++++++++++-
drivers/vhost/vhost.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 12 ++++++++++
3 files changed, 107 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2f7c76a..c964000 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -32,6 +32,10 @@
* Using this limit prevents one virtqueue from starving others. */
#define VHOST_NET_WEIGHT 0x80000
+/* MAX number of TX used buffers for outstanding zerocopy */
+#define VHOST_MAX_PEND 128
+#define VHOST_GOODCOPY_LEN 256
+
enum {
VHOST_NET_VQ_RX = 0,
VHOST_NET_VQ_TX = 1,
@@ -129,6 +133,7 @@ static void handle_tx(struct vhost_net *net)
int err, wmem;
size_t hdr_size;
struct socket *sock;
+ struct skb_ubuf_info pend;
/* TODO: check that we are running from vhost_worker? */
sock = rcu_dereference_check(vq->private_data, 1);
@@ -151,6 +156,10 @@ static void handle_tx(struct vhost_net *net)
hdr_size = vq->vhost_hlen;
for (;;) {
+ /* Release DMAs done buffers first */
+ if (atomic_read(&vq->refcnt) > VHOST_MAX_ZEROCOPY_PEND)
+ vhost_zerocopy_signal_used(vq, false);
+
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in,
@@ -166,6 +175,12 @@ static void handle_tx(struct vhost_net *net)
set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
break;
}
+ /* If more outstanding DMAs, queue the work */
+ if (atomic_read(&vq->refcnt) > VHOST_MAX_PEND) {
+ tx_poll_start(net, sock);
+ set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
+ break;
+ }
if (unlikely(vhost_enable_notify(vq))) {
vhost_disable_notify(vq);
continue;
@@ -188,17 +203,37 @@ static void handle_tx(struct vhost_net *net)
iov_length(vq->hdr, s), hdr_size);
break;
}
+ /* use msg_control to pass vhost zerocopy ubuf info to skb */
+ if (sock_flag(sock->sk, SOCK_ZEROCOPY)) {
+ vq->heads[vq->upend_idx].id = head;
+ if (len <= VHOST_GOODCOPY_LEN)
+ /* copy don't need to wait for DMA done */
+ vq->heads[vq->upend_idx].len =
+ VHOST_DMA_DONE_LEN;
+ else {
+ vq->heads[vq->upend_idx].len = len;
+ pend.callback = vhost_zerocopy_callback;
+ pend.arg = vq;
+ pend.desc = vq->upend_idx;
+ msg.msg_control = &pend;
+ msg.msg_controllen = sizeof(pend);
+ }
+ atomic_inc(&vq->refcnt);
+ vq->upend_idx = (vq->upend_idx + 1) % UIO_MAXIOV;
+ }
/* TODO: Check specific error and bomb out unless ENOBUFS? */
err = sock->ops->sendmsg(NULL, sock, &msg, len);
if (unlikely(err < 0)) {
- vhost_discard_vq_desc(vq, 1);
+ if (!sock_flag(sock->sk, SOCK_ZEROCOPY))
+ vhost_discard_vq_desc(vq, 1);
tx_poll_start(net, sock);
break;
}
if (err != len)
pr_debug("Truncated TX packet: "
" len %d != %zd\n", err, len);
- vhost_add_used_and_signal(&net->dev, vq, head, 0);
+ if (!sock_flag(sock->sk, SOCK_ZEROCOPY))
+ vhost_add_used_and_signal(&net->dev, vq, head, 0);
total_len += len;
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 2ab2912..1a40310 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -174,6 +174,9 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->call_ctx = NULL;
vq->call = NULL;
vq->log_ctx = NULL;
+ vq->upend_idx = 0;
+ vq->done_idx = 0;
+ atomic_set(&vq->refcnt, 0);
}
static int vhost_worker(void *data)
@@ -385,16 +388,62 @@ long vhost_dev_reset_owner(struct vhost_dev *dev)
return 0;
}
+/* Since we need to keep the order of used_idx as avail_idx, it's possible that
+ * DMA done not in order in lower device driver for some reason. To prevent
+ * used_idx out of order, upend_idx is used to track avail_idx order, done_idx
+ * is used to track used_idx order. Once lower device DMA done, then upend_idx
+ * can move to done_idx.
+ */
+void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool shutdown)
+{
+ int i, j = 0;
+
+ i = vq->done_idx;
+ while (i != vq->upend_idx) {
+ if ((vq->heads[i].len == VHOST_DMA_DONE_LEN) || shutdown) {
+ /* reset len = 0 */
+ vq->heads[i].len = 0;
+ i = (i + 1) % UIO_MAXIOV;
+ ++j;
+ } else
+ break;
+ }
+ if (j) {
+ /* heads is an array, i is used to tracking the last done_idx,
+ * if last done_idx is less than current done_idx, then
+ * add_used_n is not contiguous */
+ if (i > vq->done_idx)
+ vhost_add_used_n(vq, &vq->heads[vq->done_idx], j);
+ else {
+ vhost_add_used_n(vq, &vq->heads[vq->done_idx],
+ UIO_MAXIOV - vq->done_idx);
+ vhost_add_used_n(vq, vq->heads, i);
+ }
+ vq->done_idx = i;
+ vhost_signal(vq->dev, vq);
+ atomic_sub(j, &vq->refcnt);
+ }
+}
+
/* Caller should have device mutex */
void vhost_dev_cleanup(struct vhost_dev *dev)
{
int i;
+ unsigned long begin = jiffies;
for (i = 0; i < dev->nvqs; ++i) {
if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
vhost_poll_stop(&dev->vqs[i].poll);
vhost_poll_flush(&dev->vqs[i].poll);
}
+ /* Wait for all lower device DMAs done, then notify virtio_net
+ * or just notify it without waiting for all DMA done here ?
+ * in case of DMAs never done for some reason */
+ if (atomic_read(&dev->vqs[i].refcnt)) {
+ /* how long should we wait ? */
+ msleep(1000);
+ vhost_zerocopy_signal_used(&dev->vqs[i], true);
+ }
if (dev->vqs[i].error_ctx)
eventfd_ctx_put(dev->vqs[i].error_ctx);
if (dev->vqs[i].error)
@@ -1416,3 +1465,12 @@ void vhost_disable_notify(struct vhost_virtqueue *vq)
vq_err(vq, "Failed to enable notification at %p: %d\n",
&vq->used->flags, r);
}
+
+void vhost_zerocopy_callback(struct sk_buff *skb)
+{
+ int idx = skb_shinfo(skb)->ubuf.desc;
+ struct vhost_virtqueue *vq = skb_shinfo(skb)->ubuf.arg;
+
+ /* set len = 1 to mark this desc buffers done DMA */
+ vq->heads[idx].len = VHOST_DMA_DONE_LEN;
+}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index b3363ae..8e3ecc7 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -13,6 +13,10 @@
#include <linux/virtio_ring.h>
#include <asm/atomic.h>
+/* This is for zerocopy, used buffer len is set to 1 when lower device DMA
+ * done */
+#define VHOST_DMA_DONE_LEN 1
+
struct vhost_device;
struct vhost_work;
@@ -108,6 +112,12 @@ struct vhost_virtqueue {
/* Log write descriptors */
void __user *log_base;
struct vhost_log *log;
+ /* vhost zerocopy support */
+ atomic_t refcnt; /* num of outstanding zerocopy DMAs */
+ /* copy of avail idx to monitor outstanding DMA zerocopy buffers */
+ int upend_idx;
+ /* copy of used idx to monintor DMA done zerocopy buffers */
+ int done_idx;
};
struct vhost_dev {
@@ -154,6 +164,8 @@ bool vhost_enable_notify(struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
+void vhost_zerocopy_callback(struct sk_buff *skb);
+void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool shutdown);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Ben Hutchings @ 2011-05-16 19:35 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann,
netdev, kvm, linux-kernel
In-Reply-To: <1305574128.3456.23.camel@localhost.localdomain>
On Mon, 2011-05-16 at 12:28 -0700, Shirley Ma wrote:
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
>
> include/linux/netdevice.h | 10 ++++++++++
> 1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index a134d80..2646251 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1066,6 +1066,16 @@ struct net_device {
> #define NETIF_F_NOCACHE_COPY (1 << 30) /* Use no-cache copyfromuser */
> #define NETIF_F_LOOPBACK (1 << 31) /* Enable loopback */
>
> +/*
> + * Bit 31 is for device to map userspace buffers -- zerocopy
> + * Device can set this flag when it supports HIGHDMA.
> + * Device can't recycle this kind of skb buffers.
> + * There are 256 bytes copied, the rest of buffers are mapped.
> + * The userspace callback should only be called when last reference to this skb
> + * is gone.
> + */
> +#define NETIF_F_ZEROCOPY (1 << 31)
Sorry, bit 31 is taken. You get the job of turning features into a
wider bitmap.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: linux-next: Tree for May 16 (net/ipv4/ping)
From: Randy Dunlap @ 2011-05-16 19:35 UTC (permalink / raw)
To: Stephen Rothwell, netdev; +Cc: linux-next, LKML
In-Reply-To: <20110516151019.be338f1c.sfr@canb.auug.org.au>
On Mon, 16 May 2011 15:10:19 +1000 Stephen Rothwell wrote:
> Hi all,
>
> Changes since 20110513:
when CONFIG_PROC_SYSCTL is not enabled:
ping.c:(.text+0x52af3): undefined reference to `inet_get_ping_group_range_net'
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* [PATCH V5 5/6 net-next] macvtap: macvtap TX zero-copy support
From: Shirley Ma @ 2011-05-16 19:36 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
Only when buffer size is greater than GOODCOPY_LEN (256), macvtap
enables zero-copy.
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
drivers/net/macvtap.c | 129 ++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 118 insertions(+), 11 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 6696e56..145e51c 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -60,6 +60,7 @@ static struct proto macvtap_proto = {
*/
static dev_t macvtap_major;
#define MACVTAP_NUM_DEVS 65536
+#define GOODCOPY_LEN 256
static struct class *macvtap_class;
static struct cdev macvtap_cdev;
@@ -340,6 +341,7 @@ static int macvtap_open(struct inode *inode, struct file *file)
{
struct net *net = current->nsproxy->net_ns;
struct net_device *dev = dev_get_by_index(net, iminor(inode));
+ struct macvlan_dev *vlan = netdev_priv(dev);
struct macvtap_queue *q;
int err;
@@ -369,6 +371,15 @@ static int macvtap_open(struct inode *inode, struct file *file)
q->flags = IFF_VNET_HDR | IFF_NO_PI | IFF_TAP;
q->vnet_hdr_sz = sizeof(struct virtio_net_hdr);
+ /*
+ * so far only KVM virtio_net uses macvtap, enable zero copy between
+ * guest kernel and host kernel when lower device supports zerocopy
+ */
+ if (vlan) {
+ if (vlan->lowerdev->features & NETIF_F_ZEROCOPY)
+ sock_set_flag(&q->sk, SOCK_ZEROCOPY);
+ }
+
err = macvtap_set_queue(dev, file, q);
if (err)
sock_put(&q->sk);
@@ -433,6 +444,80 @@ static inline struct sk_buff *macvtap_alloc_skb(struct sock *sk, size_t prepad,
return skb;
}
+/* set skb frags from iovec, this can move to core network code for reuse */
+static int zerocopy_sg_from_iovec(struct sk_buff *skb, const struct iovec *from,
+ int offset, size_t count)
+{
+ int len = iov_length(from, count) - offset;
+ int copy = skb_headlen(skb);
+ int size, offset1 = 0;
+ int i = 0;
+ skb_frag_t *f;
+
+ /* Skip over from offset */
+ while (count && (offset >= from->iov_len)) {
+ offset -= from->iov_len;
+ ++from;
+ --count;
+ }
+
+ /* copy up to skb headlen */
+ while (count && (copy > 0)) {
+ size = min_t(unsigned int, copy, from->iov_len - offset);
+ if (copy_from_user(skb->data + offset1, from->iov_base + offset,
+ size))
+ return -EFAULT;
+ if (copy > size) {
+ ++from;
+ --count;
+ }
+ copy -= size;
+ offset1 += size;
+ offset = 0;
+ }
+
+ if (len == offset1)
+ return 0;
+
+ while (count--) {
+ struct page *page[MAX_SKB_FRAGS];
+ int num_pages;
+ unsigned long base;
+
+ len = from->iov_len - offset1;
+ if (!len) {
+ offset1 = 0;
+ ++from;
+ continue;
+ }
+ base = (unsigned long)from->iov_base + offset1;
+ size = ((base & ~PAGE_MASK) + len + ~PAGE_MASK) >> PAGE_SHIFT;
+ num_pages = get_user_pages_fast(base, size, 0, &page[i]);
+ if ((num_pages != size) ||
+ (num_pages > MAX_SKB_FRAGS - skb_shinfo(skb)->nr_frags))
+ /* put_page is in skb free */
+ return -EFAULT;
+ skb->data_len += len;
+ skb->len += len;
+ skb->truesize += len;
+ atomic_add(len, &skb->sk->sk_wmem_alloc);
+ while (len) {
+ f = &skb_shinfo(skb)->frags[i];
+ f->page = page[i];
+ f->page_offset = base & ~PAGE_MASK;
+ f->size = min_t(int, len, PAGE_SIZE - f->page_offset);
+ skb_shinfo(skb)->nr_frags++;
+ /* increase sk_wmem_alloc */
+ base += f->size;
+ len -= f->size;
+ i++;
+ }
+ offset1 = 0;
+ ++from;
+ }
+ return 0;
+}
+
/*
* macvtap_skb_from_vnet_hdr and macvtap_skb_to_vnet_hdr should
* be shared with the tun/tap driver.
@@ -515,16 +600,18 @@ static int macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
/* Get packet from user space buffer */
-static ssize_t macvtap_get_user(struct macvtap_queue *q,
- const struct iovec *iv, size_t count,
- int noblock)
+static ssize_t macvtap_get_user(struct macvtap_queue *q, struct msghdr *m,
+ const struct iovec *iv, unsigned long total_len,
+ size_t count, int noblock)
{
struct sk_buff *skb;
struct macvlan_dev *vlan;
- size_t len = count;
+ unsigned long len = total_len;
int err;
struct virtio_net_hdr vnet_hdr = { 0 };
int vnet_hdr_len = 0;
+ int copylen;
+ bool zerocopy = false;
if (q->flags & IFF_VNET_HDR) {
vnet_hdr_len = q->vnet_hdr_sz;
@@ -552,12 +639,32 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,
if (unlikely(len < ETH_HLEN))
goto err;
- skb = macvtap_alloc_skb(&q->sk, NET_IP_ALIGN, len, vnet_hdr.hdr_len,
- noblock, &err);
+ if (m && m->msg_control)
+ zerocopy = true;
+
+ if (zerocopy) {
+ /* There are 256 bytes to be copied in skb, so there is enough
+ * room for skb expand head in case it is used.
+ * The rest buffer is mapped from userspace.
+ */
+ copylen = vnet_hdr.hdr_len;
+ if (!copylen)
+ copylen = GOODCOPY_LEN;
+ } else
+ copylen = len;
+
+ skb = macvtap_alloc_skb(&q->sk, NET_IP_ALIGN, copylen,
+ vnet_hdr.hdr_len, noblock, &err);
if (!skb)
goto err;
- err = skb_copy_datagram_from_iovec(skb, 0, iv, vnet_hdr_len, len);
+ if (zerocopy) {
+ err = zerocopy_sg_from_iovec(skb, iv, vnet_hdr_len, count);
+ memcpy(&skb_shinfo(skb)->ubuf, m->msg_control,
+ sizeof(struct skb_ubuf_info));
+ } else
+ err = skb_copy_datagram_from_iovec(skb, 0, iv, vnet_hdr_len,
+ len);
if (err)
goto err_kfree;
@@ -579,7 +686,7 @@ static ssize_t macvtap_get_user(struct macvtap_queue *q,
kfree_skb(skb);
rcu_read_unlock_bh();
- return count;
+ return total_len;
err_kfree:
kfree_skb(skb);
@@ -601,8 +708,8 @@ static ssize_t macvtap_aio_write(struct kiocb *iocb, const struct iovec *iv,
ssize_t result = -ENOLINK;
struct macvtap_queue *q = file->private_data;
- result = macvtap_get_user(q, iv, iov_length(iv, count),
- file->f_flags & O_NONBLOCK);
+ result = macvtap_get_user(q, NULL, iv, iov_length(iv, count), count,
+ file->f_flags & O_NONBLOCK);
return result;
}
@@ -815,7 +922,7 @@ static int macvtap_sendmsg(struct kiocb *iocb, struct socket *sock,
struct msghdr *m, size_t total_len)
{
struct macvtap_queue *q = container_of(sock, struct macvtap_queue, sock);
- return macvtap_get_user(q, m->msg_iov, total_len,
+ return macvtap_get_user(q, m, m->msg_iov, total_len, m->msg_iovlen,
m->msg_flags & MSG_DONTWAIT);
}
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Shirley Ma @ 2011-05-16 19:38 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann,
netdev, kvm, linux-kernel
In-Reply-To: <1305574518.2885.25.camel@bwh-desktop>
On Mon, 2011-05-16 at 20:35 +0100, Ben Hutchings wrote:
> Sorry, bit 31 is taken. You get the job of turning features into a
> wider bitmap.
:) will do it.
Thanks
Shirley
^ permalink raw reply
* Re: linux-next: Tree for May 16 (net/ipv4/ping)
From: David Miller @ 2011-05-16 19:38 UTC (permalink / raw)
To: randy.dunlap; +Cc: sfr, netdev, linux-next, linux-kernel, segoon
In-Reply-To: <20110516123534.5d3a51b9.randy.dunlap@oracle.com>
From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Mon, 16 May 2011 12:35:34 -0700
> On Mon, 16 May 2011 15:10:19 +1000 Stephen Rothwell wrote:
>
>> Hi all,
>>
>> Changes since 20110513:
>
>
> when CONFIG_PROC_SYSCTL is not enabled:
>
> ping.c:(.text+0x52af3): undefined reference to `inet_get_ping_group_range_net'
Vasiliy, please fix this.
^ permalink raw reply
* [PATCH V5 6/6 net-next] example: enable zero-copy support in ixgbe
From: Shirley Ma @ 2011-05-16 19:42 UTC (permalink / raw)
To: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann
Cc: netdev, kvm, linux-kernel
Device can enable zero-copy flag when HIGHDMA is supported.
Signed-off-by: Shirley Ma <xma@us.ibm.com>
---
drivers/net/ixgbe/ixgbe_main.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 2dce3d0..7e9e881 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -7553,6 +7553,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
netdev->vlan_features |= NETIF_F_HIGHDMA;
}
+ /* enable zero-copy when device supports HIGHDMA */
+ if (netdev->features & NETIF_F_HIGHDMA)
+ netdev->features |= NETIF_F_ZEROCOPY;
+
if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED)
netdev->features |= NETIF_F_LRO;
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Ben Hutchings @ 2011-05-16 19:47 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, mst, Eric Dumazet, Avi Kivity, Arnd Bergmann,
netdev, kvm, linux-kernel
In-Reply-To: <1305574680.3456.33.camel@localhost.localdomain>
On Mon, 2011-05-16 at 12:38 -0700, Shirley Ma wrote:
> On Mon, 2011-05-16 at 20:35 +0100, Ben Hutchings wrote:
> > Sorry, bit 31 is taken. You get the job of turning features into a
> > wider bitmap.
>
> :) will do it.
Bear in mind that feature masks are manipulated in many different
places. This is not a simple task.
See previous discussion at:
http://thread.gmane.org/gmane.linux.network/193284
and especially:
http://thread.gmane.org/gmane.linux.network/193284/focus=193332
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: several packets in a single buffer in Rx
From: Tomas Winkler @ 2011-05-16 20:04 UTC (permalink / raw)
To: Emmanuel Grumbach
Cc: Michał Mirosław, netdev, linux-wireless, Johannes Berg,
Guy, Wey-Yi, guy.cohen
In-Reply-To: <BANLkTin_stn+7Ja3hbuaOgtrEVJEoT9Lhw@mail.gmail.com>
2011/5/16 Emmanuel Grumbach <egrumbach@gmail.com>:
> 2011/5/16 Michał Mirosław <mirqus@gmail.com>:
>> W dniu 16 maja 2011 14:59 użytkownik Emmanuel Grumbach
>> <egrumbach@gmail.com> napisał:
>>> 2011/5/16 Michał Mirosław <mirqus@gmail.com>:
>>>> 2011/5/16 Emmanuel Grumbach <egrumbach@gmail.com>:
>>>>> I would like to be able to deliver the same page several times to the
>>>>> stack without having the stack consume it before the last time I
>>>>> deliver it.
>>>>> Of course I would like to avoid cloning it.
>>>>
>>>> Just do get_page() on the page having another packet in it before
>>>> passing skb up.
>>>>
>>>
>>> I can see the path:
>>> __kfree_skb -> skb_release_all -> skb_release_data -> put_page
>>> put_page will free the page iff the _count variable reaches 0. Of course,
>>> _count is incremented by get_page.
>>>
>>> I will give it try.
>>>
>>> I understand that this will work regardless the order given to
>>> alloc_pages right ?
>>
>> Yes. Remember that if you put a lot of packets in a big-order page
>> then the memory will be freed only after all packets are freed.
>
> Sure. Thanks for the help.
How it is ensured that skb manipulation won't corrupt another packet
on the same page?
Thanks
Tomas
^ permalink raw reply
* Re: several packets in a single buffer in Rx
From: Ben Hutchings @ 2011-05-16 20:15 UTC (permalink / raw)
To: Tomas Winkler
Cc: Emmanuel Grumbach, Michał Mirosław,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA, Johannes Berg, Guy, Wey-Yi,
guy.cohen-ral2JQCrhuEAvxtiuMwx3w
In-Reply-To: <BANLkTimvCVcQX-7teQgG7EQV9eeLgG3JnQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 2011-05-16 at 23:04 +0300, Tomas Winkler wrote:
> 2011/5/16 Emmanuel Grumbach <egrumbach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 2011/5/16 Michał Mirosław <mirqus-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> W dniu 16 maja 2011 14:59 użytkownik Emmanuel Grumbach
> >> <egrumbach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> napisał:
> >>> 2011/5/16 Michał Mirosław <mirqus-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >>>> 2011/5/16 Emmanuel Grumbach <egrumbach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >>>>> I would like to be able to deliver the same page several times to the
> >>>>> stack without having the stack consume it before the last time I
> >>>>> deliver it.
> >>>>> Of course I would like to avoid cloning it.
> >>>>
> >>>> Just do get_page() on the page having another packet in it before
> >>>> passing skb up.
> >>>>
> >>>
> >>> I can see the path:
> >>> __kfree_skb -> skb_release_all -> skb_release_data -> put_page
> >>> put_page will free the page iff the _count variable reaches 0. Of course,
> >>> _count is incremented by get_page.
> >>>
> >>> I will give it try.
> >>>
> >>> I understand that this will work regardless the order given to
> >>> alloc_pages right ?
> >>
> >> Yes. Remember that if you put a lot of packets in a big-order page
> >> then the memory will be freed only after all packets are freed.
> >
> > Sure. Thanks for the help.
>
> How it is ensured that skb manipulation won't corrupt another packet
> on the same page?
Fragments attached to an skb are always treated as read-only. The
headers will be copied into the skb's header buffer and may be modified
there.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [GIT] Networking
From: David Miller @ 2011-05-16 20:29 UTC (permalink / raw)
To: torvalds; +Cc: akpm, netdev, linux-kernel
1) SFC crashes on 'ethtool -d', from Ben Hutchings.
2) VMXNET3 driver doesn't set LRO correctly on probe, this hits a lot
of VMWare users. Fix from Thomas Jarosch.
3) IPVS needs to use correct SEQ release interface, otherwise it leaks
netns references. Fix from Hans Schillstrom.
4) Users can (via tun/tap for example) trigger the warnings generated
by the netdevice feature validation checks we make. Make such
warnings of DEBUG level so user's can spam the logs. Fix from
Michał Mirosław.
There is a dup of bridge netfilter fix (already in your tree) with
commit ID cb68552858c64db302771469b1202ea09e696329 in here because
Pablo applied it to the netfilter tree as well (and in his tree it
appears as commit d8083deb4f1aa0977980dfb834fcc336ef38318f). So when
I merged his tree to get the IPVS namespace leak fix, I got the bridge
netfilter fix dup too.
Please pull, thanks a lot!
The following changes since commit df8d06ade6eed9077f658ac8696fc1cb5c081220:
Merge branch 'omap-fixes-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6 (2011-05-16 08:55:49 -0700)
are available in the git repository at:
master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master
Ben Hutchings (1):
sfc: Fix oops in register dump after mapping change
David S. Miller (1):
Merge branch 'pablo/nf-2.6-updates' of git://1984.lsi.us.es/net-2.6
Hans Schillstrom (1):
IPVS: fix netns if reading ip_vs_* procfs entries
Michał Mirosław (1):
net: Change netdev_fix_features messages loglevel
Stephen Hemminger (1):
bridge: fix forwarding of IPv6
Thomas Jarosch (1):
vmxnet3: Fix inconsistent LRO state after initialization
drivers/net/sfc/nic.c | 7 +++++++
drivers/net/vmxnet3/vmxnet3_ethtool.c | 3 +++
net/core/dev.c | 22 ++++++++--------------
net/netfilter/ipvs/ip_vs_app.c | 2 +-
net/netfilter/ipvs/ip_vs_conn.c | 4 ++--
net/netfilter/ipvs/ip_vs_ctl.c | 6 +++---
6 files changed, 24 insertions(+), 20 deletions(-)
^ permalink raw reply
* Re: [PATCH] net: Change netdev_fix_features messages loglevel
From: Michael S. Tsirkin @ 2011-05-16 20:37 UTC (permalink / raw)
To: David Miller; +Cc: mirq-linux, netdev, herbert, bhutchings
In-Reply-To: <20110516.151434.829498612745581899.davem@davemloft.net>
On Mon, May 16, 2011 at 03:14:34PM -0400, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Mon, 16 May 2011 15:17:57 +0200 (CEST)
>
> > Those reduced to DEBUG can possibly be triggered by unprivileged processes
> > and are nothing exceptional. Illegal checksum combinations can only be
> > caused by driver bug, so promote those messages to WARN.
> >
> > Since GSO without SG will now only cause DEBUG message from
> > netdev_fix_features(), remove the workaround from register_netdevice().
> >
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>
> Applied, thanks.
Cool, how about we make 'Features changed' debug as well?
This way userspace can't fill up the log just by tweaking tun features
with an ioctl.
Untested, but you get the message.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/net/core/dev.c b/net/core/dev.c
index 3ed09f8..538a1fe 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5270,7 +5270,7 @@ int __netdev_update_features(struct net_device *dev)
if (dev->features == features)
return 0;
- netdev_info(dev, "Features changed: 0x%08x -> 0x%08x\n",
+ netdev_dbg(dev, "Features changed: 0x%08x -> 0x%08x\n",
dev->features, features);
if (dev->netdev_ops->ndo_set_features)
--
MST
^ permalink raw reply related
* Re: [PATCH 1/1] igmp: fix ip_mc_clear_src to not reset ip_mc_list->sf{mode,count}
From: David Stevens @ 2011-05-16 20:42 UTC (permalink / raw)
To: David Miller
Cc: jmorris, kaber, kuznet, linux-kbuild, linux-kernel, mmarek,
netdev, pekkas, vfalico, yoshfuji
In-Reply-To: <20110516.140359.111037536766782557.davem@davemloft.net>
> From: Veaceslav Falico <vfalico@redhat.com>
> Date: Sun, 15 May 2011 18:59:45 +0200
>
> > ip_mc_clear_src resets the imc->sfcount and imc->sfmode, without
taking into
> > account the current number of sockets listening on that multicast
> struct, which
> > can lead to bogus routes for local listeners.
> >
> > On NETDEV_DOWN/UP event, if there were 3 multicast listeners for
> that interface's
> > address, the imc->sfcount[MCAST_EXCLUDE] will be reset to 1. And
> after that a
> > listener socket destroys, multicast traffic will not be delivered to
local
> > listeners because __mkroute_output drops the local flag for the route
(by
> > checking ip_check_mc).
On NETDEV_DOWN, all group memberships are dropped.
ip_mc_clear_src()
is simply freeing all the source filters and turning it into an "EXCLUDE
nobody"
membership (ie, the same as an ordinary join without source filtering).
This
ordinarily happens when you are deleting the group entirely (when the
reference
count goes to 0), but is also called on device down.
This patch is not appropriate; when the groups are deleted, the
source
filters are deleted, and the filter counts have to reflect the source
filters
in the list. If you had an "INCLUDE A" filter, for example, that would
become
an "INCLUDE nobody" filter and drop all traffic (from A or not). The
number
of source filters is not related to the number of listener sockets, and
the
function of ip_mc_clear_src() is to make it 0 (with the special case of 1
for
EXCLUDE), so setting the counts has to be done for proper functioning.
I don't quite understand the problem you're trying to solve here
--
when the device comes back up, the group should be re-added with
{EXCLUDE,nobody} and
ip_check_mc() should therefore return 1. Of course, while the interface is
down, the mc_list is empty and it'd return 0 in that case.
Do you have a small test program to demonstrate the problem?
For the patch, I have to say NACK.
+-DLS
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Michael S. Tsirkin @ 2011-05-16 20:45 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <1305574484.3456.30.camel@localhost.localdomain>
> +/* Since we need to keep the order of used_idx as avail_idx, it's possible that
> + * DMA done not in order in lower device driver for some reason. To prevent
> + * used_idx out of order, upend_idx is used to track avail_idx order, done_idx
> + * is used to track used_idx order. Once lower device DMA done, then upend_idx
> + * can move to done_idx.
Could you clarify this please? virtio explicitly allows out of order
completion of requests. Does it simplify code that we try to keep
used index updates in-order? Because if not, this is not
really a requirement.
--
MST
^ permalink raw reply
* Re: kernel bug relating to networking
From: James @ 2011-05-16 20:47 UTC (permalink / raw)
Cc: Kernel Mailing List, netdev
In-Reply-To: <BANLkTikF3ntwWjV3iuwwCnJhZKJMVHHygQ@mail.gmail.com>
On 05/16/11 11:47, Daniel Baluta wrote:
> On Mon, May 16, 2011 at 6:35 PM, James <bjlockie@lockie.ca> wrote:
>> I originally posted to linux-net@vger.kernel.org but all that list
> This should be netdev@vger.kernel.org.
That is confusing since the welcome message said: "welcome message for
linux-net@vger.kernel.org".
Both lists are listed at http://vger.kernel.org/vger-lists.html but it
doesn't say the difference.
^ permalink raw reply
* Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return
From: Michał Mirosław @ 2011-05-16 20:51 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <1305557597.2885.5.camel@bwh-desktop>
On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > all drivers are converted to fix/set_features.
> > > >
> > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > ---
> > > >
> > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > [...]
> > > ETHTOOL_F_WISH means that the requested features could not all be
> > > enabled, *but are remembered*. ETHTOOL_F_COMPAT means they were not
> > > remembered.
> > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > (net: Implement SFEATURES compatibility for not updated drivers).
> That's also problematic because it means we can't make any use of the
> 'available' masks from ETHTOOL_GFEATURES.
>
> The patch I sent is actually tested with a modified ethtool. The
> fallback works. I don't think you've tested whether any of your
> proposals can actually practically be used by ethtool.
While reading your patches I noted some differences in the way we see
the new [GS]FEATURES ops.
First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
feature names become an ABI instead. That's what ETH_SS_FEATURES string
set is for, and that's what comments in kernel's <linux/ethtool.h>
include say.
dev->features are exposed directly by kernel only in two ways:
1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
in headers for userspace, this should be treated like a debugging
facility and not an ABI
2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
and RX hashing) that are renamed to ETH_FLAG_* - only those constants
are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel? The
assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
but there is an conversion layer in kernel that allows old binaries to
work correctly in the common case. (-EOPNOTSUPP is still returned for
drivers which can't change particular feature. The difference is seen
only in that disabling and enabling e.g. checksumming won't disable other
dependent features in the result.)
Right now we already agree that NETIF_F_COMPAT should go.
I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
This might be made even more useful by adding simple wildcard matching.
Best Regards,
Michał Mirosław
^ permalink raw reply
* [RFC PATCH ethtool] ethtool: implement G/SFEATURES calls
From: Michał Mirosław @ 2011-05-16 20:54 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, David Miller
In-Reply-To: <20110516205137.GA7667@rere.qmqm.pl>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
ethtool.c | 299 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 293 insertions(+), 6 deletions(-)
diff --git a/ethtool.c b/ethtool.c
index 34fe107..86a5a8b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -33,6 +33,7 @@
#include <limits.h>
#include <ctype.h>
+#include <unistd.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <arpa/inet.h>
@@ -83,6 +84,9 @@ static int do_gcoalesce(int fd, struct ifreq *ifr);
static int do_scoalesce(int fd, struct ifreq *ifr);
static int do_goffload(int fd, struct ifreq *ifr);
static int do_soffload(int fd, struct ifreq *ifr);
+static void parse_sfeatures_args(int argc, char **argp, int argi);
+static int do_gfeatures(int fd, struct ifreq *ifr);
+static int do_sfeatures(int fd, struct ifreq *ifr);
static int do_gstats(int fd, struct ifreq *ifr);
static int rxflow_str_to_type(const char *str);
static int parse_rxfhashopts(char *optstr, u32 *data);
@@ -119,6 +123,8 @@ static enum {
MODE_SRING,
MODE_GOFFLOAD,
MODE_SOFFLOAD,
+ MODE_GFEATURES,
+ MODE_SFEATURES,
MODE_GSTATS,
MODE_GNFC,
MODE_SNFC,
@@ -197,6 +203,10 @@ static struct option {
" [ ntuple on|off ]\n"
" [ rxhash on|off ]\n"
},
+ { "-w", "--show-features", MODE_GFEATURES, "Get offload status" },
+ { "-W", "--request-features", MODE_SFEATURES, "Set requested offload",
+ " [ feature-name on|off [...] ]\n"
+ " see --show-features output for feature-name strings\n" },
{ "-i", "--driver", MODE_GDRV, "Show driver information" },
{ "-d", "--register-dump", MODE_GREGS, "Do a register dump",
" [ raw on|off ]\n"
@@ -768,6 +778,8 @@ static void parse_cmdline(int argc, char **argp)
(mode == MODE_SRING) ||
(mode == MODE_GOFFLOAD) ||
(mode == MODE_SOFFLOAD) ||
+ (mode == MODE_GFEATURES) ||
+ (mode == MODE_SFEATURES) ||
(mode == MODE_GSTATS) ||
(mode == MODE_GNFC) ||
(mode == MODE_SNFC) ||
@@ -858,6 +870,11 @@ static void parse_cmdline(int argc, char **argp)
i = argc;
break;
}
+ if (mode == MODE_SFEATURES) {
+ parse_sfeatures_args(argc, argp, i);
+ i = argc;
+ break;
+ }
if (mode == MODE_SCLSRULE) {
if (!strcmp(argp[i], "flow-type")) {
i += 1;
@@ -1867,21 +1884,30 @@ static int dump_rxfhash(int fhash, u64 val)
return 0;
}
-static int doit(void)
+static int get_control_socket(struct ifreq *ifr)
{
- struct ifreq ifr;
int fd;
/* Setup our control structures. */
- memset(&ifr, 0, sizeof(ifr));
- strcpy(ifr.ifr_name, devname);
+ memset(ifr, 0, sizeof(*ifr));
+ strcpy(ifr->ifr_name, devname);
/* Open control socket. */
fd = socket(AF_INET, SOCK_DGRAM, 0);
- if (fd < 0) {
+ if (fd < 0)
perror("Cannot get control socket");
+
+ return fd;
+}
+
+static int doit(void)
+{
+ struct ifreq ifr;
+ int fd;
+
+ fd = get_control_socket(&ifr);
+ if (fd < 0)
return 70;
- }
/* all of these are expected to populate ifr->ifr_data as needed */
if (mode == MODE_GDRV) {
@@ -1918,6 +1944,10 @@ static int doit(void)
return do_goffload(fd, &ifr);
} else if (mode == MODE_SOFFLOAD) {
return do_soffload(fd, &ifr);
+ } else if (mode == MODE_GFEATURES) {
+ return do_gfeatures(fd, &ifr);
+ } else if (mode == MODE_SFEATURES) {
+ return do_sfeatures(fd, &ifr);
} else if (mode == MODE_GSTATS) {
return do_gstats(fd, &ifr);
} else if (mode == MODE_GNFC) {
@@ -2355,6 +2385,263 @@ static int do_soffload(int fd, struct ifreq *ifr)
return 0;
}
+static int get_feature_strings(int fd, struct ifreq *ifr,
+ struct ethtool_gstrings **strs)
+{
+ struct ethtool_sset_info *sset_info;
+ struct ethtool_gstrings *strings;
+ int sz_str, n_strings, err;
+
+ sset_info = malloc(sizeof(struct ethtool_sset_info) + sizeof(u32));
+ sset_info->cmd = ETHTOOL_GSSET_INFO;
+ sset_info->sset_mask = (1ULL << ETH_SS_FEATURES);
+ ifr->ifr_data = (caddr_t)sset_info;
+ err = send_ioctl(fd, ifr);
+
+ if ((err < 0) ||
+ (!(sset_info->sset_mask & (1ULL << ETH_SS_FEATURES)))) {
+ perror("Cannot get driver strings info");
+ return -100;
+ }
+
+ n_strings = sset_info->data[0];
+ free(sset_info);
+ sz_str = n_strings * ETH_GSTRING_LEN;
+
+ strings = calloc(1, sz_str + sizeof(struct ethtool_gstrings));
+ if (!strings) {
+ fprintf(stderr, "no memory available\n");
+ return -95;
+ }
+
+ strings->cmd = ETHTOOL_GSTRINGS;
+ strings->string_set = ETH_SS_FEATURES;
+ strings->len = n_strings;
+ ifr->ifr_data = (caddr_t) strings;
+ err = send_ioctl(fd, ifr);
+ if (err < 0) {
+ perror("Cannot get feature strings information");
+ free(strings);
+ return -96;
+ }
+
+ *strs = strings;
+ return n_strings;
+}
+
+struct ethtool_sfeatures *features_req;
+
+static void parse_sfeatures_args(int argc, char **argp, int argi)
+{
+ struct cmdline_info *cmdline_desc, *cp;
+ struct ethtool_gstrings *strings;
+ struct ifreq ifr;
+ int n_strings, sz_features, i;
+ int fd, changed = 0;
+
+ fd = get_control_socket(&ifr);
+ if (fd < 0)
+ exit(100);
+
+ n_strings = get_feature_strings(fd, &ifr, &strings);
+ if (n_strings < 0)
+ exit(-n_strings);
+
+ sz_features = sizeof(*features_req->features) * ((n_strings + 31) / 32);
+
+ cp = cmdline_desc = calloc(n_strings, sizeof(*cmdline_desc));
+ features_req = calloc(1, sizeof(*features_req) + sz_features);
+ if (!cmdline_desc || !features_req) {
+ fprintf(stderr, "no memory available\n");
+ exit(95);
+ }
+
+ features_req->size = (n_strings + 31) / 32;
+
+ for (i = 0; i < n_strings; ++i) {
+ if (!strings->data[i*ETH_GSTRING_LEN])
+ continue;
+
+ strings->data[i*ETH_GSTRING_LEN + ETH_GSTRING_LEN-1] = 0;
+ cp->name = (const char *)strings->data + i * ETH_GSTRING_LEN;
+ cp->type = CMDL_FLAG;
+ cp->flag_val = 1 << (i % 32);
+ cp->wanted_val = &features_req->features[i / 32].requested;
+ cp->seen_val = &features_req->features[i / 32].valid;
+ ++cp;
+ }
+
+ parse_generic_cmdline(argc, argp, argi, &changed,
+ cmdline_desc, cp - cmdline_desc);
+
+ free(cmdline_desc);
+ free(strings);
+ close(fd);
+
+ if (!changed) {
+ free(features_req);
+ features_req = NULL;
+ }
+}
+
+static int send_gfeatures(int fd, struct ifreq *ifr, int n_strings,
+ struct ethtool_gfeatures **features_p)
+{
+ struct ethtool_gfeatures *features;
+ int err, sz_features;
+
+ sz_features = sizeof(*features->features) * ((n_strings + 31) / 32);
+ features = calloc(1, sz_features + sizeof(*features));
+ if (!features) {
+ fprintf(stderr, "no memory available\n");
+ return 95;
+ }
+
+ features->cmd = ETHTOOL_GFEATURES;
+ features->size = (n_strings + 31) / 32;
+ ifr->ifr_data = (caddr_t) features;
+ err = send_ioctl(fd, ifr);
+
+ if (err < 0) {
+ perror("Cannot get feature status");
+ free(features);
+ return 97;
+ }
+
+ *features_p = features;
+ return 0;
+}
+
+static int do_gfeatures(int fd, struct ifreq *ifr)
+{
+ struct ethtool_gstrings *strings;
+ struct ethtool_gfeatures *features;
+ int n_strings, err, i;
+
+ n_strings = get_feature_strings(fd, ifr, &strings);
+ if (n_strings < 0)
+ return -n_strings;
+
+ err = send_gfeatures(fd, ifr, n_strings, &features);
+ if (err) {
+ free(strings);
+ return err;
+ }
+
+ fprintf(stdout, "Offload state: (name: enabled,wanted,changable)\n");
+ for (i = 0; i < n_strings; i++) {
+ if (!strings->data[i * ETH_GSTRING_LEN])
+ continue; /* empty */
+#define P_FLAG(f) \
+ (features->features[i / 32].f & (1 << (i % 32))) ? "yes" : " no"
+#define PA_FLAG(f) \
+ (features->features[i / 32].available & (1 << (i % 32))) ? P_FLAG(f) : "---"
+#define PN_FLAG(f) \
+ (features->features[i / 32].never_changed & (1 << (i % 32))) ? "---" : P_FLAG(f)
+ fprintf(stdout, " %-*.*s %s,%s,%s\n",
+ ETH_GSTRING_LEN, ETH_GSTRING_LEN,
+ &strings->data[i * ETH_GSTRING_LEN],
+ P_FLAG(active), PA_FLAG(requested), PN_FLAG(available));
+#undef P_FLAG
+#undef PA_FLAG
+#undef PN_FLAG
+ }
+ free(strings);
+ free(features);
+
+ return 0;
+}
+
+static void print_gfeatures_diff(
+ const struct ethtool_get_features_block *expected,
+ const struct ethtool_get_features_block *set,
+ const char *strings, int n_strings)
+{
+ int i;
+
+ if (n_strings > 32)
+ n_strings = 32;
+
+ for (i = 0; i < n_strings; ++i) {
+ const char *name = &strings[i * ETH_GSTRING_LEN];
+ u32 mask = 1 << i;
+
+ if (!((expected->active ^ set->active) & mask))
+ continue;
+
+ fprintf(stderr, "feature %.*s is %s (expected: %s, saved: %s)\n",
+ ETH_GSTRING_LEN, name,
+ set->active & mask ? "enabled" : "disabled",
+ expected->active & mask ? "enabled" : "disabled",
+ !(set->available & mask) ? "not user-changeable" :
+ set->requested & mask ? "enabled" : "disabled"
+ );
+ }
+}
+
+static int do_sfeatures(int fd, struct ifreq *ifr)
+{
+ struct ethtool_gstrings *strings;
+ struct ethtool_gfeatures *features0, *features1;
+ int n_strings, err, i;
+
+ if (!features_req) {
+ fprintf(stderr, "no features changed\n");
+ return 97;
+ }
+
+ n_strings = get_feature_strings(fd, ifr, &strings);
+ if (n_strings < 0) {
+ free(features_req);
+ return -n_strings;
+ }
+
+ err = send_gfeatures(fd, ifr, n_strings, &features0);
+ if (err) {
+ perror("Cannot read features");
+ goto free_strings;
+ }
+
+ features_req->cmd = ETHTOOL_SFEATURES;
+ ifr->ifr_data = (caddr_t) features_req;
+ err = send_ioctl(fd, ifr);
+ if (err < 0) {
+ perror("Cannot change features");
+ err = 97;
+ goto free_features;
+ }
+
+ err = send_gfeatures(fd, ifr, n_strings, &features1);
+ if (err) {
+ perror("Cannot verify features");
+ goto free_features;
+ }
+
+ /* make features0 .active what we expect to be set */
+ i = (n_strings + 31) / 32;
+ while (i--) {
+ features0->features[i].active &= ~features_req->features[i].valid;
+ features0->features[i].active |=
+ features_req->features[i].requested &
+ features_req->features[i].valid;
+ }
+
+ for (i = 0; i < n_strings; i += 32)
+ print_gfeatures_diff(&features0->features[i / 32],
+ &features1->features[i / 32],
+ (char *)&strings->data[i * ETH_GSTRING_LEN],
+ n_strings - i);
+
+ free(features1);
+free_features:
+ free(features0);
+free_strings:
+ free(strings);
+ free(features_req);
+ return err;
+}
+
+
static int do_gset(int fd, struct ifreq *ifr)
{
int err;
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Shirley Ma @ 2011-05-16 20:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <20110516204540.GD18148@redhat.com>
On Mon, 2011-05-16 at 23:45 +0300, Michael S. Tsirkin wrote:
> > +/* Since we need to keep the order of used_idx as avail_idx, it's
> possible that
> > + * DMA done not in order in lower device driver for some reason. To
> prevent
> > + * used_idx out of order, upend_idx is used to track avail_idx
> order, done_idx
> > + * is used to track used_idx order. Once lower device DMA done,
> then upend_idx
> > + * can move to done_idx.
>
> Could you clarify this please? virtio explicitly allows out of order
> completion of requests. Does it simplify code that we try to keep
> used index updates in-order? Because if not, this is not
> really a requirement.
Hello Mike,
Based on my testing, vhost_add_used() must be in order from
vhost_get_vq_desc(). Otherwise, virtio_net ring seems get double
freed. I didn't spend time on debugging this.
in virtqueue_get_buf
if (unlikely(!vq->data[i])) {
BAD_RING(vq, "id %u is not a head!\n", i);
return NULL;
}
That's the reason I created the upend_idx and done_idx.
Thanks
Shirley
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox