[RFC] um: have real DMA barriers with virtio

[RFC] um: have real DMA barriers with virtio

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When we have virtio enabled, we must have real barriers since we
may be running on an SMP machine (quite likely are, in fact), so
the other process can be on another CPU.

Make the override to compiler barriers depend on !virtio.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/x86/um/asm/barrier.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/um/asm/barrier.h b/arch/x86/um/asm/barrier.h
index eb0654f39fd2..fd90b3c7cba8 100644
--- a/arch/x86/um/asm/barrier.h
+++ b/arch/x86/um/asm/barrier.h
@@ -23,8 +23,16 @@
 
 #endif /* CONFIG_X86_32 */
 
+#if !IS_ENABLED(CONFIG_VIRTIO_UML)
+/*
+ * We can afford to just have compiler barriers here - unless
+ * virtio is enabled, because then we communicate with another
+ * process and (despite being UP internally) cannot assume we
+ * run on a UP system.
+ */
 #define dma_rmb()	barrier()
 #define dma_wmb()	barrier()
+#endif
 
 #include <asm-generic/barrier.h>
 
-- 
2.20.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Johannes Berg]

On Sat, 2019-09-07 at 00:02 +0200, Johannes Berg wrote:
> 
> Make the override to compiler barriers depend on !virtio.

 
> +#if !IS_ENABLED(CONFIG_VIRTIO_UML)
> +/*
> + * We can afford to just have compiler barriers here - unless
> + * virtio is enabled, because then we communicate with another
> + * process and (despite being UP internally) cannot assume we
> + * run on a UP system.
> + */
>  #define dma_rmb()	barrier()
>  #define dma_wmb()	barrier()
> +#endif

I suppose the [RFC] question is: should we even keep this at all? It
seems to me that we'd only encounter dma_rmb()/dma_wmb() in a driver
that's for some kind of DMA device, which is only possible with virtio,
so there's no gain in overriding them to just barrier() since the non-
driver part of the kernel will never use them?

IOW - is the ifdef worth it, vs. just removing it completely? I suspect
not.

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Johannes Berg]

On Sat, 2019-09-07 at 21:32 +0200, Johannes Berg wrote:
> On Sat, 2019-09-07 at 00:02 +0200, Johannes Berg wrote:
> > Make the override to compiler barriers depend on !virtio.
> 
>  
> > +#if !IS_ENABLED(CONFIG_VIRTIO_UML)
> > +/*
> > + * We can afford to just have compiler barriers here - unless
> > + * virtio is enabled, because then we communicate with another
> > + * process and (despite being UP internally) cannot assume we
> > + * run on a UP system.
> > + */
> >  #define dma_rmb()	barrier()
> >  #define dma_wmb()	barrier()
> > +#endif
> 
> I suppose the [RFC] question is: should we even keep this at all? It
> seems to me that we'd only encounter dma_rmb()/dma_wmb() in a driver
> that's for some kind of DMA device, which is only possible with virtio,
> so there's no gain in overriding them to just barrier() since the non-
> driver part of the kernel will never use them?
> 
> IOW - is the ifdef worth it, vs. just removing it completely? I suspect
> not.

Hmm, the other option could be to just define them to virt_rmb() and
virt_wmb() which are just __smp_rmb() and __smp_wmb() respectively,
which are theoretically a bit weaker than dma_rmb()/dma_wmb() on real
platforms I think, but since we don't implement it any different the net
effect will be the same - might just result in a bit easier to
understand code in the future if we say here

/* we only do DMA to virt devices */
#define dma_rmb() virt_rmb()
#define dma_wmb() virt_wmb()

instead of leaving it out entirely, in which case it'll make the generic
code fall back to just rmb()/wmb().

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Anton Ivanov]

On 07/09/2019 20:32, Johannes Berg wrote:
> On Sat, 2019-09-07 at 00:02 +0200, Johannes Berg wrote:
>>
>> Make the override to compiler barriers depend on !virtio.
> 
>   
>> +#if !IS_ENABLED(CONFIG_VIRTIO_UML)
>> +/*
>> + * We can afford to just have compiler barriers here - unless
>> + * virtio is enabled, because then we communicate with another
>> + * process and (despite being UP internally) cannot assume we
>> + * run on a UP system.
>> + */
>>   #define dma_rmb()	barrier()
>>   #define dma_wmb()	barrier()
>> +#endif
> 
> I suppose the [RFC] question is: should we even keep this at all? It
> seems to me that we'd only encounter dma_rmb()/dma_wmb() in a driver
> that's for some kind of DMA device, which is only possible with virtio,
> so there's no gain in overriding them to just barrier() since the non-
> driver part of the kernel will never use them?

