From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rusty Russell Subject: Re: [PATCHv2] virtio: use smp_XX barriers on SMP Date: Fri, 29 Jan 2010 07:44:59 +1030 Message-ID: <201001290744.59324.rusty@rustcorp.com.au> References: <20100127224222.GA31545@redhat.com> <201001281001.09999.rusty@rustcorp.com.au> <20100128133708.GA3776@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20100128133708.GA3776@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: "Michael S. Tsirkin" Cc: virtualization@lists.linux-foundation.org List-Id: virtualization@lists.linuxfoundation.org On Fri, 29 Jan 2010 12:07:08 am Michael S. Tsirkin wrote: > On Thu, Jan 28, 2010 at 10:01:09AM +1030, Rusty Russell wrote: > > On Thu, 28 Jan 2010 09:12:23 am Michael S. Tsirkin wrote: > > > Where possible, we should use SMP barriers which are more lightweight than > > > mandatory barriers, because mandatory barriers also control MMIO effects on > > > accesses through relaxed memory I/O windows (which virtio does not use) > > > (compare specifically smp_rmb and rmb on x86_64). > > > > Xen had a similar issue, in that UP guests running on SMP hosts need to issue > > SMP barriers. Is this not also a requirement for virtio? > > Of course it is. That's why I have ifdef CONFIG_SMP and use > mandatory barriers on UP. Sorry, this was an off-the-cuff comment. I was concerned that UP barriers might be insufficient on some archs. But I can't find any such archs when I actually look (at least, x86, s390 and powerpc). I've applied your patch, minus the first two substitutions: @@ -36,10 +54,10 @@ panic("%s:in_use = %i\n", \ (_vq)->vq.name, (_vq)->in_use); \ (_vq)->in_use = __LINE__; \ - mb(); \ + virtio_mb(); \ } while (0) #define END_USE(_vq) \ - do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; mb(); } while(0) + do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; virtio_mb(); } while(0) #else #define BAD_RING(_vq, fmt, args...) \ do { \ These barriers are actually trying to make sure in_use is set in a timely manner (this is debug code). They're bogus AFAICT: if you don't have any other synchronization then you're in danger of nesting and you want to know. And barriers might change timing too much when DEBUG is defined. Thanks! Rusty.