Message ID | 1567807353-Ieca0b50a173f8a587d412db861c7ae4d6bae97c4@changeid |
---|---|
State | RFC |
Headers | show |
Series | [RFC] um: have real DMA barriers with virtio | expand |
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
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
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 >
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
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 >
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
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>