public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
From: Avi Kivity <avi@redhat.com>
To: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>,
	Marcelo Tosatti <mtosatti@redhat.com>,
	kvm@vger.kernel.org
Subject: Re: [PATCH v2 0/4] Fix accessed bit tracking
Date: Tue, 08 Jun 2010 10:54:39 +0300	[thread overview]
Message-ID: <4C0DF73F.603@redhat.com> (raw)
In-Reply-To: <4C0DE904.5050007@cn.fujitsu.com>

On 06/08/2010 09:53 AM, Xiao Guangrong wrote:
>
> Avi Kivity wrote:
>    
>> On 06/08/2010 05:35 AM, Xiao Guangrong wrote:
>>      
>>>        
>>>> We can avoid the exchange in most cases, for example if the new spte has
>>>> the accessed bit set (already in the patch set) or if the page is
>>>> already marked as accessed, or if we see the old spte has the accessed
>>>> bit set (so no race can occur).  I'll update the patches to avoid
>>>> atomics when possible.
>>>>
>>>>          
>>> Umm, the reason that we need atomics here is to avoid vcpu to update
>>> spte when we read A bit
>>> form it, so, perhaps we can use below way to avoid atomics completely:
>>>
>>> - set reserved bit in spte
>>> - get A bit form spte
>>> - set new spte
>>>
>>> the worst case is cause vcpu #PF here, but it doesn't matter since the
>>> old mapping is already invalid,
>>> also need a remote tlb flush later.
>>>
>>>        
>> To set the reserved bit in the spte, you need an atomic operation (well,
>> unless you use a sub-word-acccess to set a reserved bit in the high 32
>> bits).
>>      
> I think we no need atomic here, for example, we can do it like this:
>
> *spte |= RSVD_BIT
> [ maybe need a write barrier here? ]
>    

That can drop an A bit.  If *spte starts out with A cleared, we can have

cpu0                        cpu1

fetch *spte (A=0)
                             set A bit
write *spte (A=0, RSVD=1)


> After this sentence completed, we can ensure that the spte can not updated A bit
> by vcpu, so we can get A bit safely.
>    

You also need a remote tlb flush...

>>> Yes, but atomics are "LOCK" instructions, it can stop multiple cpus
>>> runing in parallel.
>>>
>>>        
>> Only if those cpus are accessing the same word you're accessing.
>>
>>      
> Oh, you are right, the LOCK only locked the memory defined by the destination operand,
> but i also recall that page table access can pass LOCK instruction, below description
> is form intel' spec Vol. 3 7-5:
>
> Locked operations are atomic with respect to all other memory operations and all externally
> visible events. Only instruction fetch and page table accesses can pass locked instructions.
> Locked instructions can be used to synchronize data written by one processor and read by another
> processor.
>    

But actually setting the A bit will use LOCK itself.  So in the 
following sequence


    write pte (A=0)
    test_and_clear_bit(A, pte)
    access memory through pte

the test_and_clear_bit can return A=1 due to speculation and the 
parapgraph above, but setting the A bit by the processor will happen 
with a bus lock, so it won't lose information.

-- 
error compiling committee.c: too many arguments to function


  reply	other threads:[~2010-06-08  7:54 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-06-07  7:10 [PATCH v2 0/4] Fix accessed bit tracking Avi Kivity
2010-06-07  7:10 ` [PATCH v2 1/4] KVM: MMU: Introduce drop_spte() Avi Kivity
2010-06-07  7:10 ` [PATCH v2 2/4] KVM: MMU: Move accessed/dirty bit checks from rmap_remove() to drop_spte() Avi Kivity
2010-06-07  8:16   ` Lai Jiangshan
2010-06-07  9:01     ` Avi Kivity
2010-06-07  7:10 ` [PATCH v2 3/4] KVM: MMU: Atomically check for accessed bit when dropping an spte Avi Kivity
2010-06-08  2:07   ` Xiao Guangrong
2010-06-08  5:51     ` Avi Kivity
2010-06-07  7:10 ` [PATCH v2 4/4] KVM: MMU: Don't drop accessed bit while updating " Avi Kivity
2010-06-07  8:43 ` [PATCH v2 0/4] Fix accessed bit tracking Lai Jiangshan
2010-06-07  9:00   ` Avi Kivity
2010-06-08  2:35     ` Xiao Guangrong
2010-06-08  5:24       ` Avi Kivity
2010-06-08  6:53         ` Xiao Guangrong
2010-06-08  7:54           ` Avi Kivity [this message]
2010-06-08  8:30             ` 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=4C0DF73F.603@redhat.com \
    --to=avi@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=laijs@cn.fujitsu.com \
    --cc=mtosatti@redhat.com \
    --cc=xiaoguangrong@cn.fujitsu.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