Disk IO at present is for all practical purposes DMA and is abusing 
artefacts from the IPC to work.

The IO itself happens in another thread which may be on a different CPU. 
It does not use any barriers or anything to ensure that the buffered 
data is synchronized and relies on the fact that the IPC which uses a 
socketpair somewhere in the guts of the kernel will invoke a barrier 
while pushing the data along.

I have looked a couple of times at expressing this and other helper 
threads as proper DMA, but never got around to do it.

If it is done for virtio this will make life easier elsewhere to finish 
the other bits.

> 
> IOW - is the ifdef worth it, vs. just removing it completely? I suspect
> not.
> 
> johannes

While at it - the WARN_ in the virtio driver should really become more 
informative so it is clear who and what caused them.


> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 


-- 
Anton R. Ivanov
https://www.kot-begemot.co.uk/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Johannes Berg]

On Sun, 2019-09-08 at 21:23 +0100, Anton Ivanov wrote:
> On 07/09/2019 20:32, Johannes Berg wrote:
> > On Sat, 2019-09-07 at 00:02 +0200, Johannes Berg wrote:
> > > Make the override to compiler barriers depend on !virtio.
> > 
> >   
> > > +#if !IS_ENABLED(CONFIG_VIRTIO_UML)
> > > +/*
> > > + * We can afford to just have compiler barriers here - unless
> > > + * virtio is enabled, because then we communicate with another
> > > + * process and (despite being UP internally) cannot assume we
> > > + * run on a UP system.
> > > + */
> > >   #define dma_rmb()	barrier()
> > >   #define dma_wmb()	barrier()
> > > +#endif
> > 
> > I suppose the [RFC] question is: should we even keep this at all? It
> > seems to me that we'd only encounter dma_rmb()/dma_wmb() in a driver
> > that's for some kind of DMA device, which is only possible with virtio,
> > so there's no gain in overriding them to just barrier() since the non-
> > driver part of the kernel will never use them?
> 
> Disk IO at present is for all practical purposes DMA and is abusing 
> artefacts from the IPC to work.

Hmm, good point.

> The IO itself happens in another thread which may be on a different CPU. 
> It does not use any barriers or anything to ensure that the buffered 
> data is synchronized and relies on the fact that the IPC which uses a 
> socketpair somewhere in the guts of the kernel will invoke a barrier 
> while pushing the data along.

Makes sense. However, the UBD code still doesn't contain any DMA
barriers, so what I said above still holds true I think - just removing
the dma_rmb/dma_wmb definitions shouldn't really affect much if
anything.

Not sure I understand you correctly, but I guess you're saying we should
remove the definitions in all cases, so we can use the DMA barriers
properly even in the non-virtio case?

> While at it - the WARN_ in the virtio driver should really become more 
> informative so it is clear who and what caused them.

I replaced most of them with just error logging, since the call stacks
weren't really useful anyway. There are now two left, but those are
internal (to UML) calculation issues, so not something that'd trigger in
a device-dependent way.

That said, I did find a few other issues still, so I'll be reposting.
I'll also try to open /proc/self/mem as the file descriptor to pass to
the virtio device instead of using physmem_fd, because our .bss isn't
actually part of physmem_fd, and that cost me a few hours debugging
already ;-)

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Anton Ivanov]

