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: Thu, 30 Aug 2012 19:49:23 +0300 Message-ID: <20120830164923.GA22024@redhat.com> References: <20120829225120.GA9146@redhat.com> <20120830100906.5b4bcffd.yoshikawa.takuya@oss.ntt.co.jp> <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> 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]:55865 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751537Ab2H3QsI (ORCPT ); Thu, 30 Aug 2012 12:48:08 -0400 Content-Disposition: inline In-Reply-To: <20120831010956.000856ead3c6f96dbe2bec46@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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? And you expect to solve it somehow without barriers explicit or implicit? > ) > > > 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: [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 -- MST