[PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Will Deacon]

For consistency with the rest of the stage-2 page-table page allocations
(performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
included in the GFP flags for the PGD pages.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/hyp/pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 0cdf6e461cbd..95141b0d6088 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -846,7 +846,7 @@ int kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm *kvm)
 	u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0;
 
 	pgd_sz = kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE;
-	pgt->pgd = alloc_pages_exact(pgd_sz, GFP_KERNEL | __GFP_ZERO);
+	pgt->pgd = alloc_pages_exact(pgd_sz, GFP_KERNEL_ACCOUNT | __GFP_ZERO);
 	if (!pgt->pgd)
 		return -ENOMEM;
 
-- 
2.29.0.rc2.309.g374f81d7ae-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Gavin Shan]

Hi Will,

On 10/27/20 1:44 AM, Will Deacon wrote:
> For consistency with the rest of the stage-2 page-table page allocations
> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
> included in the GFP flags for the PGD pages.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Quentin Perret <qperret@google.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   arch/arm64/kvm/hyp/pgtable.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

The patch itself looks good to me:

Reviewed-by: Gavin Shan <gshan@redhat.com>

Another question is why the page-table pages for hyp mode aren't
allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and hyp_map_walker()?
The page-table pages for host or guest are allocated with GFP_PGTABLE_USER
in alloc_pte_one().

#define GFP_PGTABLE_USER      (GFP_PGTABLE_KERNEL | __GFP_ACCOUNT)
#define GFP_PGTABLE_KERNEL    (GFP_KERNEL | __GFP_ZERO)

Cheers,
Gavin

> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 0cdf6e461cbd..95141b0d6088 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -846,7 +846,7 @@ int kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm *kvm)
>   	u32 start_level = VTCR_EL2_TGRAN_SL0_BASE - sl0;
>   
>   	pgd_sz = kvm_pgd_pages(ia_bits, start_level) * PAGE_SIZE;
> -	pgt->pgd = alloc_pages_exact(pgd_sz, GFP_KERNEL | __GFP_ZERO);
> +	pgt->pgd = alloc_pages_exact(pgd_sz, GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>   	if (!pgt->pgd)
>   		return -ENOMEM;
>   
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Marc Zyngier]

On 2020-10-26 23:41, Gavin Shan wrote:
> Hi Will,
> 
> On 10/27/20 1:44 AM, Will Deacon wrote:
>> For consistency with the rest of the stage-2 page-table page 
>> allocations
>> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT 
>> is
>> included in the GFP flags for the PGD pages.
>> 
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Quentin Perret <qperret@google.com>
>> Signed-off-by: Will Deacon <will@kernel.org>
>> ---
>>   arch/arm64/kvm/hyp/pgtable.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
> 
> The patch itself looks good to me:
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> 
> Another question is why the page-table pages for hyp mode aren't
> allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and 
> hyp_map_walker()?

Why user task would you account the hypervisor mappings to? The page 
tables
used for HYP code and data are definitely not attributable to any task.

The kvm and kvm_vcpu mappings *could* be attributed to a user task, but
the page tables are likely shared with other tasks. So who gets the 
blame?

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Gavin Shan]

Hi Marc,

On 10/27/20 8:27 PM, Marc Zyngier wrote:
> On 2020-10-26 23:41, Gavin Shan wrote:
>> On 10/27/20 1:44 AM, Will Deacon wrote:
>>> For consistency with the rest of the stage-2 page-table page allocations
>>> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
>>> included in the GFP flags for the PGD pages.
>>>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Quentin Perret <qperret@google.com>
>>> Signed-off-by: Will Deacon <will@kernel.org>
>>> ---
>>>   arch/arm64/kvm/hyp/pgtable.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>

[...]

>> Another question is why the page-table pages for hyp mode aren't
>> allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and hyp_map_walker()?
> 
> Why user task would you account the hypervisor mappings to? The page tables
> used for HYP code and data are definitely not attributable to any task.
> 
> The kvm and kvm_vcpu mappings *could* be attributed to a user task, but
> the page tables are likely shared with other tasks. So who gets the blame?
> 

As replied to Will, the qemu could be put into one cgroup (memory cgroup
specificly). Without __GFP_ACCOUNT, the consumed memory for the page-table
isn't limited. I think qemu is the owner of the consumed memory in this
case.

Cheers,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Marc Zyngier]

