On Thu, 30 Aug 2012 13:10:33 +0300
"Michael S. Tsirkin" <mst@xxxxxxxxxx> wrote:
> > OK, I'll do these on top of this patch.
>
> Tweaking these 5 lines for readability across multiple
> patches is just not worth it.
> As long as we do random cleanups of this function it's probably easier
> to just do them all in one patch.
OK.
> > > Something like the below pseudo code would do this I think?
> > >
> > > #define APIC_VECTORS_PER_REG 32
> > >
> > > int vec;
> > > for (vec = MAX_APIC_VECTOR - APIC_VECTORS_PER_REG;
> > > vec -= APIC_VECTORS_PER_REG; vec >= 0) {
> > > u32 *reg = bitmap + REG_POS(vec);
> >
> > We want to introduce apic_read_register(bitmap, reg) instead.
> > u32 reg = apic_read_register(bitmap, REG_POS(vec));
>
> If Marcelo takes it, I don't mind :)
>
> > > if (*reg)
> > > return __fls(*reg) - 1 + vec;
> >
> > Because it is not clear that this *reg is the same value
> > tested before.
>
> Before - where? Looks like this is the only place where
> *reg is used.
if (*reg) // BEFORE
return __fls(*reg) - 1 + vec; // AFTER
Unless the value pointed to by a pointer cannot be updated
concurrently, it seems a good practice to use a local variable
explicitely in C level.
I know that this will not change anything actually, but many
bitops functions do similar things.
Takuya
> > > }
> > > return -1
> > >
> > > count_vectors similar:
> > >
> > > for (vec = 0; vec < MAX_APIC_VECTOR; vec += APIC_VECTORS_PER_REG) {
> > > u32 *reg = bitmap + REG_POS(vec);
> >
> > Same here.
>
> Same question :)
--
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
