On Thu, Aug 30, 2012 at 07:24:39PM +0900, Takuya Yoshikawa wrote:
> 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.
This last statement is very wrong. If you are trying to address concurrent
access on smp, using a varible will never fix it. You need ACCESS_ONCE,
barriers and all that jazz.
If instead you are talking about readability, using a wrapper just to do
'+' looks like a bit of an overkill to me: you almost literally do
#define plus(a,b) (a+b).
> I know that this will not change anything actually, but many
> bitops functions do similar things.
>
> Takuya
They do a bit more: they avoid duplication by calling
VEC_POS and REG_POS with the same paramater.
But let's not argue theoretically, send a patch and maintainers
will judge it.
> > > > }
> > > > 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
