[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC Patch v2 03/10] virtio-net rsc: Chain Lookup, Pack
|
From: |
Wei Xu |
|
Subject: |
Re: [Qemu-devel] [RFC Patch v2 03/10] virtio-net rsc: Chain Lookup, Packet Caching and Framework of IPv4 |
|
Date: |
Mon, 01 Feb 2016 16:02:58 +0800 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
On 02/01/2016 01:55 PM, Jason Wang wrote:
On 02/01/2016 02:13 AM, address@hidden wrote:
From: Wei Xu <address@hidden>
Upon a packet is arriving, a corresponding chain will be selected or created,
or be bypassed if it's not an IPv4 packets.
The callback in the chain will be invoked to call the real coalescing.
Since the coalescing is based on the TCP connection, so the packets will be
cached if there is no previous data within the same connection.
The framework of IPv4 is also introduced.
This patch depends on patch 2918cf2 (Detailed IPv4 and General TCP data
coalescing)
Then looks like the order needs to be changed?
OK, as mentioned in other feedbacks, some of the patches should be merged, will
adjust the patch set again, thanks.
Signed-off-by: Wei Xu <address@hidden>
---
hw/net/virtio-net.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 172 insertions(+), 1 deletion(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4e9458e..cfbac6d 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -14,10 +14,12 @@
#include "qemu/iov.h"
#include "hw/virtio/virtio.h"
#include "net/net.h"
+#include "net/eth.h"
#include "net/checksum.h"
#include "net/tap.h"
#include "qemu/error-report.h"
#include "qemu/timer.h"
+#include "qemu/sockets.h"
#include "hw/virtio/virtio-net.h"
#include "net/vhost_net.h"
#include "hw/virtio/virtio-bus.h"
@@ -37,6 +39,21 @@
#define endof(container, field) \
(offsetof(container, field) + sizeof(((container *)0)->field))
+#define VIRTIO_HEADER 12 /* Virtio net header size */
This looks wrong if mrg_rxbuf (VIRTIO_NET_F_MRG_RXBUF) is off
OK.
+#define IP_OFFSET (VIRTIO_HEADER + sizeof(struct eth_header))
+
+#define MAX_VIRTIO_IP_PAYLOAD (65535 + IP_OFFSET)
+
+/* Global statistics */
+static uint32_t rsc_chain_no_mem;
This is meaningless, see below comments.
Yes, should remove this.
+
+/* Switcher to enable/disable rsc */
+static bool virtio_net_rsc_bypass;
+
+/* Coalesce callback for ipv4/6 */
+typedef int32_t (VirtioNetCoalesce) (NetRscChain *chain, NetRscSeg *seg,
+ const uint8_t *buf, size_t size);
+
typedef struct VirtIOFeature {
uint32_t flags;
size_t end;
@@ -1019,7 +1036,8 @@ static int receive_filter(VirtIONet *n, const uint8_t
*buf, int size)
return 0;
}
-static ssize_t virtio_net_receive(NetClientState *nc, const uint8_t *buf, size_t size)
+static ssize_t virtio_net_do_receive(NetClientState *nc,
+ const uint8_t *buf, size_t size)
{
VirtIONet *n = qemu_get_nic_opaque(nc);
VirtIONetQueue *q = virtio_net_get_subqueue(nc);
@@ -1623,6 +1641,159 @@ static void virtio_net_rsc_cleanup(VirtIONet *n)
}
}
+static int virtio_net_rsc_cache_buf(NetRscChain *chain, NetClientState *nc,
+ const uint8_t *buf, size_t size)
+{
+ NetRscSeg *seg;
+
+ seg = g_malloc(sizeof(NetRscSeg));
+ if (!seg) {
+ return 0;
+ }
g_malloc() can't fail, no need to check if it succeeded.
OK.
+
+ seg->buf = g_malloc(MAX_VIRTIO_IP_PAYLOAD);
+ if (!seg->buf) {
+ goto out;
+ }
+
+ memmove(seg->buf, buf, size);
+ seg->size = size;
+ seg->dup_ack_count = 0;
+ seg->is_coalesced = 0;
+ seg->nc = nc;
+
+ QTAILQ_INSERT_TAIL(&chain->buffers, seg, next);
+ return size;
+
+out:
+ g_free(seg);
+ return 0;
+}
+
+
+static int32_t virtio_net_rsc_try_coalesce4(NetRscChain *chain,
+ NetRscSeg *seg, const uint8_t *buf, size_t size)
+{
+ /* This real part of this function will be introduced in next patch, just
+ * return a 'final' to feed the compilation. */
+ return RSC_FINAL;
+}
+
+static size_t virtio_net_rsc_callback(NetRscChain *chain, NetClientState *nc,
+ const uint8_t *buf, size_t size, VirtioNetCoalesce *coalesce)
+{
Looks like this function was called directly, so "callback" suffix is
not accurate.
OK.
+ int ret;
+ NetRscSeg *seg, *nseg;
+
+ if (QTAILQ_EMPTY(&chain->buffers)) {
+ if (!virtio_net_rsc_cache_buf(chain, nc, buf, size)) {
+ return 0;
+ } else {
+ return size;
+ }
+ }
+
+ QTAILQ_FOREACH_SAFE(seg, &chain->buffers, next, nseg) {
+ ret = coalesce(chain, seg, buf, size);
+ if (RSC_FINAL == ret) {
Let's use "ret == RSC_FINAL" for a consistent coding style with other
qemu codes.
OK.
+ ret = virtio_net_do_receive(seg->nc, seg->buf, seg->size);
+ QTAILQ_REMOVE(&chain->buffers, seg, next);
+ g_free(seg->buf);
+ g_free(seg);
+ if (ret == 0) {
+ /* Send failed */
+ return 0;
+ }
+
+ /* Send current packet */
+ return virtio_net_do_receive(nc, buf, size);
+ } else if (RSC_NO_MATCH == ret) {
+ continue;
+ } else {
+ /* Coalesced, mark coalesced flag to tell calc cksum for ipv4 */
+ seg->is_coalesced = 1;
+ return size;
+ }
+ }
+
+ return virtio_net_rsc_cache_buf(chain, nc, buf, size);
Maybe you can move the seg traversing info coalesce() function? This can
greatly simplify the codes above? (E.g no need to call
virtio_net_rsc_cache_buf() in two places?)
Good idea.
+}
+
+static size_t virtio_net_rsc_receive4(void *opq, NetClientState* nc,
+ const uint8_t *buf, size_t size)
+{
+ NetRscChain *chain;
+
+ chain = (NetRscChain *)opq;
+ return virtio_net_rsc_callback(chain, nc, buf, size,
+ virtio_net_rsc_try_coalesce4);
+}
+
+static NetRscChain *virtio_net_rsc_lookup_chain(NetClientState *nc,
+ uint16_t proto)
+{
+ VirtIONet *n;
+ NetRscChain *chain;
+ NICState *nic;
+
+ /* Only handle IPv4/6 */
+ if (proto != (uint16_t)ETH_P_IP) {
The comments is inconsistent with code, the code can only handle IPv4
currently.
OK.
+ return NULL;
+ }
+
+ nic = (NICState *)nc;
This cast is wrong for multiqueue backend. You can refer the exist
virtio_net_receive() for how to vet VirtIONet structure. E.g:
...
VirtIONet *n = qemu_get_nic_opaque(nc);
OK, will check it out.
...
+ n = container_of(&nic, VirtIONet, nic);
+ QTAILQ_FOREACH(chain, &n->rsc_chains, next) {
+ if (chain->proto == proto) {
+ return chain;
+ }
+ }
+
+ chain = g_malloc(sizeof(*chain));
+ if (!chain) {
+ rsc_chain_no_mem++;
Since g_malloc() can't fail, this is meaningless.
OK.
+ return NULL;
+ }
+
+ chain->proto = proto;
+ chain->do_receive = virtio_net_rsc_receive4;
+
+ QTAILQ_INIT(&chain->buffers);
+ QTAILQ_INSERT_TAIL(&n->rsc_chains, chain, next);
+ return chain;
+}
Better to split the chain initialization from lookup. And we can
initialize ipv4 chain during initialization.
Since the allocation happens really seldom, is it ok to keep the mechanism to
make the logic clean?
+
+static ssize_t virtio_net_rsc_receive(NetClientState *nc,
+ const uint8_t *buf, size_t size)
+{
+ uint16_t proto;
+ NetRscChain *chain;
+ struct eth_header *eth;
+
+ if (size < IP_OFFSET) {
+ return virtio_net_do_receive(nc, buf, size);
+ }
+
+ eth = (struct eth_header *)(buf + VIRTIO_HEADER);
+ proto = htons(eth->h_proto);
+ chain = virtio_net_rsc_lookup_chain(nc, proto);
+ if (!chain) {
+ return virtio_net_do_receive(nc, buf, size);
+ } else {
+ return chain->do_receive(chain, nc, buf, size);
+ }
+}
+
+static ssize_t virtio_net_receive(NetClientState *nc,
+ const uint8_t *buf, size_t size)
+{
+ if (virtio_net_rsc_bypass) {
+ return virtio_net_do_receive(nc, buf, size);
+ } else {
+ return virtio_net_rsc_receive(nc, buf, size);
+ }
+}
+
static NetClientInfo net_virtio_info = {
.type = NET_CLIENT_OPTIONS_KIND_NIC,
.size = sizeof(NICState),