On 2020-10-28 05:56, Gavin Shan wrote:
> Hi Marc,
> 
> On 10/27/20 8:27 PM, Marc Zyngier wrote:
>> On 2020-10-26 23:41, Gavin Shan wrote:
>>> On 10/27/20 1:44 AM, Will Deacon wrote:
>>>> For consistency with the rest of the stage-2 page-table page 
>>>> allocations
>>>> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT 
>>>> is
>>>> included in the GFP flags for the PGD pages.
>>>> 
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Cc: Quentin Perret <qperret@google.com>
>>>> Signed-off-by: Will Deacon <will@kernel.org>
>>>> ---
>>>>   arch/arm64/kvm/hyp/pgtable.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>> 
> 
> [...]
> 
>>> Another question is why the page-table pages for hyp mode aren't
>>> allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and 
>>> hyp_map_walker()?
>> 
>> Why user task would you account the hypervisor mappings to? The page 
>> tables
>> used for HYP code and data are definitely not attributable to any 
>> task.
>> 
>> The kvm and kvm_vcpu mappings *could* be attributed to a user task, 
>> but
>> the page tables are likely shared with other tasks. So who gets the 
>> blame?
>> 
> 
> As replied to Will, the qemu could be put into one cgroup (memory 
> cgroup
> specificly). Without __GFP_ACCOUNT, the consumed memory for the 
> page-table
> isn't limited. I think qemu is the owner of the consumed memory in this
> case.

*which* QEMU? Take two struct kvm, each representing a VM, each created
by a separate instance of QEMU. These two structures happen to share
a physical page (which is pretty likely in your case given that you use
crazy page sizes).

Who gets the accounting for the page tables that map that page? No, you
are not allowed to align the size of the structure to make the problem 
go
away.

Either you account the page tables to both, or you account them to none
of them. Given that both are wrong, I choose the path of least effort.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Will Deacon]

On Tue, Oct 27, 2020 at 10:41:33AM +1100, Gavin Shan wrote:
> On 10/27/20 1:44 AM, Will Deacon wrote:
> > For consistency with the rest of the stage-2 page-table page allocations
> > (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
> > included in the GFP flags for the PGD pages.
> > 
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Quentin Perret <qperret@google.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >   arch/arm64/kvm/hyp/pgtable.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> 
> The patch itself looks good to me:
> 
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> 
> Another question is why the page-table pages for hyp mode aren't
> allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and hyp_map_walker()?
> The page-table pages for host or guest are allocated with GFP_PGTABLE_USER
> in alloc_pte_one().
> 
> #define GFP_PGTABLE_USER      (GFP_PGTABLE_KERNEL | __GFP_ACCOUNT)
> #define GFP_PGTABLE_KERNEL    (GFP_KERNEL | __GFP_ZERO)

I think because the guest pages are allocated as a direct result of the VMM,
whereas I tend to think of the hyp page-tables more like kernel page-tables
(which aren't accounted afaik: see GFP_PGTABLE_USER vs GFP_PGTABLE_KERNEL).

Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Gavin Shan]

Hi Will,

On 10/27/20 8:27 PM, Will Deacon wrote:
> On Tue, Oct 27, 2020 at 10:41:33AM +1100, Gavin Shan wrote:
>> On 10/27/20 1:44 AM, Will Deacon wrote:
>>> For consistency with the rest of the stage-2 page-table page allocations
>>> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
>>> included in the GFP flags for the PGD pages.
>>>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Quentin Perret <qperret@google.com>
>>> Signed-off-by: Will Deacon <will@kernel.org>
>>> ---
>>>    arch/arm64/kvm/hyp/pgtable.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>
>> The patch itself looks good to me:
>>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>>
>> Another question is why the page-table pages for hyp mode aren't
>> allocated with __GFP_ACCOUNT in kvm_pgtable_hyp_init and hyp_map_walker()?
>> The page-table pages for host or guest are allocated with GFP_PGTABLE_USER
>> in alloc_pte_one().
>>
>> #define GFP_PGTABLE_USER      (GFP_PGTABLE_KERNEL | __GFP_ACCOUNT)
>> #define GFP_PGTABLE_KERNEL    (GFP_KERNEL | __GFP_ZERO)
> 
> I think because the guest pages are allocated as a direct result of the VMM,
> whereas I tend to think of the hyp page-tables more like kernel page-tables
> (which aren't accounted afaik: see GFP_PGTABLE_USER vs GFP_PGTABLE_KERNEL).
> 

Assume qemu is the only userspace counter-part. qemu is the process and could
be put into one cgroup (memory cgroup specificly). Without __GFP_ACCOUNT,
the memory consumed by page-table isn't limited by cgroup policies. I'm not
sure if this is exactly what we want, even it's trivial in terms of the issue
itself and the amount of consumed memory.

Cheers,
Gavin

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT

[Marc Zyngier]

On Mon, 26 Oct 2020 14:44:23 +0000, Will Deacon wrote:
> For consistency with the rest of the stage-2 page-table page allocations
> (performing using a kvm_mmu_memory_cache), ensure that __GFP_ACCOUNT is
> included in the GFP flags for the PGD pages.

Applied to next, thanks!

[1/1] KVM: arm64: Allocate stage-2 pgd pages with GFP_KERNEL_ACCOUNT
      commit: 7efe8ef274024ef1d5c495c79dfcbbff38c5f366

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm