From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH] KVM: x86: lapic: Fix the misuse of likely() in
 find_highest_vector()
Date: Thu, 30 Aug 2012 13:44:59 +0300
Message-ID: <20120830104458.GA19997@redhat.com>
References: <20120824181549.e1535ae0.yoshikawa.takuya@oss.ntt.co.jp>
 <20120827202542.GA1414@amt.cnet>
 <20120828185756.c754f4b4.yoshikawa.takuya@oss.ntt.co.jp>
 <20120829191057.GA10834@amt.cnet>
 <20120829225120.GA9146@redhat.com>
 <20120830100906.5b4bcffd.yoshikawa.takuya@oss.ntt.co.jp>
 <20120830063702.GB9286@redhat.com>
 <20120830185052.086cbb0f.yoshikawa.takuya@oss.ntt.co.jp>
 <20120830101033.GA19622@redhat.com>
 <20120830192439.3e088595.yoshikawa.takuya@oss.ntt.co.jp>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Marcelo Tosatti <mtosatti@redhat.com>, avi@redhat.com,
	kvm@vger.kernel.org
To: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:37721 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751560Ab2H3LVc (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 30 Aug 2012 07:21:32 -0400
Content-Disposition: inline
In-Reply-To: <20120830192439.3e088595.yoshikawa.takuya@oss.ntt.co.jp>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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