From: Marcelo Tosatti <mtosatti@redhat.com>
To: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Cc: Avi Kivity <avi@redhat.com>, LKML <linux-kernel@vger.kernel.org>,
KVM <kvm@vger.kernel.org>
Subject: Re: [PATCH v4 06/10] KVM: MMU: fast path of handling guest page fault
Date: Wed, 2 May 2012 18:07:01 -0300 [thread overview]
Message-ID: <20120502210701.GA12604@amt.cnet> (raw)
In-Reply-To: <4FA0C607.5010002@linux.vnet.ibm.com>
On Wed, May 02, 2012 at 01:28:39PM +0800, Xiao Guangrong wrote:
> On 05/01/2012 09:34 AM, Marcelo Tosatti wrote:
>
>
> >
> > It is getting better, but not yet, there are still reads of sptep
> > scattered all over (as mentioned before, i think a pattern of read spte
> > once, work on top of that, atomically write and then deal with results
> > _everywhere_ (where mmu lock is held) is more consistent.
> >
>
>
> But we only need care the path which depends on is_writable_pte(), no?
Yes.
> So, where call is_writable_pte() are spte_has_volatile_bits(),
> spte_write_protect() and set_spte().
>
> I have changed these functions:
> In spte_has_volatile_bits():
> static bool spte_has_volatile_bits(u64 spte)
> {
> + /*
> + * Always atomicly update spte if it can be updated
> + * out of mmu-lock.
> + */
> + if (spte_can_lockless_update(spte))
> + return true;
> +
>
> In spte_write_protect():
>
> + spte = mmu_spte_update(sptep, spte);
> +
> + if (is_writable_pte(spte))
> + *flush |= true;
> +
> The 'spte' is from atomically read-write (xchg).
>
> in set_spte():
> set_pte:
> - mmu_spte_update(sptep, spte);
> + entry = mmu_spte_update(sptep, spte);
> /*
> * If we overwrite a writable spte with a read-only one we
> * should flush remote TLBs. Otherwise rmap_write_protect
> The 'entry' is also the latest value.
>
> > /*
> > * If we overwrite a writable spte with a read-only one we
> > * should flush remote TLBs. Otherwise rmap_write_protect
> > * will find a read-only spte, even though the writable spte
> > * might be cached on a CPU's TLB.
> > */
> > if (is_writable_pte(entry) && !is_writable_pte(*sptep))
> > kvm_flush_remote_tlbs(vcpu->kvm);
> >
> > This is inconsistent with the above obviously.
> >
>
>
> 'entry' is not a problem since it is from atomically read-write as
> mentioned above, i need change this code to:
>
> /*
> * Optimization: for pte sync, if spte was writable the hash
> * lookup is unnecessary (and expensive). Write protection
> * is responsibility of mmu_get_page / kvm_sync_page.
> * Same reasoning can be applied to dirty page accounting.
> */
> if (!can_unsync && is_writable_pte(entry) /* Use 'entry' instead of '*sptep'. */
> goto set_pte
> ......
>
>
> if (is_writable_pte(entry) && !is_writable_pte(spte)) /* Use 'spte' instead of '*sptep'. */
> kvm_flush_remote_tlbs(vcpu->kvm);
What is of more importance than the ability to verify that this or that
particular case are ok at the moment is to write code in such a way that
its easy to verify that it is correct.
Thus the suggestion above:
"scattered all over (as mentioned before, i think a pattern of read spte
once, work on top of that, atomically write and then deal with results
_everywhere_ (where mmu lock is held) is more consistent."
next prev parent reply other threads:[~2012-05-02 21:11 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-04-25 4:00 [PATCH v4 00/10] KVM: MMU: fast page fault Xiao Guangrong
2012-04-25 4:01 ` [PATCH v4 01/10] KVM: MMU: return bool in __rmap_write_protect Xiao Guangrong
2012-04-25 4:01 ` [PATCH v4 02/10] KVM: MMU: abstract spte write-protect Xiao Guangrong
2012-04-25 4:02 ` [PATCH v4 03/10] KVM: VMX: export PFEC.P bit on ept Xiao Guangrong
2012-04-25 4:02 ` [PATCH v4 04/10] KVM: MMU: introduce SPTE_MMU_WRITEABLE bit Xiao Guangrong
2012-04-25 4:03 ` [PATCH v4 05/10] KVM: MMU: introduce SPTE_WRITE_PROTECT bit Xiao Guangrong
2012-04-25 4:03 ` [PATCH v4 06/10] KVM: MMU: fast path of handling guest page fault Xiao Guangrong
2012-04-26 23:45 ` Marcelo Tosatti
2012-04-27 5:53 ` Xiao Guangrong
2012-04-27 14:52 ` Marcelo Tosatti
2012-04-28 6:10 ` Xiao Guangrong
2012-05-01 1:34 ` Marcelo Tosatti
2012-05-02 5:28 ` Xiao Guangrong
2012-05-02 21:07 ` Marcelo Tosatti [this message]
2012-05-03 11:26 ` Xiao Guangrong
2012-05-05 14:08 ` Marcelo Tosatti
2012-05-06 9:36 ` Avi Kivity
2012-05-07 6:52 ` Xiao Guangrong
2012-04-29 8:50 ` Takuya Yoshikawa
2012-05-01 2:31 ` Marcelo Tosatti
2012-05-02 5:39 ` Xiao Guangrong
2012-05-02 21:10 ` Marcelo Tosatti
2012-05-03 12:09 ` Xiao Guangrong
2012-05-03 12:13 ` Avi Kivity
2012-05-03 0:15 ` Takuya Yoshikawa
2012-05-03 12:23 ` Xiao Guangrong
2012-05-03 12:40 ` Takuya Yoshikawa
2012-04-25 4:04 ` [PATCH v4 07/10] KVM: MMU: lockless update spte on fast page fault path Xiao Guangrong
2012-04-25 4:04 ` [PATCH v4 08/10] KVM: MMU: trace fast page fault Xiao Guangrong
2012-04-25 4:05 ` [PATCH v4 09/10] KVM: MMU: fix kvm_mmu_pagetable_walk tracepoint Xiao Guangrong
2012-04-25 4:06 ` [PATCH v4 10/10] KVM: MMU: document mmu-lock and fast page fault Xiao Guangrong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120502210701.GA12604@amt.cnet \
--to=mtosatti@redhat.com \
--cc=avi@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=xiaoguangrong@linux.vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).