Re: [PATCH -v3] KVM: x86: lapic: Clean up find_highest_vector() and count_vectors()

["Michael S. Tsirkin" <mst@redhat.com>]

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" <mst@redhat.com> 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>:
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>:
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