Re: [PATCH v9 2/4] arm: ARMv7 dirty page logging inital mem region write protect (w/no huge PUD support)

Mario Smarduch
2014-08-12T21:08:27Z

On 08/12/2014 02:36 AM, Christoffer Dall wrote:
> On Mon, Aug 11, 2014 at 06:36:14PM -0700, Mario Smarduch wrote:
>> On 08/11/2014 12:12 PM, Christoffer Dall wrote:
>
> [...]
>
>>>> +/**
>>>> + * stage2_wp_range() - write protect stage2 memory region range
>>>> + * @kvm: The KVM pointer
>>>> + * @start: Start address of range
>>>> + * &end: End address of range
>>>> + */
>>>> +static void stage2_wp_range(struct kvm *kvm, phys_addr_t addr, phys_addr_t end)
>>>> +{
>>>> + pgd_t *pgd;
>>>> + phys_addr_t next;
>>>> +
>>>> + pgd = kvm->arch.pgd + pgd_index(addr);
>>>> + do {
>>>> + /*
>>>> + * Release kvm_mmu_lock periodically if the memory region is
>>>> + * large features like detect hung task, lock detector or lock
>>> large. Otherwise, we may see panics due to..
>>>> + * dep may panic. In addition holding the lock this long will
>>> extra white space ^^ Additionally, holding the lock for a
>>> long timer will
>>>> + * also starve other vCPUs. Applies to huge VM memory regions.
>>> ^^^ I don't understand this
>>> last remark.
>> Sorry overlooked this.
>>
>> While testing - VM regions that were small (~1GB) holding the mmu_lock
>> caused not problems, but when I was running memory regions around 2GB large
>> some kernel lockup detection/lock contention options (some selected by default)
>> caused deadlock warnings/panics in host kernel.
>>
>> This was in one my previous review comments sometime ago, I can go back
>> and find the options.
>>
>
> Just drop the last part of the comment, so the whole thing reads:
>
> /*
> * Release kvm_mmu_lock periodically if the memory region is
> * large. Otherwise, we may see kernel panics from debugging features
> * such as "detect hung task", "lock detector" or "lock dep checks".
> * Additionally, holding the lock too long will also starve other vCPUs.
> */
>
> And check the actual names of those debugging features or use the
> CONFIG_<WHATEVER> names and say "we may see kernel panics with CONFIG_X,
> CONFIG_Y, and CONFIG_Z.
>
> Makes sense?

Yep sure does.

>
> -Christoffer
>