[v4,8/9] s390/mm: hugetlb pages within a gmap can not be freed
diff mbox

Message ID 20180627135510.117945-9-frankja@linux.ibm.com
State New
Headers show

Commit Message

Janosch Frank June 27, 2018, 1:55 p.m. UTC
From: Dominik Dingel <dingel@linux.vnet.ibm.com>

Guests backed by huge pages could theoretically free unused pages via
the diagnose 10 instruction. We currently don't allow that, so we
don't have to refault it once it's needed again.

Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/mm/gmap.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

David Hildenbrand June 28, 2018, 7:42 a.m. UTC | #1
On 27.06.2018 15:55, Janosch Frank wrote:
> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
> 
> Guests backed by huge pages could theoretically free unused pages via
> the diagnose 10 instruction. We currently don't allow that, so we
> don't have to refault it once it's needed again.
> 
> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  arch/s390/mm/gmap.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index af0a87eeede0..c691b9d9d223 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -723,6 +723,9 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
>  		vmaddr |= gaddr & ~PMD_MASK;
>  		/* Find vma in the parent mm */
>  		vma = find_vma(gmap->mm, vmaddr);
> +		/* We do not discard pages that are backed by hugetlbfs */

Can you add why we don't do that?

> +		if (vma && is_vm_hugetlb_page(vma))
> +			continue;
>  		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
>  		zap_page_range(vma, vmaddr, size);
>  	}
> 

Apart from that

Reviewed-by: David Hildenbrand <david@redhat.com>
Janosch Frank June 28, 2018, 10:13 a.m. UTC | #2
On 28.06.2018 09:42, David Hildenbrand wrote:
> On 27.06.2018 15:55, Janosch Frank wrote:
>> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>
>> Guests backed by huge pages could theoretically free unused pages via
>> the diagnose 10 instruction. We currently don't allow that, so we
>> don't have to refault it once it's needed again.
>>
>> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
>> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> ---
>>  arch/s390/mm/gmap.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index af0a87eeede0..c691b9d9d223 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -723,6 +723,9 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
>>  		vmaddr |= gaddr & ~PMD_MASK;
>>  		/* Find vma in the parent mm */
>>  		vma = find_vma(gmap->mm, vmaddr);
>> +		/* We do not discard pages that are backed by hugetlbfs */
> 
> Can you add why we don't do that?

It's in the patch description, but if you also need it here, sure.

> 
>> +		if (vma && is_vm_hugetlb_page(vma))
>> +			continue;
>>  		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
>>  		zap_page_range(vma, vmaddr, size);
>>  	}
>>
> 
> Apart from that
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>
>
David Hildenbrand June 28, 2018, 10:32 a.m. UTC | #3
On 28.06.2018 12:13, Janosch Frank wrote:
> On 28.06.2018 09:42, David Hildenbrand wrote:
>> On 27.06.2018 15:55, Janosch Frank wrote:
>>> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>>
>>> Guests backed by huge pages could theoretically free unused pages via
>>> the diagnose 10 instruction. We currently don't allow that, so we
>>> don't have to refault it once it's needed again.
>>>
>>> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>> ---
>>>  arch/s390/mm/gmap.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index af0a87eeede0..c691b9d9d223 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -723,6 +723,9 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
>>>  		vmaddr |= gaddr & ~PMD_MASK;
>>>  		/* Find vma in the parent mm */
>>>  		vma = find_vma(gmap->mm, vmaddr);
>>> +		/* We do not discard pages that are backed by hugetlbfs */
>>
>> Can you add why we don't do that?
> 
> It's in the patch description, but if you also need it here, sure.

"We currently don't allow that, so we don't have to refault it once it's
needed again." why exactly can't we do that? Is it an artificial limitation?

> 
>>
>>> +		if (vma && is_vm_hugetlb_page(vma))
>>> +			continue;
>>>  		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
>>>  		zap_page_range(vma, vmaddr, size);
>>>  	}
>>>
>>
>> Apart from that
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>
> 
>
Janosch Frank June 28, 2018, 10:36 a.m. UTC | #4
On 28.06.2018 12:32, David Hildenbrand wrote:
> On 28.06.2018 12:13, Janosch Frank wrote:
>> On 28.06.2018 09:42, David Hildenbrand wrote:
>>> On 27.06.2018 15:55, Janosch Frank wrote:
>>>> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>>>
>>>> Guests backed by huge pages could theoretically free unused pages via
>>>> the diagnose 10 instruction. We currently don't allow that, so we
>>>> don't have to refault it once it's needed again.
>>>>
>>>> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
>>>> Reviewed-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>>> ---
>>>>  arch/s390/mm/gmap.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>>> index af0a87eeede0..c691b9d9d223 100644
>>>> --- a/arch/s390/mm/gmap.c
>>>> +++ b/arch/s390/mm/gmap.c
>>>> @@ -723,6 +723,9 @@ void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
>>>>  		vmaddr |= gaddr & ~PMD_MASK;
>>>>  		/* Find vma in the parent mm */
>>>>  		vma = find_vma(gmap->mm, vmaddr);
>>>> +		/* We do not discard pages that are backed by hugetlbfs */
>>>
>>> Can you add why we don't do that?
>>
>> It's in the patch description, but if you also need it here, sure.
> 
> "We currently don't allow that, so we don't have to refault it once it's
> needed again." why exactly can't we do that? Is it an artificial limitation?

diag44 doesn't make sense on areas that are not pmd aligned and sized
for huge guests.

Also I'm not sure if a hpage would go back to the VMs pool or the
general pool and both cases are not beneficial.

> 
>>
>>>
>>>> +		if (vma && is_vm_hugetlb_page(vma))
>>>> +			continue;
>>>>  		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
>>>>  		zap_page_range(vma, vmaddr, size);
>>>>  	}
>>>>
>>>
>>> Apart from that
>>>
>>> Reviewed-by: David Hildenbrand <david@redhat.com>
>>>
>>
>>
> 
>

Patch
diff mbox

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index af0a87eeede0..c691b9d9d223 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -723,6 +723,9 @@  void gmap_discard(struct gmap *gmap, unsigned long from, unsigned long to)
 		vmaddr |= gaddr & ~PMD_MASK;
 		/* Find vma in the parent mm */
 		vma = find_vma(gmap->mm, vmaddr);
+		/* We do not discard pages that are backed by hugetlbfs */
+		if (vma && is_vm_hugetlb_page(vma))
+			continue;
 		size = min(to - gaddr, PMD_SIZE - (gaddr & ~PMD_MASK));
 		zap_page_range(vma, vmaddr, size);
 	}