Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend
2014/1/13 Stefan Hajnoczi stefa...@gmail.com
On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
I was trying to figure out whether it's okay for this function to be a
nop. I've come to the conclusion that it's okay:
If the netdev supports vnet_hdr then we enable vnet_hdr. If not, it
will not enable it.
Other NICs never enable vnet_hdr even if the netdev supports it.
This interface is basically useless because set_vnet_hdr_len() is
also needed and provides the same information, i.e. that we are
transitioning from vnet_hdr off - on.
The bool argument is misleading since we never use this function to
disable vnet_hdr. It's impossible to transition on - off.
I suggest removing using_vnet_hdr() and instead relying solely on
set_vnet_hdr_len(). Do this as the first patch in the series before
introducing the function pointers, that way all your following patches
become simpler.
Completely agree. As usual, I didn't want to change too much the existing
code.
This means that I have to remove the tap_using_vnet_hdr() calls from
virtio-net and vmxnet3 NICs before this patches, haven't I?
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4,
int tso6,
+ int ecn, int ufo)
+{
+}
This interface is necessary for toggling offload features at runtime,
e.g. because ethtool was used inside the guest. Offloads can impact
performance and sometimes expose bugs. Therefore users may wish to
disable certain offloads.
Please consider supporting set_offload()!
Yes, we are considering to do that, but at the moment there is no such a
support.
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+ offset of 'me-ifname'. */
+memset(req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s-me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_OFFSET;
+req.nr_arg1 = len;
+err = ioctl(s-me.fd, NIOCREGIF, req);
+if (err) {
+error_report(Unable to execute NETMAP_BDG_OFFSET on %s: %s,
+ s-me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length, may be usefule in the
future. */
s/usefule/useful/
--
Vincenzo Maffione
Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend
On Mon, Jan 13, 2014 at 04:11:25PM +0100, Vincenzo Maffione wrote:
2014/1/13 Stefan Hajnoczi stefa...@gmail.com
On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
I was trying to figure out whether it's okay for this function to be a
nop. I've come to the conclusion that it's okay:
If the netdev supports vnet_hdr then we enable vnet_hdr. If not, it
will not enable it.
Other NICs never enable vnet_hdr even if the netdev supports it.
This interface is basically useless because set_vnet_hdr_len() is
also needed and provides the same information, i.e. that we are
transitioning from vnet_hdr off - on.
The bool argument is misleading since we never use this function to
disable vnet_hdr. It's impossible to transition on - off.
I suggest removing using_vnet_hdr() and instead relying solely on
set_vnet_hdr_len(). Do this as the first patch in the series before
introducing the function pointers, that way all your following patches
become simpler.
Completely agree. As usual, I didn't want to change too much the existing
code.
This means that I have to remove the tap_using_vnet_hdr() calls from
virtio-net and vmxnet3 NICs before this patches, haven't I?
Yep. I suggest starting this patch series with a few cleanup patches to
get the tap_*() interface into shape.
Then you can move that cleaned-up interface to NetClient in the second
half of the patch series. By doing the cleanup first, you make the
second half simpler.
Stefan
Re: [Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend
On Fri, Dec 13, 2013 at 01:05:02PM +0100, Vincenzo Maffione wrote:
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
I was trying to figure out whether it's okay for this function to be a
nop. I've come to the conclusion that it's okay:
If the netdev supports vnet_hdr then we enable vnet_hdr. If not, it
will not enable it.
Other NICs never enable vnet_hdr even if the netdev supports it.
This interface is basically useless because set_vnet_hdr_len() is
also needed and provides the same information, i.e. that we are
transitioning from vnet_hdr off - on.
The bool argument is misleading since we never use this function to
disable vnet_hdr. It's impossible to transition on - off.
I suggest removing using_vnet_hdr() and instead relying solely on
set_vnet_hdr_len(). Do this as the first patch in the series before
introducing the function pointers, that way all your following patches
become simpler.
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int
tso6,
+ int ecn, int ufo)
+{
+}
This interface is necessary for toggling offload features at runtime,
e.g. because ethtool was used inside the guest. Offloads can impact
performance and sometimes expose bugs. Therefore users may wish to
disable certain offloads.
Please consider supporting set_offload()!
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+ offset of 'me-ifname'. */
+memset(req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s-me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_OFFSET;
+req.nr_arg1 = len;
+err = ioctl(s-me.fd, NIOCREGIF, req);
+if (err) {
+error_report(Unable to execute NETMAP_BDG_OFFSET on %s: %s,
+ s-me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length, may be usefule in the future. */
s/usefule/useful/
[Qemu-devel] [PATCH 4/5] net: add offloadings support to netmap backend
Whit this patch, the netmap backend supports TSO/UFO/CSUM
offloadings, and accepts the virtio-net header, similarly to what
happens with TAP. The offloading callbacks in the NetClientInfo
interface have been implemented.
Signed-off-by: Vincenzo Maffione v.maffi...@gmail.com
---
net/netmap.c | 64 +++-
1 file changed, 63 insertions(+), 1 deletion(-)
diff --git a/net/netmap.c b/net/netmap.c
index 0ccc497..f02a574 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -54,6 +54,7 @@ typedef struct NetmapState {
boolread_poll;
boolwrite_poll;
struct ioveciov[IOV_MAX];
+int vnet_hdr_len; /* Current virtio-net header length. */
} NetmapState;
#define D(format, ...) \
@@ -274,7 +275,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc,
return iov_size(iov, iovcnt);
}
-i = ring-cur;
+last = i = ring-cur;
avail = ring-avail;
if (avail iovcnt) {
@@ -394,6 +395,60 @@ static void netmap_cleanup(NetClientState *nc)
s-me.fd = -1;
}
+/* Offloading manipulation support callbacks.
+ *
+ * Currently we are simply able to pass the virtio-net header
+ * throughout the VALE switch, without modifying it or interpreting it.
+ * For this reason we are always able to support TSO/UFO/CSUM and
+ * different header lengths as long as two virtio-net-header-capable
+ * VMs are attached to the bridge.
+ */
+static bool netmap_has_ufo(NetClientState *nc)
+{
+return true;
+}
+
+static int netmap_has_vnet_hdr(NetClientState *nc)
+{
+return true;
+}
+
+static int netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+{
+return len = 0 len = NETMAP_BDG_MAX_OFFSET;
+}
+
+static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
+{
+}
+
+static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int
tso6,
+ int ecn, int ufo)
+{
+}
+
+static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
+{
+NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+int err;
+struct nmreq req;
+
+/* Issue a NETMAP_BDG_OFFSET command to change the netmap adapter
+ offset of 'me-ifname'. */
+memset(req, 0, sizeof(req));
+pstrcpy(req.nr_name, sizeof(req.nr_name), s-me.ifname);
+req.nr_version = NETMAP_API;
+req.nr_cmd = NETMAP_BDG_OFFSET;
+req.nr_arg1 = len;
+err = ioctl(s-me.fd, NIOCREGIF, req);
+if (err) {
+error_report(Unable to execute NETMAP_BDG_OFFSET on %s: %s,
+ s-me.ifname, strerror(errno));
+} else {
+/* Keep track of the current length, may be usefule in the future. */
+s-vnet_hdr_len = len;
+}
+}
/* NetClientInfo methods */
static NetClientInfo net_netmap_info = {
@@ -403,6 +458,12 @@ static NetClientInfo net_netmap_info = {
.receive_iov = netmap_receive_iov,
.poll = netmap_poll,
.cleanup = netmap_cleanup,
+.has_ufo = netmap_has_ufo,
+.has_vnet_hdr = netmap_has_vnet_hdr,
+.has_vnet_hdr_len = netmap_has_vnet_hdr_len,
+.using_vnet_hdr = netmap_using_vnet_hdr,
+.set_offload = netmap_set_offload,
+.set_vnet_hdr_len = netmap_set_vnet_hdr_len,
};
/* The exported init function
@@ -428,6 +489,7 @@ int net_init_netmap(const NetClientOptions *opts,
nc = qemu_new_net_client(net_netmap_info, peer, netmap, name);
s = DO_UPCAST(NetmapState, nc, nc);
s-me = me;
+s-vnet_hdr_len = 0;
netmap_read_poll(s, true); /* Initially only poll for reads. */
return 0;
--
1.8.5.1
