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

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

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