Re: [PATCH v12 2/6] KVM: Add generic support for dirty page logging

[Mario Smarduch <m.smarduch@samsung.com>]

On 11/02/2014 07:01 PM, Takuya Yoshikawa wrote:
> On Thu, 30 Oct 2014 12:19:00 -0700
> Mario Smarduch <m.smarduch@samsung.com> wrote:
> 
>> On 10/30/2014 05:14 AM, Cornelia Huck wrote:
>>> On Wed, 22 Oct 2014 15:34:07 -0700
>>> Mario Smarduch <m.smarduch@samsung.com> wrote:
>>>
>>>> This patch defines KVM_GENERIC_DIRTYLOG, and moves dirty log read function
>>>> to it's own file virt/kvm/dirtylog.c. x86 is updated to use the generic
>>>> dirty log interface, selecting KVM_GENERIC_DIRTYLOG in its Kconfig and 
>>>> makefile. No other architectures are affected, each uses it's own version.
>>>> This changed from previous patch revision where non-generic architectures 
>>>> were modified.
>>>>
>>>> In subsequent patch armv7 does samething. All other architectures continue
>>>> use architecture defined version.
>>>>
>>>
>>> Hm.
>>>
>>> "The x86 specific version of dirty page logging is generic enough to be
>>> used by other architectures, noteably ARMv7. So let's move the x86 code
>>> under virt/kvm/ and make it depend on KVM_GENERIC_DIRTYLOG. Other
>>> architectures continue to use their own implementations."
>>>
>>> ?
>>
>> I'll update descriptions for both patches, with the more concise
>> descriptions.
> 
> I don't think it's so generic.
> 
> Especially, the relationship between mmu_lock and TLB flushing has
> been changed a few times for optimizing x86 mmu code, and it may change
> in the future again.
> 
> Since how mmu_lock is used is depends on each arch, it's not so
> simple to make the function generic IMO.
> 
> Thanks,
> 	Takuya

Hi Takuya,

If the bitmap can be processed locklessly then it's not reusable for sure.

The interface is not "to generic", but "generic enough" for arm/armv8 to
reuse.
For TLB flushing I would think optimizations would wind up in arch subtree.
I'm not convinced that location of couple lines doesn't make the
interface re-usable, going back to 3.10 it hardly changed.

- Mario

> 
>>
>> Thanks.
>>
>>>
>>>>
>>>> Signed-off-by: Mario Smarduch <m.smarduch@samsung.com>
>>>> ---
>>>>  arch/x86/include/asm/kvm_host.h |    3 --
>>>>  arch/x86/kvm/Kconfig            |    1 +
>>>>  arch/x86/kvm/Makefile           |    1 +
>>>>  arch/x86/kvm/x86.c              |   86 ------------------------------
>>>>  include/linux/kvm_host.h        |    4 ++
>>>>  virt/kvm/Kconfig                |    3 ++
>>>>  virt/kvm/dirtylog.c             |  112 +++++++++++++++++++++++++++++++++++++++
>>>>  7 files changed, 121 insertions(+), 89 deletions(-)
>>>>  create mode 100644 virt/kvm/dirtylog.c
>>>>
>>>
>>>> diff --git a/virt/kvm/dirtylog.c b/virt/kvm/dirtylog.c
>>>> new file mode 100644
>>>> index 0000000..67ffffa
>>>> --- /dev/null
>>>> +++ b/virt/kvm/dirtylog.c
>>>> @@ -0,0 +1,112 @@
>>>> +/*
>>>> + * kvm generic dirty logging support, used by architectures that share
>>>> + * comman dirty page logging implementation.
>>>
>>> s/comman/common/
>>>
>>> The approach looks sane to me, especially as it does not change other
>>> architectures needlessly.
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>