On Thu, 30 Aug 2012 19:49:23 +0300
"Michael S. Tsirkin" <mst@xxxxxxxxxx> 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@xxxxxxxxxx> 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?
Anyway, I'm now thinking that we do not care about such
things here, and can just follow your advice, yes?
>
> > )
> >
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
