Subject: Re: [PART1 RFC v3 12/12] svm: Manage vcpu load/unload when enable
 AVIC
To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>
References: <1458281388-14452-1-git-send-email-Suravee.Suthikulpanit@amd.com>
 <1458281388-14452-13-git-send-email-Suravee.Suthikulpanit@amd.com>
 <20160318214450.GB2332@potion.brq.redhat.com> <56FCE556.80306@amd.com>
 <20160331141908.GA2171@potion.brq.redhat.com>
CC: <pbonzini@redhat.com>, <joro@8bytes.org>, <bp@alien8.de>,
        <gleb@kernel.org>, <alex.williamson@redhat.com>, <kvm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <wei@redhat.com>,
        <sherry.hurwitz@amd.com>
From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Message-ID: <57038E6B.3080808@amd.com>
Date: Tue, 5 Apr 2016 17:07:39 +0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0)
 Gecko/20100101 Thunderbird/38.6.0
MIME-Version: 1.0
In-Reply-To: <20160331141908.GA2171@potion.brq.redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2016 10:08:00.7040
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR12MB0436
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2475
Lines: 60

Hi Radim,

On 3/31/16 21:19, Radim Krčmář wrote:
> 2016-03-31 15:52+0700, Suravee Suthikulpanit:
>> On 03/19/2016 04:44 AM, Radim Krčmář wrote:
>>> 2016-03-18 01:09-0500, Suravee Suthikulpanit:
>>>> +	} else {
>>>> +		new_entry = READ_ONCE(*entry);
>>>> +		/**
>>>> +		 * This handles the case when vcpu is scheduled out
>>>> +		 * and has not yet not called blocking. We save the
>>>> +		 * AVIC running flag so that we can restore later.
>>>> +		 */
>>>
>>> is_running must be disabled in between ...blocking and ...unblocking,
>>> because we don't want to miss interrupts and block forever.
>>> I somehow don't get it from the comment. :)
>>
>> Not sure if I understand your concern.  However, the is_running bit
>> setting/clearing should be handled in the avic_set_running below. This part
>> only handles othe case when the is_running bit still set when calling
>> vcpu_put (and later on loading some other vcpus). This way, when we are
>> re-loading this vcpu, we can restore the is_running bit accordingly.
>
> I think that the comment is misleading.  The saved is_running flag only
> matters after svm_vcpu_blocking, yet the comment says that it handles
> the irrelevant case before.

Actually, my understanding is if the svm_vcpu_blocking() is called, the 
is_running bit would have been cleared. At this point, if the vcpu is 
unloaded. We should not need to worry about it. Is that not the case here?

> Another minor bug is that was_running isn't initialized to 1, so we need
> to halt before is_running gets set.

Just to make sure, you are referring to the point where the is_running 
is not set for first time the vcpu is loaded?  If so, I agree. Thanks 
for the good catch.

> It would be clearer to toggle a flag in svm_vcpu_(un)blocking and set
> is_running = !is_blocking.

Not sure what you meant here. We are already setting/unsetting the 
is_running bit when vcpu is blocking/unblocking. Are you suggesting just 
simply move the current avic_set_running() into the svm_vcpu_blocking 
and svm_vcpu_unblocking()?


> Doing so will also be immeasurably faster,
> because avic_vcpu_load is called far more than svm_vcpu_(un)blocking.

Actually, this is not the same as handling normal vcpu blocking and 
unblocking, which we are already setting/un-setting the is_running bit 
in the avic_set_running(). The was_running should only be set to 1 if 
the vcpu is unloaded but has not yet calling halt.

Am I missing your points somehow?

Thanks,
Suravee