linux-kernel - Re: [PATCH 7/8] KVM: make async

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 28 Oct 2010 11:14:50 +0200
From:	Gleb Natapov <gleb@...hat.com>
To:	Xiao Guangrong <xiaoguangrong@...fujitsu.com>
Cc:	Avi Kivity <avi@...hat.com>, Marcelo Tosatti <mtosatti@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>, KVM <kvm@...r.kernel.org>
Subject: Re: [PATCH 7/8] KVM: make async_pf work queue lockless

On Thu, Oct 28, 2010 at 05:08:58PM +0800, Xiao Guangrong wrote:
> On 10/27/2010 07:41 PM, Gleb Natapov wrote:
> > On Wed, Oct 27, 2010 at 05:09:41PM +0800, Xiao Guangrong wrote:
> >> The async_pf number is very few since only pending interrupt can
> >> let it re-enter to the guest mode.
> >>
> >> During my test(Host 4 CPU + 4G, Guest 4 VCPU + 6G), it's no
> >> more than 10 requests in the system.
> >>
> >> So, we can only increase the completion counter in the work queue
> >> context, and walk vcpu->async_pf.queue list to get all completed
> >> async_pf
> >>
> > That depends on the load. I used memory cgroups to create very big
> > memory pressure and I saw hundreds of apfs per second. We shouldn't
> > optimize for very low numbers. With vcpu->async_pf.queue having more
> > then one element I am not sure your patch is beneficial.
> > 
> 
> Maybe we need a new no-lock way to record the complete apfs, i'll reproduce
> your test environment and improve it.
> 
That is always welcomed :)

> >> +
> >> +		list_del(&work->queue);
> >> +		vcpu->async_pf.queued--;
> >> +		kmem_cache_free(async_pf_cache, work);
> >> +		if (atomic_dec_and_test(&vcpu->async_pf.done))
> >> +			break;
> > You should do atomic_dec() and always break. We cannot inject two apfs during
> > one vcpu entry.
> > 
> 
> Sorry, i'm little confused. 
> 
> Why 'atomic_dec_and_test(&vcpu->async_pf.done)' always break? async_pf.done is used to
In your code it is not, but it should (at least if guest is PV, read
below).

> record the complete apfs and many apfs may be completed when vcpu enters guest mode(it
> means vcpu->async_pf.done > 1)
> 
Correct, but only one apf should be handled on each vcpu entry in case
of PV guest. Look at kvm_arch_async_page_present(vcpu, work); that is called
in a loop in your code. If vcpu->arch.apf.msr_val & KVM_ASYNC_PF_ENABLED
is not null it injects exception into the guest. You can't inject more
then one exception on each guest entry. If guest is not PV you are
correct that we can loop here until vcpu->async_pf.done == 0.

> Look at the current code:
> 
> void kvm_check_async_pf_completion(struct kvm_vcpu *vcpu)
> {
> 	......
> 	spin_lock(&vcpu->async_pf.lock);
> 	work = list_first_entry(&vcpu->async_pf.done, typeof(*work), link);
> 	list_del(&work->link);
> 	spin_unlock(&vcpu->async_pf.lock);
> 	......
> }
> 
> You only handle one complete apf, why we inject them at once? I missed something? :-(

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/