On 09/09/2019 07:49, Johannes Berg wrote:
> On Sun, 2019-09-08 at 21:23 +0100, Anton Ivanov wrote:
>> On 07/09/2019 20:32, Johannes Berg wrote:
>>> On Sat, 2019-09-07 at 00:02 +0200, Johannes Berg wrote:
>>>> Make the override to compiler barriers depend on !virtio.
>>>
>>>    
>>>> +#if !IS_ENABLED(CONFIG_VIRTIO_UML)
>>>> +/*
>>>> + * We can afford to just have compiler barriers here - unless
>>>> + * virtio is enabled, because then we communicate with another
>>>> + * process and (despite being UP internally) cannot assume we
>>>> + * run on a UP system.
>>>> + */
>>>>    #define dma_rmb()	barrier()
>>>>    #define dma_wmb()	barrier()
>>>> +#endif
>>>
>>> I suppose the [RFC] question is: should we even keep this at all? It
>>> seems to me that we'd only encounter dma_rmb()/dma_wmb() in a driver
>>> that's for some kind of DMA device, which is only possible with virtio,
>>> so there's no gain in overriding them to just barrier() since the non-
>>> driver part of the kernel will never use them?
>>
>> Disk IO at present is for all practical purposes DMA and is abusing
>> artefacts from the IPC to work.
> 
> Hmm, good point.
> 
>> The IO itself happens in another thread which may be on a different CPU.
>> It does not use any barriers or anything to ensure that the buffered
>> data is synchronized and relies on the fact that the IPC which uses a
>> socketpair somewhere in the guts of the kernel will invoke a barrier
>> while pushing the data along.
> 
> Makes sense. However, the UBD code still doesn't contain any DMA
> barriers, so what I said above still holds true I think - just removing
> the dma_rmb/dma_wmb definitions shouldn't really affect much if
> anything.
> 
> Not sure I understand you correctly, but I guess you're saying we should
> remove the definitions in all cases, so we can use the DMA barriers
> properly even in the non-virtio case?

Apologies,

I would like at some point to convert the "helper thread" semantics of 
ubd, etc to look like DMA and use DMA barrier as well as have the 
relevant DMA infrastructure.

At present they work only because the IPC forces the host kernel to 
invoke a barrier. If I remember correctly, the actual place where the 
barrier is invoked is the IPC socket enqueue/dequeue in the host kernel.

If, for some reason the host kernel does not invoke the barrier (f.e., 
if someone optimizes the queue operations so there is no need) they will 
break.

IMHO adding DMA emulation will have benefits for both existing and 
future drivers.

> 
>> While at it - the WARN_ in the virtio driver should really become more
>> informative so it is clear who and what caused them.
> 
> I replaced most of them with just error logging, since the call stacks
> weren't really useful anyway. There are now two left, but those are
> internal (to UML) calculation issues, so not something that'd trigger in
> a device-dependent way.

Cool.

> 
> That said, I did find a few other issues still, so I'll be reposting.
> I'll also try to open /proc/self/mem as the file descriptor to pass to
> the virtio device instead of using physmem_fd, because our .bss isn't
> actually part of physmem_fd, and that cost me a few hours debugging
> already ;-)

I will retest vs BESS/DPDK when it is ready.

> 
> johannes
> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
> 

-- 
Anton R. Ivanov
https://www.kot-begemot.co.uk/

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [RFC] um: have real DMA barriers with virtio

[Johannes Berg]

On Mon, 2019-09-09 at 08:27 +0100, Anton Ivanov wrote:

> I would like at some point to convert the "helper thread" semantics of 
> ubd, etc to look like DMA and use DMA barrier as well as have the 
> relevant DMA infrastructure.

Right, I got that part, makes sense.

> At present they work only because the IPC forces the host kernel to 
> invoke a barrier. If I remember correctly, the actual place where the 
> barrier is invoked is the IPC socket enqueue/dequeue in the host kernel.
> 
> If, for some reason the host kernel does not invoke the barrier (f.e., 
> if someone optimizes the queue operations so there is no need) they will 
> break.

Sure, also makes sense.

> IMHO adding DMA emulation will have benefits for both existing and 
> future drivers.

Well, in a sense virtio is emulating DMA. We could even just add another
virtio transport (not the vhost-user backed one we have now) and just
rely on virtio-blk, and implement just the device side like qemu already
would. I suppose it might be easy to even copy that.

But anyway - sorry for being dense, maybe it's too early morning :-) But
I still don't understand what that means for this patch...

> > That said, I did find a few other issues still, so I'll be reposting.
> > I'll also try to open /proc/self/mem as the file descriptor to pass to
> > the virtio device instead of using physmem_fd, because our .bss isn't
> > actually part of physmem_fd, and that cost me a few hours debugging
> > already ;-)
> 
> I will retest vs BESS/DPDK when it is ready.

Thanks. I'll repost in the next couple of days, want to experiment with
the /proc/self/mem first, and if that works better use it.

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um