From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH -v3] KVM: x86: lapic: Clean up find_highest_vector() and count_vectors() Date: Wed, 5 Sep 2012 12:26:49 +0300 Message-ID: <20120905092649.GD10326@redhat.com> References: <20120830063702.GB9286@redhat.com> <20120830185052.086cbb0f.yoshikawa.takuya@oss.ntt.co.jp> <20120830101033.GA19622@redhat.com> <20120830192439.3e088595.yoshikawa.takuya@oss.ntt.co.jp> <20120830104458.GA19997@redhat.com> <20120830213019.82a822da.yoshikawa.takuya@oss.ntt.co.jp> <20120830132131.GB21132@redhat.com> <20120831010956.000856ead3c6f96dbe2bec46@gmail.com> <20120830164923.GA22024@redhat.com> <20120905173031.59581bb9.yoshikawa.takuya@oss.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Takuya Yoshikawa , Marcelo Tosatti , avi@redhat.com, kvm@vger.kernel.org To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:22561 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758375Ab2IEJZa (ORCPT ); Wed, 5 Sep 2012 05:25:30 -0400 Content-Disposition: inline In-Reply-To: <20120905173031.59581bb9.yoshikawa.takuya@oss.ntt.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Sep 05, 2012 at 05:30:31PM +0900, Takuya Yoshikawa wrote: > On Thu, 30 Aug 2012 19:49:23 +0300 > "Michael S. Tsirkin" wrote: > > > On Fri, Aug 31, 2012 at 01:09:56AM +0900, Takuya Yoshikawa wrote: > > > On Thu, 30 Aug 2012 16:21:31 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > > +static u32 apic_read_reg(int reg_off, void *bitmap) > > > > > +{ > > > > > + return *((u32 *)(bitmap + reg_off)); > > > > > +} > > > > > + > > > > > > > > Contrast with apic_set_reg which gets apic, > > > > add fact that all callers invoke REG_POS and you will > > > > see this is a bad API. > > > > > > > > I played with some APIs but in the end it's > > > > probably better to just open-code this. > > > > > > I don't mind open-coding this. > > > > > > > As a bonus, open-coding will avoid the need > > > > for cast above, which is good: casts make code more > > > > fragile. > > > > > > But I still don't understand why we can eliminate casting: > > > > > > u32 reg_val; > > > > > > reg_val = *((u32 *)(bitmap + REG_POS(vec))); > > > if (reg_val) > > > return __fls(reg_val) + vec; > > > > > > (I'm not sure compilers are allowed to push out the value and > > > do multiple references for this code as explained in > > > https://lwn.net/Articles/508991/ > > > > So you *were* talking about concurrency? > > Yes and no, please see below. > > > And you expect to solve it somehow without barriers > > explicit or implicit? > > What I want to make clear is that the value we pass to > __fls() is not zero, not any more, to avoid undefined > behaviour. > > So as you showed below, if the value passed to __fls() is > exactly from the register, which we did non-zero check, > that's fine. Barriers are not related here. > > But as can be seen in the last part of the article above, > that's may theoretically not be guranteed? It's not guaranteed if another thread can modify the bitmap. Is this the case here? If yes we need at least ACCESS_ONCE. > Anyway, I'm now thinking that we do not care about such > things here, and can just follow your advice, yes? Unless you see an issue with it ... > > > > > ) > > > > > > > > > If you mean > > > > > > u32 *reg; > > > > > > reg = bitmap + REG_POS(vec); > > > if (*reg) > > > return __fls(*reg) + vec; > > > > yes > > > > > I'm still not confident if this is a good style. > > > I rarely see code doing > > > > > > if (*p) > > > __fls(*p); > > > > > > This looks like explicite multiple references: I'm not saying > > > this will actually be compiled to do multiple references. > > > > > > Thanks, > > > Takuya > > > > It's just weird. Both versions are exactly equivalent in C. > > Adding a temporary changes *nothing* so the best readability > > wins. And IMHO, a version that does not cast wins hands down. > > I did a small test just to give you an example: > > Thank you for the example. > > What you showed is what I wanted to mean by > "I'm not saying this will actually be compiled to ..." > > Thanks, > Takuya > > > > > [mst@robin ~]$ cat a.c > > > > int foo(void *bitmap) > > { > > unsigned *reg; > > > > reg = bitmap + 4; > > if (*reg) > > return *reg + 1; > > > > return -1; > > } > > [mst@robin ~]$ cat b.c > > > > int foo(void *bitmap) > > { > > unsigned reg; > > > > reg = *((unsigned *)(bitmap + 4)); > > if (reg) > > return reg + 1; > > > > return -1; > > } > > > > [mst@robin ~]$ gcc -O2 -c a.c > > [mst@robin ~]$ gcc -O2 -c b.c > > > > > > [mst@robin ~]$ objdump -ld a.o > > > > a.o: file format elf32-i386 > > > > > > Disassembly of section .text: > > > > 00000000 : > > foo(): > > 0: 8b 44 24 04 mov 0x4(%esp),%eax > > 4: 8b 50 04 mov 0x4(%eax),%edx > > 7: b8 ff ff ff ff mov $0xffffffff,%eax > > c: 8d 4a 01 lea 0x1(%edx),%ecx > > f: 85 d2 test %edx,%edx > > 11: 0f 45 c1 cmovne %ecx,%eax > > 14: c3 ret > > [mst@robin ~]$ objdump -ld b.o > > > > b.o: file format elf32-i386 > > > > > > Disassembly of section .text: > > > > 00000000 : > > foo(): > > 0: 8b 44 24 04 mov 0x4(%esp),%eax > > 4: 8b 50 04 mov 0x4(%eax),%edx > > 7: b8 ff ff ff ff mov $0xffffffff,%eax > > c: 8d 4a 01 lea 0x1(%edx),%ecx > > f: 85 d2 test %edx,%edx > > 11: 0f 45 c1 cmovne %ecx,%eax > > 14: c3 ret