| Message ID | 1392396024-32420-1-git-send-email-v.maffione@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote: > This patch fixes configure so that netmap is not compiled in if the > host doesn't support an API version >= 11. > > Moreover, some modifications have been done to net/netmap.c in > order to reflect the current netmap API (11). > > Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com> > --- > configure | 3 +++ > net/netmap.c | 57 ++++++++++++++------------------------------------------- > 2 files changed, 17 insertions(+), 43 deletions(-) > > diff --git a/configure b/configure > index 88133a1..61eb932 100755 > --- a/configure > +++ b/configure > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then > #include <net/if.h> > #include <net/netmap.h> > #include <net/netmap_user.h> > +#if (NETMAP_API < 11) || (NETMAP_API > 15) > +#error > +#endif Why error when NETMAP_API > 15? > @@ -56,31 +58,6 @@ typedef struct NetmapState { > struct iovec iov[IOV_MAX]; > } NetmapState; > > -#define D(format, ...) \ > - do { \ > - struct timeval __xxts; \ > - gettimeofday(&__xxts, NULL); \ > - printf("%03d.%06d %s [%d] " format "\n", \ > - (int)__xxts.tv_sec % 1000, (int)__xxts.tv_usec, \ > - __func__, __LINE__, ##__VA_ARGS__); \ > - } while (0) > - > -/* Rate limited version of "D", lps indicates how many per second */ > -#define RD(lps, format, ...) \ > - do { \ > - static int t0, __cnt; \ > - struct timeval __xxts; \ > - gettimeofday(&__xxts, NULL); \ > - if (t0 != __xxts.tv_sec) { \ > - t0 = __xxts.tv_sec; \ > - __cnt = 0; \ > - } \ > - if (__cnt++ < lps) { \ > - D(format, ##__VA_ARGS__); \ > - } \ > - } while (0) > - > - > #ifndef __FreeBSD__ > #define pkt_copy bcopy > #else Why are you deleting this? > @@ -237,7 +214,7 @@ static ssize_t netmap_receive(NetClientState *nc, > return size; > } > > - if (ring->avail == 0) { > + if (nm_ring_empty(ring)) { > /* No available slots in the netmap TX ring. */ > netmap_write_poll(s, true); > return 0; > @@ -250,8 +227,7 @@ static ssize_t netmap_receive(NetClientState *nc, > ring->slot[i].len = size; > ring->slot[i].flags = 0; > pkt_copy(buf, dst, size); > - ring->cur = NETMAP_RING_NEXT(ring, i); > - ring->avail--; > + ring->cur = ring->head = nm_ring_next(ring, i); > ioctl(s->me.fd, NIOCTXSYNC, NULL); > > return size; Are these changes related to the NETMAP_WITH_LIBS macro? Please do that in a separate patch so we keep the version checking change separate from the NETMAP_WITH_LIBS change.
2014-02-19 16:30 GMT+01:00 Stefan Hajnoczi <stefanha@gmail.com>: > On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote: > > This patch fixes configure so that netmap is not compiled in if the > > host doesn't support an API version >= 11. > > > > Moreover, some modifications have been done to net/netmap.c in > > order to reflect the current netmap API (11). > > > > Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com> > > --- > > configure | 3 +++ > > net/netmap.c | 57 > ++++++++++++++------------------------------------------- > > 2 files changed, 17 insertions(+), 43 deletions(-) > > > > diff --git a/configure b/configure > > index 88133a1..61eb932 100755 > > --- a/configure > > +++ b/configure > > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then > > #include <net/if.h> > > #include <net/netmap.h> > > #include <net/netmap_user.h> > > +#if (NETMAP_API < 11) || (NETMAP_API > 15) > > +#error > > +#endif > > Why error when NETMAP_API > 15? > Well, I believe Luigi can answer this question better than me, so I would wait for his answer. > > > @@ -56,31 +58,6 @@ typedef struct NetmapState { > > struct iovec iov[IOV_MAX]; > > } NetmapState; > > > > -#define D(format, ...) \ > > - do { \ > > - struct timeval __xxts; \ > > - gettimeofday(&__xxts, NULL); \ > > - printf("%03d.%06d %s [%d] " format "\n", \ > > - (int)__xxts.tv_sec % 1000, (int)__xxts.tv_usec, \ > > - __func__, __LINE__, ##__VA_ARGS__); \ > > - } while (0) > > - > > -/* Rate limited version of "D", lps indicates how many per second */ > > -#define RD(lps, format, ...) \ > > - do { \ > > - static int t0, __cnt; \ > > - struct timeval __xxts; \ > > - gettimeofday(&__xxts, NULL); \ > > - if (t0 != __xxts.tv_sec) { \ > > - t0 = __xxts.tv_sec; \ > > - __cnt = 0; \ > > - } \ > > - if (__cnt++ < lps) { \ > > - D(format, ##__VA_ARGS__); \ > > - } \ > > - } while (0) > > - > > - > > #ifndef __FreeBSD__ > > #define pkt_copy bcopy > > #else > > Why are you deleting this? > Because now equivalent code is contained by "netmap_user.h" > > > @@ -237,7 +214,7 @@ static ssize_t netmap_receive(NetClientState *nc, > > return size; > > } > > > > - if (ring->avail == 0) { > > + if (nm_ring_empty(ring)) { > > /* No available slots in the netmap TX ring. */ > > netmap_write_poll(s, true); > > return 0; > > @@ -250,8 +227,7 @@ static ssize_t netmap_receive(NetClientState *nc, > > ring->slot[i].len = size; > > ring->slot[i].flags = 0; > > pkt_copy(buf, dst, size); > > - ring->cur = NETMAP_RING_NEXT(ring, i); > > - ring->avail--; > > + ring->cur = ring->head = nm_ring_next(ring, i); > > ioctl(s->me.fd, NIOCTXSYNC, NULL); > > > > return size; > > Are these changes related to the NETMAP_WITH_LIBS macro? Please do that > in a separate patch so we keep the version checking change separate from > the NETMAP_WITH_LIBS change. > Yes it is. The macro drives an #ifdef in netmap_user.h, so that we can use some new functions/macros. Ok for the splitting. Is it necessary to prepare a patch series or can I send two different patches? Thanks, Vincenzo
On Wed, Feb 19, 2014 at 7:30 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote: > > This patch fixes configure so that netmap is not compiled in if the > > host doesn't support an API version >= 11. > > > > Moreover, some modifications have been done to net/netmap.c in > > order to reflect the current netmap API (11). > > > > Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com> > > --- > > configure | 3 +++ > > net/netmap.c | 57 > ++++++++++++++------------------------------------------- > > 2 files changed, 17 insertions(+), 43 deletions(-) > > > > diff --git a/configure b/configure > > index 88133a1..61eb932 100755 > > --- a/configure > > +++ b/configure > > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then > > #include <net/if.h> > > #include <net/netmap.h> > > #include <net/netmap_user.h> > > +#if (NETMAP_API < 11) || (NETMAP_API > 15) > > +#error > > +#endif > > Why error when NETMAP_API > 15? > this is meant to simulate a minor/major version number. We will mark minor new features with values up to 15, and if something happens that requires a change in the backend we will move above 15, at which point we will submit backend fixes and also the necessary update to ./configure > > - ring->cur = NETMAP_RING_NEXT(ring, i); > > - ring->avail--; > > + ring->cur = ring->head = nm_ring_next(ring, i); > > ioctl(s->me.fd, NIOCTXSYNC, NULL); > > > > return size; > > Are these changes related to the NETMAP_WITH_LIBS macro? Please do that > in a separate patch so we keep the version checking change separate from > the NETMAP_WITH_LIBS change. > netmap version 11 and above has NETMAP_WITH_LIBS, while previous versions do not, so this ./configure change has to go together with the change in the backend. The netmap 11 code has already been committed to the FreeBSD source repositories (for HEAD, 10 and 9) and to code.google.com/p/netmap/ (for those who want it on linux). So there is really no point, in my opinion, to make one intermediate commit just to ./configure to disable netmap detection on FreeBSD (it is already off on linux), immediately followed by this one that uses the new feature. Just to clarify: with one exception (fields in struct netmap_ring) the netmap changes that we have are not at the kernel/user boundary but in a library which avoids replicating long and boring code (interface name parsing, parameter setting) in applications. Avoiding the single incompatible struct change would have been of course possible, but at the cost extra complexity in the kernel and in userspace (to support two slightly different interfaces). Since netmap is (so far) relatively little used I thought it was more important to fix the API and keep it simple. cheers luigi -----------------------------------------+------------------------------- Prof. Luigi RIZZO, rizzo@iet.unipi.it . Dip. di Ing. dell'Informazione http://www.iet.unipi.it/~luigi/ . Universita` di Pisa TEL +39-050-2211611 . via Diotisalvi 2 Mobile +39-338-6809875 . 56122 PISA (Italy) -----------------------------------------+-------------------------------
On Wed, Feb 19, 2014 at 10:30:03AM -0800, Luigi Rizzo wrote: > On Wed, Feb 19, 2014 at 7:30 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote: > > > diff --git a/configure b/configure > > > index 88133a1..61eb932 100755 > > > --- a/configure > > > +++ b/configure > > > @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then > > > #include <net/if.h> > > > #include <net/netmap.h> > > > #include <net/netmap_user.h> > > > +#if (NETMAP_API < 11) || (NETMAP_API > 15) > > > +#error > > > +#endif > > > > Why error when NETMAP_API > 15? > > > > this is meant to simulate a minor/major version number. > We will mark minor new features with values up to 15, > and if something happens that requires a change in the > backend we will move above 15, at which point we > will submit backend fixes and also the necessary > update to ./configure I see. A comment in the code would be nice to explain that. > > > - ring->cur = NETMAP_RING_NEXT(ring, i); > > > - ring->avail--; > > > + ring->cur = ring->head = nm_ring_next(ring, i); > > > ioctl(s->me.fd, NIOCTXSYNC, NULL); > > > > > > return size; > > > > Are these changes related to the NETMAP_WITH_LIBS macro? Please do that > > in a separate patch so we keep the version checking change separate from > > the NETMAP_WITH_LIBS change. > > > > netmap version 11 and above has NETMAP_WITH_LIBS, > while previous versions do not, so this ./configure > change has to go together with the change in the backend. > > The netmap 11 code has already been committed to the FreeBSD > source repositories (for HEAD, 10 and 9) and to > code.google.com/p/netmap/ (for those who want it on linux). > > So there is really no point, in my opinion, to make one > intermediate commit just to ./configure to disable > netmap detection on FreeBSD (it is already off on linux), > immediately followed by this one that uses the new feature. > > Just to clarify: with one exception (fields in struct netmap_ring) > the netmap changes that we have are not at the kernel/user boundary > but in a library which avoids replicating long and boring code > (interface name parsing, parameter setting) in applications. > > Avoiding the single incompatible struct change would have > been of course possible, but at the cost > extra complexity in the kernel and in userspace > (to support two slightly different interfaces). > Since netmap is (so far) relatively little used I thought it > was more important to fix the API and keep it simple. Thanks for explaining. Please put justification for the NETMAP_WITH_LIBS changes in the commit description. Stefan
On Wed, Feb 19, 2014 at 04:57:28PM +0100, Vincenzo Maffione wrote: > 2014-02-19 16:30 GMT+01:00 Stefan Hajnoczi <stefanha@gmail.com>: > > > On Fri, Feb 14, 2014 at 05:40:24PM +0100, Vincenzo Maffione wrote: > > > @@ -56,31 +58,6 @@ typedef struct NetmapState { > > > struct iovec iov[IOV_MAX]; > > > } NetmapState; > > > > > > -#define D(format, ...) \ > > > - do { \ > > > - struct timeval __xxts; \ > > > - gettimeofday(&__xxts, NULL); \ > > > - printf("%03d.%06d %s [%d] " format "\n", \ > > > - (int)__xxts.tv_sec % 1000, (int)__xxts.tv_usec, \ > > > - __func__, __LINE__, ##__VA_ARGS__); \ > > > - } while (0) > > > - > > > -/* Rate limited version of "D", lps indicates how many per second */ > > > -#define RD(lps, format, ...) \ > > > - do { \ > > > - static int t0, __cnt; \ > > > - struct timeval __xxts; \ > > > - gettimeofday(&__xxts, NULL); \ > > > - if (t0 != __xxts.tv_sec) { \ > > > - t0 = __xxts.tv_sec; \ > > > - __cnt = 0; \ > > > - } \ > > > - if (__cnt++ < lps) { \ > > > - D(format, ##__VA_ARGS__); \ > > > - } \ > > > - } while (0) > > > - > > > - > > > #ifndef __FreeBSD__ > > > #define pkt_copy bcopy > > > #else > > > > Why are you deleting this? > > > Because now equivalent code is contained by "netmap_user.h" Please mention that in the commit description. (I guess it's too late to give them a NETMAP_* prefix since defining D() and RD() in a system header has a fair chance of causing namespace conflicts.) Stefan
On Thu, Feb 20, 2014 at 10:49:52AM +0100, Stefan Hajnoczi wrote: > On Wed, Feb 19, 2014 at 04:57:28PM +0100, Vincenzo Maffione wrote: ... > Please mention that in the commit description. > > (I guess it's too late to give them a NETMAP_* prefix since defining D() > and RD() in a system header has a fair chance of causing namespace > conflicts.) you are right that the naming is unfortunate and we will try to address that at a later time, removing the macros. As partial mitigation of the problem, they are intended only for debugging (so they should only be used by lazy programmers; applications should prefer application-specific logging functions), and D(), RD() and ND() are only defined when 'NETMAP_WITH_LIBS' is defined and this should happen only in the single file that implements the netmap backend. cheers luigi
diff --git a/configure b/configure index 88133a1..61eb932 100755 --- a/configure +++ b/configure @@ -2118,6 +2118,9 @@ if test "$netmap" != "no" ; then #include <net/if.h> #include <net/netmap.h> #include <net/netmap_user.h> +#if (NETMAP_API < 11) || (NETMAP_API > 15) +#error +#endif int main(void) { return 0; } EOF if compile_prog "" "" ; then diff --git a/net/netmap.c b/net/netmap.c index 0ccc497..8a67322 100644 --- a/net/netmap.c +++ b/net/netmap.c @@ -27,6 +27,8 @@ #include <net/if.h> #include <sys/mman.h> #include <stdint.h> +#include <stdio.h> +#define NETMAP_WITH_LIBS #include <net/netmap.h> #include <net/netmap_user.h> @@ -56,31 +58,6 @@ typedef struct NetmapState { struct iovec iov[IOV_MAX]; } NetmapState; -#define D(format, ...) \ - do { \ - struct timeval __xxts; \ - gettimeofday(&__xxts, NULL); \ - printf("%03d.%06d %s [%d] " format "\n", \ - (int)__xxts.tv_sec % 1000, (int)__xxts.tv_usec, \ - __func__, __LINE__, ##__VA_ARGS__); \ - } while (0) - -/* Rate limited version of "D", lps indicates how many per second */ -#define RD(lps, format, ...) \ - do { \ - static int t0, __cnt; \ - struct timeval __xxts; \ - gettimeofday(&__xxts, NULL); \ - if (t0 != __xxts.tv_sec) { \ - t0 = __xxts.tv_sec; \ - __cnt = 0; \ - } \ - if (__cnt++ < lps) { \ - D(format, ##__VA_ARGS__); \ - } \ - } while (0) - - #ifndef __FreeBSD__ #define pkt_copy bcopy #else @@ -237,7 +214,7 @@ static ssize_t netmap_receive(NetClientState *nc, return size; } - if (ring->avail == 0) { + if (nm_ring_empty(ring)) { /* No available slots in the netmap TX ring. */ netmap_write_poll(s, true); return 0; @@ -250,8 +227,7 @@ static ssize_t netmap_receive(NetClientState *nc, ring->slot[i].len = size; ring->slot[i].flags = 0; pkt_copy(buf, dst, size); - ring->cur = NETMAP_RING_NEXT(ring, i); - ring->avail--; + ring->cur = ring->head = nm_ring_next(ring, i); ioctl(s->me.fd, NIOCTXSYNC, NULL); return size; @@ -267,17 +243,15 @@ static ssize_t netmap_receive_iov(NetClientState *nc, uint8_t *dst; int j; uint32_t i; - uint32_t avail; if (unlikely(!ring)) { /* Drop the packet. */ return iov_size(iov, iovcnt); } - i = ring->cur; - avail = ring->avail; + last = i = ring->cur; - if (avail < iovcnt) { + if (nm_ring_space(ring) < iovcnt) { /* Not enough netmap slots. */ netmap_write_poll(s, true); return 0; @@ -293,7 +267,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc, while (iov_frag_size) { nm_frag_size = MIN(iov_frag_size, ring->nr_buf_size); - if (unlikely(avail == 0)) { + if (unlikely(nm_ring_empty(ring))) { /* We run out of netmap slots while splitting the iovec fragments. */ netmap_write_poll(s, true); @@ -308,8 +282,7 @@ static ssize_t netmap_receive_iov(NetClientState *nc, pkt_copy(iov[j].iov_base + offset, dst, nm_frag_size); last = i; - i = NETMAP_RING_NEXT(ring, i); - avail--; + i = nm_ring_next(ring, i); offset += nm_frag_size; iov_frag_size -= nm_frag_size; @@ -318,9 +291,8 @@ static ssize_t netmap_receive_iov(NetClientState *nc, /* The last slot must not have NS_MOREFRAG set. */ ring->slot[last].flags &= ~NS_MOREFRAG; - /* Now update ring->cur and ring->avail. */ - ring->cur = i; - ring->avail = avail; + /* Now update ring->cur and ring->head. */ + ring->cur = ring->head = i; ioctl(s->me.fd, NIOCTXSYNC, NULL); @@ -343,7 +315,7 @@ static void netmap_send(void *opaque) /* Keep sending while there are available packets into the netmap RX ring and the forwarding path towards the peer is open. */ - while (ring->avail > 0 && qemu_can_send_packet(&s->nc)) { + while (!nm_ring_empty(ring) && qemu_can_send_packet(&s->nc)) { uint32_t i; uint32_t idx; bool morefrag; @@ -358,11 +330,10 @@ static void netmap_send(void *opaque) s->iov[iovcnt].iov_len = ring->slot[i].len; iovcnt++; - ring->cur = NETMAP_RING_NEXT(ring, i); - ring->avail--; - } while (ring->avail && morefrag); + ring->cur = ring->head = nm_ring_next(ring, i); + } while (!nm_ring_empty(ring) && morefrag); - if (unlikely(!ring->avail && morefrag)) { + if (unlikely(nm_ring_empty(ring) && morefrag)) { RD(5, "[netmap_send] ran out of slots, with a pending" "incomplete packet\n"); }
This patch fixes configure so that netmap is not compiled in if the host doesn't support an API version >= 11. Moreover, some modifications have been done to net/netmap.c in order to reflect the current netmap API (11). Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com> --- configure | 3 +++ net/netmap.c | 57 ++++++++++++++------------------------------------------- 2 files changed, 17 insertions(+), 43 deletions(-)