Re: [Patch] Virtio drivers for OpenBSD V5
On Monday 13 August 2012 17:00:56 Mike Belopuhov wrote: you're calling MCLGETI and provide an interface pointer but you forget to set watermarks via m_clsetwms. OK. The MCLGETI man page should probably get a pointer to m_clsetwms(). -- genua Gesellschaft fuer Netzwerk- und Unix-Administration mbH Domagkstrasse 7, 85551 Kirchheim bei Muenchen tel +49 89 991950-0, fax -999, www.genua.de Geschaeftsfuehrer: Dr. Magnus Harlander, Dr. Michaela Harlander, Bernhard Schneck. Amtsgericht Muenchen HRB 98238
Re: [Patch] Virtio drivers for OpenBSD V5
Hi, - Original Message - From: Mark Kettenis mark.kette...@xs4all.nl To: Stefan Fritsch stefan_frit...@genua.de Cc: tech@openbsd.org Sent: Monday, August 13, 2012 2:20:50 PM Subject: Re: [Patch] Virtio drivers for OpenBSD V5 From: Stefan Fritsch stefan_frit...@genua.de Date: Mon, 13 Aug 2012 16:24:15 +0200 Hi, here is the next iteration of my patch. Changes from V4 include: - virtio: support RING_EVENT_IDX - virtio: use lfence/sfence because __sync_synchronize() is broken on gcc 4.4 Broken how? +/* + * The mutexes are not necessary with OpenBSD's big kernel lock. + */ +#define mutex_enter(x) +#define mutex_destroy(x) +#define mutex_exit(x) +#define mutex_init(x,y,z) Then remove them completely. We don't need more simple_lock()-like pollution in the tree. +/* + * XXX: This is not optimal: + * XXX: We need remory barriers are also with UP kernels, What are you trying to say here? I think what he's getting it is that even in the UP kernel, memory ordering for some operations must be enforced. + * XXX: because the host system may be SMP. However, OpenBSD does not seem to + * XXX: provide suitable functions. + * XXX: On x86/amd64, membar_producer() could probably be a no-op because + * XXX: writes are always ordered. + * XXX: Also, gcc does not have __sync_synchronize() for all architectures. + * XXX: Also, for gcc 4.4, __sync_synchronize() is broken + */ + +#if defined(__i386__) || defined(__amd64__) +#include machine/cpufunc.h +#define membar_consumer() lfence() +#define membar_producer() sfence() +#else +#define membar_consumer() __sync_synchronize() +#define membar_producer() __sync_synchronize() +#endif Introducing these macros here would be a mistake. If we really need them, they should be added to appropriate MD header files and implemented for all supported architectures. That said, it is not obvious to me what problem is being solved by adding these memory barriers to the code. The use of barriers suggests there is some lock-less algorithm in the code, but I failed to spot it. The other side of the lockless queue is happening from the host, i.e. in Qemu. The virtqueue (aka VirtIO ring) is in shared memory between guest and host. A virtqueue descriptor (denoting, say, an ethernet frame for transmit) must be fully set up before the corresponding counter is incremented to inform the host of the newly available frame.
Re: [Patch] Virtio drivers for OpenBSD V5
On Mon, Aug 13, 2012 at 4:24 PM, Stefan Fritsch stefan_frit...@genua.de wrote: - virtio: use lfence/sfence because __sync_synchronize() is broken on gcc 4.4 please don't. use bus_space_barrier.
Re: [Patch] Virtio drivers for OpenBSD V5
On Mon, Aug 13, 2012 at 4:24 PM, Stefan Fritsch stefan_frit...@genua.de wrote: Hi, here is the next iteration of my patch. Changes from V4 include: - virtio: support RING_EVENT_IDX - virtio: use lfence/sfence because __sync_synchronize() is broken on gcc 4.4 - net: rework memory handling in vioif - net: altq support (not really tested) - net: support RING_EVENT_IDX to reduce interrupt load - net: in tx intr handler, call start again if tx queue is not empty - block: fix error handling / spl not lowered - include in amd64 GENERIC config you're calling MCLGETI and provide an interface pointer but you forget to set watermarks via m_clsetwms.
Re: [Patch] Virtio drivers for OpenBSD V5
On Monday 13 August 2012 17:07:41 you wrote: * Note: the i386 does not currently require barriers, but we must * provide the flags to MI code. This is not correct for virtio. We need a memory barrier. sure, copy it from amd64. OK. A slight complication: sfence/mfence/lfence do not exist on all i386 CPUs. sfence was introduced with SSE, [lm]fence with SSE2. This is not really a problem with the virtio driver because the virtualization extensions were introduced much later. But it may be a problem with the rest of the i386 code. How do you normally handle this? Check during boot and set a pointer to the function to be used and call that function pointer from bus_space_barrier()? -- genua Gesellschaft fuer Netzwerk- und Unix-Administration mbH Domagkstrasse 7, 85551 Kirchheim bei Muenchen tel +49 89 991950-0, fax -999, www.genua.de Geschaeftsfuehrer: Dr. Magnus Harlander, Dr. Michaela Harlander, Bernhard Schneck. Amtsgericht Muenchen HRB 98238
Re: [Patch] Virtio drivers for OpenBSD V5
On Mon, Aug 13, 2012 at 5:30 PM, Stefan Fritsch stefan_frit...@genua.de wrote: On Monday 13 August 2012 17:07:41 you wrote: * Note: the i386 does not currently require barriers, but we must * provide the flags to MI code. This is not correct for virtio. We need a memory barrier. sure, copy it from amd64. OK. A slight complication: sfence/mfence/lfence do not exist on all i386 CPUs. sfence was introduced with SSE, [lm]fence with SSE2. This is not really a problem with the virtio driver because the virtualization extensions were introduced much later. But it may be a problem with the rest of the i386 code. How do you normally handle this? Check during boot and set a pointer to the function to be used and call that function pointer from bus_space_barrier()? actually you might try this: http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/bus_space.c#877 same as we do with bus_dmamap_sync on i386/amd64.
Re: [Patch] Virtio drivers for OpenBSD V5
On Mon, Aug 13, 2012 at 5:36 PM, Mike Belopuhov m...@crypt.org.ru wrote: On Mon, Aug 13, 2012 at 5:30 PM, Stefan Fritsch stefan_frit...@genua.de wrote: On Monday 13 August 2012 17:07:41 you wrote: * Note: the i386 does not currently require barriers, but we must * provide the flags to MI code. This is not correct for virtio. We need a memory barrier. sure, copy it from amd64. OK. A slight complication: sfence/mfence/lfence do not exist on all i386 CPUs. sfence was introduced with SSE, [lm]fence with SSE2. This is not really a problem with the virtio driver because the virtualization extensions were introduced much later. But it may be a problem with the rest of the i386 code. How do you normally handle this? Check during boot and set a pointer to the function to be used and call that function pointer from bus_space_barrier()? actually you might try this: http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/bus_space.c#877 same as we do with bus_dmamap_sync on i386/amd64. but otherwise i think you should test the presense of the instruction and set a function pointer somewhere in machdep.
Re: [Patch] Virtio drivers for OpenBSD V5
On Mon, Aug 13, 2012 at 5:41 PM, Mike Belopuhov m...@crypt.org.ru wrote: On Mon, Aug 13, 2012 at 5:36 PM, Mike Belopuhov m...@crypt.org.ru wrote: On Mon, Aug 13, 2012 at 5:30 PM, Stefan Fritsch stefan_frit...@genua.de wrote: On Monday 13 August 2012 17:07:41 you wrote: * Note: the i386 does not currently require barriers, but we must * provide the flags to MI code. This is not correct for virtio. We need a memory barrier. sure, copy it from amd64. OK. A slight complication: sfence/mfence/lfence do not exist on all i386 CPUs. sfence was introduced with SSE, [lm]fence with SSE2. This is not really a problem with the virtio driver because the virtualization extensions were introduced much later. But it may be a problem with the rest of the i386 code. How do you normally handle this? Check during boot and set a pointer to the function to be used and call that function pointer from bus_space_barrier()? actually you might try this: http://nxr.netbsd.org/xref/src/sys/arch/x86/x86/bus_space.c#877 same as we do with bus_dmamap_sync on i386/amd64. but otherwise i think you should test the presense of the instruction and set a function pointer somewhere in machdep. by the way. you might actually want to use bus_dmamap_sync if the memory in question is shared with the host (DMA kind of).
Re: [Patch] Virtio drivers for OpenBSD V5
From: Stefan Fritsch stefan_frit...@genua.de Date: Mon, 13 Aug 2012 16:24:15 +0200 Hi, here is the next iteration of my patch. Changes from V4 include: - virtio: support RING_EVENT_IDX - virtio: use lfence/sfence because __sync_synchronize() is broken on gcc 4.4 Broken how? +/* + * The mutexes are not necessary with OpenBSD's big kernel lock. + */ +#define mutex_enter(x) +#define mutex_destroy(x) +#define mutex_exit(x) +#define mutex_init(x,y,z) Then remove them completely. We don't need more simple_lock()-like pollution in the tree. +/* + * XXX: This is not optimal: + * XXX: We need remory barriers are also with UP kernels, What are you trying to say here? + * XXX: because the host system may be SMP. However, OpenBSD does not seem to + * XXX: provide suitable functions. + * XXX: On x86/amd64, membar_producer() could probably be a no-op because + * XXX: writes are always ordered. + * XXX: Also, gcc does not have __sync_synchronize() for all architectures. + * XXX: Also, for gcc 4.4, __sync_synchronize() is broken + */ + +#if defined(__i386__) || defined(__amd64__) +#include machine/cpufunc.h +#define membar_consumer() lfence() +#define membar_producer() sfence() +#else +#define membar_consumer() __sync_synchronize() +#define membar_producer() __sync_synchronize() +#endif Introducing these macros here would be a mistake. If we really need them, they should be added to appropriate MD header files and implemented for all supported architectures. That said, it is not obvious to me what problem is being solved by adding these memory barriers to the code. The use of barriers suggests there is some lock-less algorithm in the code, but I failed to spot it.
