Re: [RFC][PATCH] srcu: Implement call_srcu()

[Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>]

(2012/02/01 19:49), Avi Kivity wrote:
> On 02/01/2012 12:44 PM, Avi Kivity wrote:
>> On 02/01/2012 12:22 PM, Peter Zijlstra wrote:
>>> One of the things I was thinking of is adding a sequence counter in the
>>> per-cpu data. Using that we could do something like:
>>>
>>>    unsigned int seq1 = 0, seq2 = 0, count = 0;
>>>    int cpu, idx;
>>>
>>>    idx = ACCESS_ONCE(sp->completions)&  1;
>>>
>>>    for_each_possible_cpu(cpu)
>>> 	seq1 += per_cpu(sp->per_cpu_ref, cpu)->seq;
>>>
>>>    for_each_possible_cpu(cpu)
>>> 	count += per_cpu(sp->per_cpu_ref, cpu)->c[idx];
>>>
>>>    for_each_possible_cpu(cpu)
>>> 	seq2 += per_cpu(sp->per_cpu_ref, cpu)->seq;
>>>
>>>    /*
>>>     * there's no active references and no activity, we pass
>>>     */
>>>    if (seq1 == seq2&&  count == 0)
>>> 	return;
>>>
>>>    synchronize_srcu_slow();
>>>
>>>
>>> This would add a fast-path which should catch the case Avi outlined
>>> where we call sync_srcu() when there's no other SRCU activity.
>>
>> Sorry, I was inaccurate.  In two of the cases indeed we don't expect
>> guest activity, and we're okay with waiting a bit if there is guest
>> activity - when we're altering the guest physical memory map.  But the
>> third case does have concurrent guest activity with
>> synchronize_srcu_expedited() and we still need it fast - that's when
>> userspace reads the dirty bitmap log of a running guest and replaces it
>> with a new bitmap.
>>
>> There may be a way to convert it to call_srcu() though.  Without
>> synchronize_srcu_expedited(), kvm sees both the old and the new bitmaps,
>> but that's fine, since the dirty bits will go *somewhere*, and we can
>> pick them up later in call_srcu().  The only problem is if this is the
>> very last call to kvm_vm_ioctl_get_dirty_log(), and the callback
>> triggers after it returns - we end up with a bag of bits with not one to
>> return them to.  Maybe we can detect this conditions (all vcpus ought to
>> be stopped), and do something like:
>>
>>
>>     if (all vcpus stopped) {
>>          /* no activity, this should be fast */
>>          synchronize_srcu()
>>          /* collect and return bits */
>>     } else {
>>          call_srcu(collect bits)
>>     }
>>
>> still a snag - we can't reliably detect that all vcpus are stopped, they
>> may be just resting in userspace, and restart while synchronize_srcu()
>> is running.
>>
>> Marcelo?
>>
>
> Or something completely different - we can remove srcu from the equation
> completely in this case.  Use just one bitmap (so no

I am already testing various possibilities like this.
For VGA, using clear_bit() (+ rmap write protect) works well!

> rcu_assign_pointer), and use atomic operations to copy and clear:
>
>    word = bitmap[i]
>    put_user(word)
>    atomic_and(&bitmap[i], ~word)
>
>

This kind of this was really slow IIRC.


How about just doing:

take a spin_lock
copy the entire (or some portions of) bitmap locally
clear the bitmap
unlock

write protect the dirty pages based on the copied dirty data

copy_to_user



I can show you some performance numbers, this weekend, if you like.

	Takuya