* [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-17 12:33 ` Thomas Huth
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail Claudio Imbrenda
` (12 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Bring x86 in line with the other architectures and include the generic header
at asm-generic/page.h .
This provides the macros PAGE_SHIFT, PAGE_SIZE, PAGE_MASK, virt_to_pfn, and
pfn_to_virt.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/x86/asm/page.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index 1359eb7..2cf8881 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -13,9 +13,7 @@
typedef unsigned long pteval_t;
typedef unsigned long pgd_t;
-#define PAGE_SHIFT 12
-#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
-#define PAGE_MASK (~(PAGE_SIZE-1))
+#include <asm-generic/page.h>
#ifndef __ASSEMBLY__
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header Claudio Imbrenda
@ 2020-12-17 12:33 ` Thomas Huth
0 siblings, 0 replies; 35+ messages in thread
From: Thomas Huth @ 2020-12-17 12:33 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, pbonzini, cohuck, lvivier, nadav.amit
On 16/12/2020 21.11, Claudio Imbrenda wrote:
> Bring x86 in line with the other architectures and include the generic header
> at asm-generic/page.h .
> This provides the macros PAGE_SHIFT, PAGE_SIZE, PAGE_MASK, virt_to_pfn, and
> pfn_to_virt.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/x86/asm/page.h | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
> index 1359eb7..2cf8881 100644
> --- a/lib/x86/asm/page.h
> +++ b/lib/x86/asm/page.h
> @@ -13,9 +13,7 @@
> typedef unsigned long pteval_t;
> typedef unsigned long pgd_t;
>
> -#define PAGE_SHIFT 12
> -#define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT)
> -#define PAGE_MASK (~(PAGE_SIZE-1))
> +#include <asm-generic/page.h>
>
> #ifndef __ASSEMBLY__
>
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-17 12:39 ` Thomas Huth
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements Claudio Imbrenda
` (11 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Add a list_add_tail wrapper function to allow adding elements to the end
of a list.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/list.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/lib/list.h b/lib/list.h
index 18d9516..7f9717e 100644
--- a/lib/list.h
+++ b/lib/list.h
@@ -50,4 +50,13 @@ static inline void list_add(struct linked_list *head, struct linked_list *li)
head->next = li;
}
+/*
+ * Add the given element before the given list head.
+ */
+static inline void list_add_tail(struct linked_list *head, struct linked_list *li)
+{
+ assert(head);
+ list_add(head->prev, li);
+}
+
#endif
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail Claudio Imbrenda
@ 2020-12-17 12:39 ` Thomas Huth
0 siblings, 0 replies; 35+ messages in thread
From: Thomas Huth @ 2020-12-17 12:39 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, pbonzini, cohuck, lvivier, nadav.amit
On 16/12/2020 21.11, Claudio Imbrenda wrote:
> Add a list_add_tail wrapper function to allow adding elements to the end
> of a list.
>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> lib/list.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/lib/list.h b/lib/list.h
> index 18d9516..7f9717e 100644
> --- a/lib/list.h
> +++ b/lib/list.h
> @@ -50,4 +50,13 @@ static inline void list_add(struct linked_list *head, struct linked_list *li)
> head->next = li;
> }
>
> +/*
> + * Add the given element before the given list head.
> + */
> +static inline void list_add_tail(struct linked_list *head, struct linked_list *li)
> +{
> + assert(head);
> + list_add(head->prev, li);
> +}
> +
> #endif
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 01/12] lib/x86: fix page.h to include the generic header Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 02/12] lib/list.h: add list_add_tail Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-24 18:16 ` Krish Sadhukhan
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas Claudio Imbrenda
` (10 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Add some asserts to make sure the state is consistent.
Simplify and improve the readability of vm_free.
Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling allocation metadata")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/vmalloc.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 986a34c..7a49adf 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment, size_t size)
static void vm_free(void *mem)
{
struct metadata *m;
- uintptr_t ptr, end;
+ uintptr_t ptr, page, i;
/* the pointer is not page-aligned, it was a single-page allocation */
if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
assert(GET_MAGIC(mem) == VM_MAGIC);
- ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
- free_page(phys_to_virt(ptr));
+ page = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
+ assert(page);
+ free_page(phys_to_virt(page));
return;
}
@@ -176,13 +177,14 @@ static void vm_free(void *mem)
m = GET_METADATA(mem);
assert(m->magic == VM_MAGIC);
assert(m->npages > 0);
+ assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));
/* free all the pages including the metadata page */
- ptr = (uintptr_t)mem - PAGE_SIZE;
- end = ptr + m->npages * PAGE_SIZE;
- for ( ; ptr < end; ptr += PAGE_SIZE)
- free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
- /* free the last one separately to avoid overflow issues */
- free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
+ ptr = (uintptr_t)m & PAGE_MASK;
+ for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
+ page = virt_to_pte_phys(page_root, (void *)ptr) & PAGE_MASK;
+ assert(page);
+ free_page(phys_to_virt(page));
+ }
}
static struct alloc_ops vmalloc_ops = {
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements Claudio Imbrenda
@ 2020-12-24 18:16 ` Krish Sadhukhan
2021-01-04 13:27 ` Claudio Imbrenda
0 siblings, 1 reply; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:16 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Add some asserts to make sure the state is consistent.
>
> Simplify and improve the readability of vm_free.
>
> Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling allocation metadata")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
> lib/vmalloc.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> index 986a34c..7a49adf 100644
> --- a/lib/vmalloc.c
> +++ b/lib/vmalloc.c
> @@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment, size_t size)
> static void vm_free(void *mem)
> {
> struct metadata *m;
> - uintptr_t ptr, end;
> + uintptr_t ptr, page, i;
>
> /* the pointer is not page-aligned, it was a single-page allocation */
Do we need an assert() for 'mem' if it is NULL for some reason ?
> if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
> assert(GET_MAGIC(mem) == VM_MAGIC);
> - ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> - free_page(phys_to_virt(ptr));
> + page = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> + assert(page);
> + free_page(phys_to_virt(page));
> return;
> }
>
> @@ -176,13 +177,14 @@ static void vm_free(void *mem)
> m = GET_METADATA(mem);
> assert(m->magic == VM_MAGIC);
> assert(m->npages > 0);
> + assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));
NIT: Combine the two assert()s for 'npages' perhaps ?
> /* free all the pages including the metadata page */
> - ptr = (uintptr_t)mem - PAGE_SIZE;
> - end = ptr + m->npages * PAGE_SIZE;
> - for ( ; ptr < end; ptr += PAGE_SIZE)
> - free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
> - /* free the last one separately to avoid overflow issues */
> - free_page(phys_to_virt(virt_to_pte_phys(page_root, (void *)ptr)));
> + ptr = (uintptr_t)m & PAGE_MASK;
> + for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
> + page = virt_to_pte_phys(page_root, (void *)ptr) & PAGE_MASK;
> + assert(page);
> + free_page(phys_to_virt(page));
> + }
> }
>
> static struct alloc_ops vmalloc_ops = {
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements
2020-12-24 18:16 ` Krish Sadhukhan
@ 2021-01-04 13:27 ` Claudio Imbrenda
0 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 13:27 UTC (permalink / raw)
To: Krish Sadhukhan
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Thu, 24 Dec 2020 10:16:45 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Add some asserts to make sure the state is consistent.
> >
> > Simplify and improve the readability of vm_free.
> >
> > Fixes: 3f6fee0d4da4 ("lib/vmalloc: vmalloc support for handling
> > allocation metadata")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> > lib/vmalloc.c | 20 +++++++++++---------
> > 1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> > index 986a34c..7a49adf 100644
> > --- a/lib/vmalloc.c
> > +++ b/lib/vmalloc.c
> > @@ -162,13 +162,14 @@ static void *vm_memalign(size_t alignment,
> > size_t size) static void vm_free(void *mem)
> > {
> > struct metadata *m;
> > - uintptr_t ptr, end;
> > + uintptr_t ptr, page, i;
> >
> > /* the pointer is not page-aligned, it was a single-page
> > allocation */
>
>
> Do we need an assert() for 'mem' if it is NULL for some reason ?
I thought the NULL case was handled in alloc.c, but I was mistaken.
In any case, the metadata will probably not match and trigger the other
asserts.
I will still add assert(mem), though, as it looks cleaner
> > if (!IS_ALIGNED((uintptr_t)mem, PAGE_SIZE)) {
> > assert(GET_MAGIC(mem) == VM_MAGIC);
> > - ptr = virt_to_pte_phys(page_root, mem) & PAGE_MASK;
> > - free_page(phys_to_virt(ptr));
> > + page = virt_to_pte_phys(page_root, mem) &
> > PAGE_MASK;
> > + assert(page);
> > + free_page(phys_to_virt(page));
> > return;
> > }
> >
> > @@ -176,13 +177,14 @@ static void vm_free(void *mem)
> > m = GET_METADATA(mem);
> > assert(m->magic == VM_MAGIC);
> > assert(m->npages > 0);
> > + assert(m->npages < BIT_ULL(BITS_PER_LONG - PAGE_SHIFT));
>
>
> NIT: Combine the two assert()s for 'npages' perhaps ?
no, when one assert is triggered, it will print a useful message; if
you combine the two asserts you don't know why exactly it failed.
it's functionally equivalent, but it has a better user experience.
> > /* free all the pages including the metadata page */
> > - ptr = (uintptr_t)mem - PAGE_SIZE;
> > - end = ptr + m->npages * PAGE_SIZE;
> > - for ( ; ptr < end; ptr += PAGE_SIZE)
> > - free_page(phys_to_virt(virt_to_pte_phys(page_root,
> > (void *)ptr)));
> > - /* free the last one separately to avoid overflow issues */
> > - free_page(phys_to_virt(virt_to_pte_phys(page_root, (void
> > *)ptr)));
> > + ptr = (uintptr_t)m & PAGE_MASK;
> > + for (i = 0 ; i < m->npages + 1; i++, ptr += PAGE_SIZE) {
> > + page = virt_to_pte_phys(page_root, (void *)ptr) &
> > PAGE_MASK;
> > + assert(page);
> > + free_page(phys_to_virt(page));
> > + }
> > }
> >
> > static struct alloc_ops vmalloc_ops = {
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (2 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 03/12] lib/vmalloc: add some asserts and improvements Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-24 18:17 ` Krish Sadhukhan
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator Claudio Imbrenda
` (9 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Fix the definitions of the memory areas.
Bring the headers in line with the rest of the asm headers, by having the
appropriate #ifdef _ASM$ARCH_ guarding the headers.
Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/asm-generic/memory_areas.h | 9 ++++-----
lib/arm/asm/memory_areas.h | 11 +++--------
lib/arm64/asm/memory_areas.h | 11 +++--------
lib/powerpc/asm/memory_areas.h | 11 +++--------
lib/ppc64/asm/memory_areas.h | 11 +++--------
lib/s390x/asm/memory_areas.h | 13 ++++++-------
lib/x86/asm/memory_areas.h | 27 ++++++++++++++++-----------
lib/alloc_page.h | 3 +++
lib/alloc_page.c | 4 +---
9 files changed, 42 insertions(+), 58 deletions(-)
diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
index 927baa7..3074afe 100644
--- a/lib/asm-generic/memory_areas.h
+++ b/lib/asm-generic/memory_areas.h
@@ -1,11 +1,10 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
+#define __ASM_GENERIC_MEMORY_AREAS_H__
#define AREA_NORMAL_PFN 0
#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 1
#endif
diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
index 927baa7..c723310 100644
--- a/lib/arm/asm/memory_areas.h
+++ b/lib/arm/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMARM_MEMORY_AREAS_H_
+#define _ASMARM_MEMORY_AREAS_H_
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
#endif
diff --git a/lib/arm64/asm/memory_areas.h b/lib/arm64/asm/memory_areas.h
index 927baa7..18e8ca8 100644
--- a/lib/arm64/asm/memory_areas.h
+++ b/lib/arm64/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMARM64_MEMORY_AREAS_H_
+#define _ASMARM64_MEMORY_AREAS_H_
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
#endif
diff --git a/lib/powerpc/asm/memory_areas.h b/lib/powerpc/asm/memory_areas.h
index 927baa7..76d1738 100644
--- a/lib/powerpc/asm/memory_areas.h
+++ b/lib/powerpc/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
+#define _ASMPOWERPC_MEMORY_AREAS_H_
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
#endif
diff --git a/lib/ppc64/asm/memory_areas.h b/lib/ppc64/asm/memory_areas.h
index 927baa7..b9fd46b 100644
--- a/lib/ppc64/asm/memory_areas.h
+++ b/lib/ppc64/asm/memory_areas.h
@@ -1,11 +1,6 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMPPC64_MEMORY_AREAS_H_
+#define _ASMPPC64_MEMORY_AREAS_H_
-#define AREA_NORMAL_PFN 0
-#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
-
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#include <asm-generic/memory_areas.h>
#endif
diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
index 4856a27..827bfb3 100644
--- a/lib/s390x/asm/memory_areas.h
+++ b/lib/s390x/asm/memory_areas.h
@@ -1,16 +1,15 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASMS390X_MEMORY_AREAS_H_
+#define _ASMS390X_MEMORY_AREAS_H_
-#define AREA_NORMAL_PFN BIT(31-12)
+#define AREA_NORMAL_PFN (1 << 19)
#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
#define AREA_LOW_PFN 0
#define AREA_LOW_NUMBER 1
-#define AREA_LOW 2
+#define AREA_LOW (1 << AREA_LOW_NUMBER)
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 2
#define AREA_DMA31 AREA_LOW
diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
index 952f5bd..e84016f 100644
--- a/lib/x86/asm/memory_areas.h
+++ b/lib/x86/asm/memory_areas.h
@@ -1,21 +1,26 @@
-#ifndef MEMORY_AREAS_H
-#define MEMORY_AREAS_H
+#ifndef _ASM_X86_MEMORY_AREAS_H_
+#define _ASM_X86_MEMORY_AREAS_H_
#define AREA_NORMAL_PFN BIT(36-12)
#define AREA_NORMAL_NUMBER 0
-#define AREA_NORMAL 1
+#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
-#define AREA_PAE_HIGH_PFN BIT(32-12)
-#define AREA_PAE_HIGH_NUMBER 1
-#define AREA_PAE_HIGH 2
+#define AREA_HIGH_PFN BIT(32-12)
+#define AREA_HIGH_NUMBER 1
+#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
-#define AREA_LOW_PFN 0
+#define AREA_LOW_PFN BIT(24-12)
#define AREA_LOW_NUMBER 2
-#define AREA_LOW 4
+#define AREA_LOW (1 << AREA_LOW_NUMBER)
-#define AREA_PAE (AREA_PAE | AREA_LOW)
+#define AREA_LOWEST_PFN 0
+#define AREA_LOWEST_NUMBER 3
+#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define MAX_AREAS 4
+
+#define AREA_DMA24 AREA_LOWEST
+#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
+#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
#endif
diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 816ff5d..b6aace5 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -10,6 +10,9 @@
#include <asm/memory_areas.h>
+#define AREA_ANY -1
+#define AREA_ANY_NUMBER 0xff
+
/* Returns true if the page allocator has been initialized */
bool page_alloc_initialized(void);
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 685ab1e..ed0ff02 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -19,8 +19,6 @@
#define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
-#define MAX_AREAS 6
-
#define ORDER_MASK 0x3f
#define ALLOC_MASK 0x40
#define SPECIAL_MASK 0x80
@@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
return;
}
#ifdef AREA_HIGH_PFN
- __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN), base_pfn, &top_pfn);
+ __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN, base_pfn, &top_pfn);
#endif
__page_alloc_init_area(AREA_NORMAL_NUMBER, AREA_NORMAL_PFN, base_pfn, &top_pfn);
#ifdef AREA_LOW_PFN
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas Claudio Imbrenda
@ 2020-12-24 18:17 ` Krish Sadhukhan
2021-01-04 13:19 ` Claudio Imbrenda
0 siblings, 1 reply; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:17 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Fix the definitions of the memory areas.
>
> Bring the headers in line with the rest of the asm headers, by having the
> appropriate #ifdef _ASM$ARCH_ guarding the headers.
Should we mention MAX_AREAS that the patch adds in each arch-specific
header ?
> Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
> lib/asm-generic/memory_areas.h | 9 ++++-----
> lib/arm/asm/memory_areas.h | 11 +++--------
> lib/arm64/asm/memory_areas.h | 11 +++--------
> lib/powerpc/asm/memory_areas.h | 11 +++--------
> lib/ppc64/asm/memory_areas.h | 11 +++--------
> lib/s390x/asm/memory_areas.h | 13 ++++++-------
> lib/x86/asm/memory_areas.h | 27 ++++++++++++++++-----------
> lib/alloc_page.h | 3 +++
> lib/alloc_page.c | 4 +---
> 9 files changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/lib/asm-generic/memory_areas.h b/lib/asm-generic/memory_areas.h
> index 927baa7..3074afe 100644
> --- a/lib/asm-generic/memory_areas.h
> +++ b/lib/asm-generic/memory_areas.h
> @@ -1,11 +1,10 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
> +#define __ASM_GENERIC_MEMORY_AREAS_H__
>
> #define AREA_NORMAL_PFN 0
> #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 1
>
> #endif
> diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> index 927baa7..c723310 100644
> --- a/lib/arm/asm/memory_areas.h
> +++ b/lib/arm/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMARM_MEMORY_AREAS_H_
> +#define _ASMARM_MEMORY_AREAS_H_
>
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>
> #endif
> diff --git a/lib/arm64/asm/memory_areas.h b/lib/arm64/asm/memory_areas.h
> index 927baa7..18e8ca8 100644
> --- a/lib/arm64/asm/memory_areas.h
> +++ b/lib/arm64/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMARM64_MEMORY_AREAS_H_
> +#define _ASMARM64_MEMORY_AREAS_H_
>
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>
> #endif
> diff --git a/lib/powerpc/asm/memory_areas.h b/lib/powerpc/asm/memory_areas.h
> index 927baa7..76d1738 100644
> --- a/lib/powerpc/asm/memory_areas.h
> +++ b/lib/powerpc/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
> +#define _ASMPOWERPC_MEMORY_AREAS_H_
>
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>
> #endif
> diff --git a/lib/ppc64/asm/memory_areas.h b/lib/ppc64/asm/memory_areas.h
> index 927baa7..b9fd46b 100644
> --- a/lib/ppc64/asm/memory_areas.h
> +++ b/lib/ppc64/asm/memory_areas.h
> @@ -1,11 +1,6 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMPPC64_MEMORY_AREAS_H_
> +#define _ASMPPC64_MEMORY_AREAS_H_
>
> -#define AREA_NORMAL_PFN 0
> -#define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> -
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#include <asm-generic/memory_areas.h>
>
> #endif
> diff --git a/lib/s390x/asm/memory_areas.h b/lib/s390x/asm/memory_areas.h
> index 4856a27..827bfb3 100644
> --- a/lib/s390x/asm/memory_areas.h
> +++ b/lib/s390x/asm/memory_areas.h
> @@ -1,16 +1,15 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASMS390X_MEMORY_AREAS_H_
> +#define _ASMS390X_MEMORY_AREAS_H_
>
> -#define AREA_NORMAL_PFN BIT(31-12)
> +#define AREA_NORMAL_PFN (1 << 19)
> #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>
> #define AREA_LOW_PFN 0
> #define AREA_LOW_NUMBER 1
> -#define AREA_LOW 2
> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 2
>
> #define AREA_DMA31 AREA_LOW
>
> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> index 952f5bd..e84016f 100644
> --- a/lib/x86/asm/memory_areas.h
> +++ b/lib/x86/asm/memory_areas.h
> @@ -1,21 +1,26 @@
> -#ifndef MEMORY_AREAS_H
> -#define MEMORY_AREAS_H
> +#ifndef _ASM_X86_MEMORY_AREAS_H_
> +#define _ASM_X86_MEMORY_AREAS_H_
>
> #define AREA_NORMAL_PFN BIT(36-12)
> #define AREA_NORMAL_NUMBER 0
> -#define AREA_NORMAL 1
> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>
> -#define AREA_PAE_HIGH_PFN BIT(32-12)
> -#define AREA_PAE_HIGH_NUMBER 1
> -#define AREA_PAE_HIGH 2
> +#define AREA_HIGH_PFN BIT(32-12)
> +#define AREA_HIGH_NUMBER 1
> +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
>
> -#define AREA_LOW_PFN 0
> +#define AREA_LOW_PFN BIT(24-12)
> #define AREA_LOW_NUMBER 2
> -#define AREA_LOW 4
> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>
> -#define AREA_PAE (AREA_PAE | AREA_LOW)
> +#define AREA_LOWEST_PFN 0
> +#define AREA_LOWEST_NUMBER 3
> +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
>
> -#define AREA_ANY -1
> -#define AREA_ANY_NUMBER 0xff
> +#define MAX_AREAS 4
> +
> +#define AREA_DMA24 AREA_LOWEST
> +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
> +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
>
> #endif
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 816ff5d..b6aace5 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -10,6 +10,9 @@
>
> #include <asm/memory_areas.h>
>
> +#define AREA_ANY -1
> +#define AREA_ANY_NUMBER 0xff
> +
> /* Returns true if the page allocator has been initialized */
> bool page_alloc_initialized(void);
>
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 685ab1e..ed0ff02 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -19,8 +19,6 @@
> #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>
> -#define MAX_AREAS 6
> -
> #define ORDER_MASK 0x3f
> #define ALLOC_MASK 0x40
> #define SPECIAL_MASK 0x80
> @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
> return;
> }
> #ifdef AREA_HIGH_PFN
> - __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN), base_pfn, &top_pfn);
> + __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN, base_pfn, &top_pfn);
Surprising that the compiler didn't complain !
> #endif
> __page_alloc_init_area(AREA_NORMAL_NUMBER, AREA_NORMAL_PFN, base_pfn, &top_pfn);
> #ifdef AREA_LOW_PFN
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas
2020-12-24 18:17 ` Krish Sadhukhan
@ 2021-01-04 13:19 ` Claudio Imbrenda
2021-01-05 1:17 ` Krish Sadhukhan
0 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 13:19 UTC (permalink / raw)
To: Krish Sadhukhan
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Thu, 24 Dec 2020 10:17:00 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Fix the definitions of the memory areas.
> >
> > Bring the headers in line with the rest of the asm headers, by
> > having the appropriate #ifdef _ASM$ARCH_ guarding the headers.
>
>
> Should we mention MAX_AREAS that the patch adds in each arch-specific
> header ?
not sure, it's a minor detail. I mentioned above that I "Fix the
definitions of the memory areas".
MAX_AREAS is not totally new, now it's per-arch instead of being in the
generic code.
> > Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> > lib/asm-generic/memory_areas.h | 9 ++++-----
> > lib/arm/asm/memory_areas.h | 11 +++--------
> > lib/arm64/asm/memory_areas.h | 11 +++--------
> > lib/powerpc/asm/memory_areas.h | 11 +++--------
> > lib/ppc64/asm/memory_areas.h | 11 +++--------
> > lib/s390x/asm/memory_areas.h | 13 ++++++-------
> > lib/x86/asm/memory_areas.h | 27 ++++++++++++++++-----------
> > lib/alloc_page.h | 3 +++
> > lib/alloc_page.c | 4 +---
> > 9 files changed, 42 insertions(+), 58 deletions(-)
> >
> > diff --git a/lib/asm-generic/memory_areas.h
> > b/lib/asm-generic/memory_areas.h index 927baa7..3074afe 100644
> > --- a/lib/asm-generic/memory_areas.h
> > +++ b/lib/asm-generic/memory_areas.h
> > @@ -1,11 +1,10 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
> > +#define __ASM_GENERIC_MEMORY_AREAS_H__
> >
> > #define AREA_NORMAL_PFN 0
> > #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 1
> >
> > #endif
> > diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
> > index 927baa7..c723310 100644
> > --- a/lib/arm/asm/memory_areas.h
> > +++ b/lib/arm/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMARM_MEMORY_AREAS_H_
> > +#define _ASMARM_MEMORY_AREAS_H_
> >
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >
> > #endif
> > diff --git a/lib/arm64/asm/memory_areas.h
> > b/lib/arm64/asm/memory_areas.h index 927baa7..18e8ca8 100644
> > --- a/lib/arm64/asm/memory_areas.h
> > +++ b/lib/arm64/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMARM64_MEMORY_AREAS_H_
> > +#define _ASMARM64_MEMORY_AREAS_H_
> >
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >
> > #endif
> > diff --git a/lib/powerpc/asm/memory_areas.h
> > b/lib/powerpc/asm/memory_areas.h index 927baa7..76d1738 100644
> > --- a/lib/powerpc/asm/memory_areas.h
> > +++ b/lib/powerpc/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
> > +#define _ASMPOWERPC_MEMORY_AREAS_H_
> >
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >
> > #endif
> > diff --git a/lib/ppc64/asm/memory_areas.h
> > b/lib/ppc64/asm/memory_areas.h index 927baa7..b9fd46b 100644
> > --- a/lib/ppc64/asm/memory_areas.h
> > +++ b/lib/ppc64/asm/memory_areas.h
> > @@ -1,11 +1,6 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMPPC64_MEMORY_AREAS_H_
> > +#define _ASMPPC64_MEMORY_AREAS_H_
> >
> > -#define AREA_NORMAL_PFN 0
> > -#define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > -
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#include <asm-generic/memory_areas.h>
> >
> > #endif
> > diff --git a/lib/s390x/asm/memory_areas.h
> > b/lib/s390x/asm/memory_areas.h index 4856a27..827bfb3 100644
> > --- a/lib/s390x/asm/memory_areas.h
> > +++ b/lib/s390x/asm/memory_areas.h
> > @@ -1,16 +1,15 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASMS390X_MEMORY_AREAS_H_
> > +#define _ASMS390X_MEMORY_AREAS_H_
> >
> > -#define AREA_NORMAL_PFN BIT(31-12)
> > +#define AREA_NORMAL_PFN (1 << 19)
> > #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >
> > #define AREA_LOW_PFN 0
> > #define AREA_LOW_NUMBER 1
> > -#define AREA_LOW 2
> > +#define AREA_LOW (1 << AREA_LOW_NUMBER)
> >
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 2
> >
> > #define AREA_DMA31 AREA_LOW
> >
> > diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
> > index 952f5bd..e84016f 100644
> > --- a/lib/x86/asm/memory_areas.h
> > +++ b/lib/x86/asm/memory_areas.h
> > @@ -1,21 +1,26 @@
> > -#ifndef MEMORY_AREAS_H
> > -#define MEMORY_AREAS_H
> > +#ifndef _ASM_X86_MEMORY_AREAS_H_
> > +#define _ASM_X86_MEMORY_AREAS_H_
> >
> > #define AREA_NORMAL_PFN BIT(36-12)
> > #define AREA_NORMAL_NUMBER 0
> > -#define AREA_NORMAL 1
> > +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
> >
> > -#define AREA_PAE_HIGH_PFN BIT(32-12)
> > -#define AREA_PAE_HIGH_NUMBER 1
> > -#define AREA_PAE_HIGH 2
> > +#define AREA_HIGH_PFN BIT(32-12)
> > +#define AREA_HIGH_NUMBER 1
> > +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
> >
> > -#define AREA_LOW_PFN 0
> > +#define AREA_LOW_PFN BIT(24-12)
> > #define AREA_LOW_NUMBER 2
> > -#define AREA_LOW 4
> > +#define AREA_LOW (1 << AREA_LOW_NUMBER)
> >
> > -#define AREA_PAE (AREA_PAE | AREA_LOW)
> > +#define AREA_LOWEST_PFN 0
> > +#define AREA_LOWEST_NUMBER 3
> > +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
> >
> > -#define AREA_ANY -1
> > -#define AREA_ANY_NUMBER 0xff
> > +#define MAX_AREAS 4
> > +
> > +#define AREA_DMA24 AREA_LOWEST
> > +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
> > +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
> >
> > #endif
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index 816ff5d..b6aace5 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -10,6 +10,9 @@
> >
> > #include <asm/memory_areas.h>
> >
> > +#define AREA_ANY -1
> > +#define AREA_ANY_NUMBER 0xff
> > +
> > /* Returns true if the page allocator has been initialized */
> > bool page_alloc_initialized(void);
> >
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 685ab1e..ed0ff02 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -19,8 +19,6 @@
> > #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> > #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
> >
> > -#define MAX_AREAS 6
> > -
> > #define ORDER_MASK 0x3f
> > #define ALLOC_MASK 0x40
> > #define SPECIAL_MASK 0x80
> > @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t
> > base_pfn, uintptr_t top_pfn) return;
> > }
> > #ifdef AREA_HIGH_PFN
> > - __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN),
> > base_pfn, &top_pfn);
> > + __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN,
> > base_pfn, &top_pfn);
>
>
> Surprising that the compiler didn't complain !
>
> > #endif
> > __page_alloc_init_area(AREA_NORMAL_NUMBER,
> > AREA_NORMAL_PFN, base_pfn, &top_pfn); #ifdef AREA_LOW_PFN
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas
2021-01-04 13:19 ` Claudio Imbrenda
@ 2021-01-05 1:17 ` Krish Sadhukhan
0 siblings, 0 replies; 35+ messages in thread
From: Krish Sadhukhan @ 2021-01-05 1:17 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 1/4/21 5:19 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:00 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> Fix the definitions of the memory areas.
>>>
>>> Bring the headers in line with the rest of the asm headers, by
>>> having the appropriate #ifdef _ASM$ARCH_ guarding the headers.
>>
>> Should we mention MAX_AREAS that the patch adds in each arch-specific
>> header ?
> not sure, it's a minor detail. I mentioned above that I "Fix the
> definitions of the memory areas".
>
> MAX_AREAS is not totally new, now it's per-arch instead of being in the
> generic code.
>
>>> Fixes: d74708246bd9 ("lib/asm: Add definitions of memory areas")
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>> lib/asm-generic/memory_areas.h | 9 ++++-----
>>> lib/arm/asm/memory_areas.h | 11 +++--------
>>> lib/arm64/asm/memory_areas.h | 11 +++--------
>>> lib/powerpc/asm/memory_areas.h | 11 +++--------
>>> lib/ppc64/asm/memory_areas.h | 11 +++--------
>>> lib/s390x/asm/memory_areas.h | 13 ++++++-------
>>> lib/x86/asm/memory_areas.h | 27 ++++++++++++++++-----------
>>> lib/alloc_page.h | 3 +++
>>> lib/alloc_page.c | 4 +---
>>> 9 files changed, 42 insertions(+), 58 deletions(-)
>>>
>>> diff --git a/lib/asm-generic/memory_areas.h
>>> b/lib/asm-generic/memory_areas.h index 927baa7..3074afe 100644
>>> --- a/lib/asm-generic/memory_areas.h
>>> +++ b/lib/asm-generic/memory_areas.h
>>> @@ -1,11 +1,10 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef __ASM_GENERIC_MEMORY_AREAS_H__
>>> +#define __ASM_GENERIC_MEMORY_AREAS_H__
>>>
>>> #define AREA_NORMAL_PFN 0
>>> #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 1
>>>
>>> #endif
>>> diff --git a/lib/arm/asm/memory_areas.h b/lib/arm/asm/memory_areas.h
>>> index 927baa7..c723310 100644
>>> --- a/lib/arm/asm/memory_areas.h
>>> +++ b/lib/arm/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMARM_MEMORY_AREAS_H_
>>> +#define _ASMARM_MEMORY_AREAS_H_
>>>
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>
>>> #endif
>>> diff --git a/lib/arm64/asm/memory_areas.h
>>> b/lib/arm64/asm/memory_areas.h index 927baa7..18e8ca8 100644
>>> --- a/lib/arm64/asm/memory_areas.h
>>> +++ b/lib/arm64/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMARM64_MEMORY_AREAS_H_
>>> +#define _ASMARM64_MEMORY_AREAS_H_
>>>
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>
>>> #endif
>>> diff --git a/lib/powerpc/asm/memory_areas.h
>>> b/lib/powerpc/asm/memory_areas.h index 927baa7..76d1738 100644
>>> --- a/lib/powerpc/asm/memory_areas.h
>>> +++ b/lib/powerpc/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMPOWERPC_MEMORY_AREAS_H_
>>> +#define _ASMPOWERPC_MEMORY_AREAS_H_
>>>
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>
>>> #endif
>>> diff --git a/lib/ppc64/asm/memory_areas.h
>>> b/lib/ppc64/asm/memory_areas.h index 927baa7..b9fd46b 100644
>>> --- a/lib/ppc64/asm/memory_areas.h
>>> +++ b/lib/ppc64/asm/memory_areas.h
>>> @@ -1,11 +1,6 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMPPC64_MEMORY_AREAS_H_
>>> +#define _ASMPPC64_MEMORY_AREAS_H_
>>>
>>> -#define AREA_NORMAL_PFN 0
>>> -#define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> -
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#include <asm-generic/memory_areas.h>
>>>
>>> #endif
>>> diff --git a/lib/s390x/asm/memory_areas.h
>>> b/lib/s390x/asm/memory_areas.h index 4856a27..827bfb3 100644
>>> --- a/lib/s390x/asm/memory_areas.h
>>> +++ b/lib/s390x/asm/memory_areas.h
>>> @@ -1,16 +1,15 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASMS390X_MEMORY_AREAS_H_
>>> +#define _ASMS390X_MEMORY_AREAS_H_
>>>
>>> -#define AREA_NORMAL_PFN BIT(31-12)
>>> +#define AREA_NORMAL_PFN (1 << 19)
>>> #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>
>>> #define AREA_LOW_PFN 0
>>> #define AREA_LOW_NUMBER 1
>>> -#define AREA_LOW 2
>>> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>>>
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 2
>>>
>>> #define AREA_DMA31 AREA_LOW
>>>
>>> diff --git a/lib/x86/asm/memory_areas.h b/lib/x86/asm/memory_areas.h
>>> index 952f5bd..e84016f 100644
>>> --- a/lib/x86/asm/memory_areas.h
>>> +++ b/lib/x86/asm/memory_areas.h
>>> @@ -1,21 +1,26 @@
>>> -#ifndef MEMORY_AREAS_H
>>> -#define MEMORY_AREAS_H
>>> +#ifndef _ASM_X86_MEMORY_AREAS_H_
>>> +#define _ASM_X86_MEMORY_AREAS_H_
>>>
>>> #define AREA_NORMAL_PFN BIT(36-12)
>>> #define AREA_NORMAL_NUMBER 0
>>> -#define AREA_NORMAL 1
>>> +#define AREA_NORMAL (1 << AREA_NORMAL_NUMBER)
>>>
>>> -#define AREA_PAE_HIGH_PFN BIT(32-12)
>>> -#define AREA_PAE_HIGH_NUMBER 1
>>> -#define AREA_PAE_HIGH 2
>>> +#define AREA_HIGH_PFN BIT(32-12)
>>> +#define AREA_HIGH_NUMBER 1
>>> +#define AREA_HIGH (1 << AREA_HIGH_NUMBER)
>>>
>>> -#define AREA_LOW_PFN 0
>>> +#define AREA_LOW_PFN BIT(24-12)
>>> #define AREA_LOW_NUMBER 2
>>> -#define AREA_LOW 4
>>> +#define AREA_LOW (1 << AREA_LOW_NUMBER)
>>>
>>> -#define AREA_PAE (AREA_PAE | AREA_LOW)
>>> +#define AREA_LOWEST_PFN 0
>>> +#define AREA_LOWEST_NUMBER 3
>>> +#define AREA_LOWEST (1 << AREA_LOWEST_NUMBER)
>>>
>>> -#define AREA_ANY -1
>>> -#define AREA_ANY_NUMBER 0xff
>>> +#define MAX_AREAS 4
>>> +
>>> +#define AREA_DMA24 AREA_LOWEST
>>> +#define AREA_DMA32 (AREA_LOWEST | AREA_LOW)
>>> +#define AREA_PAE36 (AREA_LOWEST | AREA_LOW | AREA_HIGH)
>>>
>>> #endif
>>> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
>>> index 816ff5d..b6aace5 100644
>>> --- a/lib/alloc_page.h
>>> +++ b/lib/alloc_page.h
>>> @@ -10,6 +10,9 @@
>>>
>>> #include <asm/memory_areas.h>
>>>
>>> +#define AREA_ANY -1
>>> +#define AREA_ANY_NUMBER 0xff
>>> +
>>> /* Returns true if the page allocator has been initialized */
>>> bool page_alloc_initialized(void);
>>>
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index 685ab1e..ed0ff02 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -19,8 +19,6 @@
>>> #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
>>> #define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>>>
>>> -#define MAX_AREAS 6
>>> -
>>> #define ORDER_MASK 0x3f
>>> #define ALLOC_MASK 0x40
>>> #define SPECIAL_MASK 0x80
>>> @@ -509,7 +507,7 @@ void page_alloc_init_area(u8 n, uintptr_t
>>> base_pfn, uintptr_t top_pfn) return;
>>> }
>>> #ifdef AREA_HIGH_PFN
>>> - __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN),
>>> base_pfn, &top_pfn);
>>> + __page_alloc_init_area(AREA_HIGH_NUMBER, AREA_HIGH_PFN,
>>> base_pfn, &top_pfn);
>>
>> Surprising that the compiler didn't complain !
>>
>>> #endif
>>> __page_alloc_init_area(AREA_NORMAL_NUMBER,
>>> AREA_NORMAL_PFN, base_pfn, &top_pfn); #ifdef AREA_LOW_PFN
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (3 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 04/12] lib/asm: Fix definitions of memory areas Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-24 18:17 ` Krish Sadhukhan
2020-12-28 19:34 ` Sean Christopherson
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops Claudio Imbrenda
` (8 subsequent siblings)
13 siblings, 2 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
This patch introduces some improvements to the code, mostly readability
improvements, but also some semantic details, and improvements in the
documentation.
* introduce and use pfn_t to semantically tag parameters as PFNs
* remove the PFN macro, use virt_to_pfn instead
* rename area_or_metadata_contains and area_contains to area_contains_pfn
and usable_area_contains_pfn respectively
* fix/improve comments in lib/alloc_page.h
* move some wrapper functions to the header
Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
Fixes: 34c950651861 ("lib/alloc_page: allow reserving arbitrary memory ranges")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.h | 49 +++++++++-----
lib/alloc_page.c | 165 +++++++++++++++++++++++------------------------
2 files changed, 116 insertions(+), 98 deletions(-)
diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index b6aace5..d8550c6 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -8,6 +8,7 @@
#ifndef ALLOC_PAGE_H
#define ALLOC_PAGE_H 1
+#include <stdbool.h>
#include <asm/memory_areas.h>
#define AREA_ANY -1
@@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
* top_pfn is the physical frame number of the first page immediately after
* the end of the area to initialize
*/
-void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn);
+void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn);
/* Enables the page allocator. At least one area must have been initialized */
void page_alloc_ops_enable(void);
@@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
/*
* Allocate aligned memory from any area.
- * Equivalent to memalign_pages_area(~0, alignment, size).
+ * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
*/
-void *memalign_pages(size_t alignment, size_t size);
+static inline void *memalign_pages(size_t alignment, size_t size)
+{
+ return memalign_pages_area(AREA_ANY, alignment, size);
+}
/*
* Allocate naturally aligned memory from the specified areas.
@@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t size);
void *alloc_pages_area(unsigned int areas, unsigned int order);
/*
- * Allocate one page from any area.
- * Equivalent to alloc_pages(0);
+ * Allocate naturally aligned memory from any area.
+ * Equivalent to alloc_pages_area(AREA_ANY, order);
*/
-void *alloc_page(void);
+static inline void *alloc_pages(unsigned int order)
+{
+ return alloc_pages_area(AREA_ANY, order);
+}
/*
- * Allocate naturally aligned memory from any area.
- * Equivalent to alloc_pages_area(~0, order);
+ * Allocate one page from any area.
+ * Equivalent to alloc_pages(0);
*/
-void *alloc_pages(unsigned int order);
+static inline void *alloc_page(void)
+{
+ return alloc_pages(0);
+}
/*
* Frees a memory block allocated with any of the memalign_pages* or
@@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
*/
void free_pages(void *mem);
-/* For backwards compatibility */
+/*
+ * Free one page.
+ * Equivalent to free_pages(mem).
+ */
static inline void free_page(void *mem)
{
return free_pages(mem);
}
-/* For backwards compatibility */
+/*
+ * Free pages by order.
+ * Equivalent to free_pages(mem).
+ */
static inline void free_pages_by_order(void *mem, unsigned int order)
{
free_pages(mem);
}
/*
- * Allocates and reserves the specified memory range if possible.
- * Returns NULL in case of failure.
+ * Allocates and reserves the specified physical memory range if possible.
+ * If the specified range cannot be reserved in its entirety, no action is
+ * performed and false is returned.
+ *
+ * Returns true in case of success, false otherwise.
*/
-void *alloc_pages_special(uintptr_t addr, size_t npages);
+bool alloc_pages_special(phys_addr_t addr, size_t npages);
/*
* Frees a reserved memory range that had been reserved with
@@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
* exactly, it can also be a subset, in which case only the specified
* pages will be freed and unreserved.
*/
-void free_pages_special(uintptr_t addr, size_t npages);
+void free_pages_special(phys_addr_t addr, size_t npages);
#endif
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index ed0ff02..8d2700d 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -17,25 +17,29 @@
#define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
#define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
-#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
#define ORDER_MASK 0x3f
#define ALLOC_MASK 0x40
#define SPECIAL_MASK 0x80
+typedef phys_addr_t pfn_t;
+
struct mem_area {
/* Physical frame number of the first usable frame in the area */
- uintptr_t base;
+ pfn_t base;
/* Physical frame number of the first frame outside the area */
- uintptr_t top;
- /* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
+ pfn_t top;
+ /* Per page metadata, each entry is a combination *_MASK and order */
u8 *page_states;
/* One freelist for each possible block size, up to NLISTS */
struct linked_list freelists[NLISTS];
};
+/* Descriptors for each possible area */
static struct mem_area areas[MAX_AREAS];
+/* Mask of initialized areas */
static unsigned int areas_mask;
+/* Protects areas and areas mask */
static struct spinlock lock;
bool page_alloc_initialized(void)
@@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
return areas_mask != 0;
}
-static inline bool area_or_metadata_contains(struct mem_area *a, uintptr_t pfn)
+/*
+ * Each memory area contains an array of metadata entries at the very
+ * beginning. The usable memory follows immediately afterwards.
+ * This function returns true if the given pfn falls anywhere within the
+ * memory area, including the metadata area.
+ */
+static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
{
- return (pfn >= PFN(a->page_states)) && (pfn < a->top);
+ return (pfn >= virt_to_pfn(a->page_states)) && (pfn < a->top);
}
-static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
+/*
+ * Each memory area contains an array of metadata entries at the very
+ * beginning. The usable memory follows immediately afterwards.
+ * This function returns true if the given pfn falls in the usable range of
+ * the given memory area.
+ */
+static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
{
return (pfn >= a->base) && (pfn < a->top);
}
@@ -69,21 +85,19 @@ static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
*/
static void split(struct mem_area *a, void *addr)
{
- uintptr_t pfn = PFN(addr);
- struct linked_list *p;
- uintptr_t i, idx;
+ pfn_t pfn = virt_to_pfn(addr);
+ pfn_t i, idx;
u8 order;
- assert(a && area_contains(a, pfn));
+ assert(a && usable_area_contains_pfn(a, pfn));
idx = pfn - a->base;
order = a->page_states[idx];
assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
assert(IS_ALIGNED_ORDER(pfn, order));
- assert(area_contains(a, pfn + BIT(order) - 1));
+ assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
/* Remove the block from its free list */
- p = list_remove(addr);
- assert(p);
+ list_remove(addr);
/* update the block size for each page in the block */
for (i = 0; i < BIT(order); i++) {
@@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
}
order--;
/* add the first half block to the appropriate free list */
- list_add(a->freelists + order, p);
+ list_add(a->freelists + order, addr);
/* add the second half block to the appropriate free list */
- list_add(a->freelists + order, (void *)((pfn + BIT(order)) * PAGE_SIZE));
+ list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
}
/*
@@ -105,7 +119,7 @@ static void split(struct mem_area *a, void *addr)
*/
static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
{
- struct linked_list *p, *res = NULL;
+ struct linked_list *p;
u8 order;
assert((al < NLISTS) && (sz < NLISTS));
@@ -130,17 +144,17 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
for (; order > sz; order--)
split(a, p);
- res = list_remove(p);
- memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK | order, BIT(order));
- return res;
+ list_remove(p);
+ memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
+ return p;
}
-static struct mem_area *get_area(uintptr_t pfn)
+static struct mem_area *get_area(pfn_t pfn)
{
uintptr_t i;
for (i = 0; i < MAX_AREAS; i++)
- if ((areas_mask & BIT(i)) && area_contains(areas + i, pfn))
+ if ((areas_mask & BIT(i)) && usable_area_contains_pfn(areas + i, pfn))
return areas + i;
return NULL;
}
@@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t pfn)
* - all of the pages of the two blocks must have the same block size
* - the function is called with the lock held
*/
-static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2)
+static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
{
- uintptr_t first, second, i;
- struct linked_list *li;
+ pfn_t first, second, i;
assert(IS_ALIGNED_ORDER(pfn, order) && IS_ALIGNED_ORDER(pfn2, order));
assert(pfn2 == pfn + BIT(order));
assert(a);
/* attempting to coalesce two blocks that belong to different areas */
- if (!area_contains(a, pfn) || !area_contains(a, pfn2 + BIT(order) - 1))
+ if (!usable_area_contains_pfn(a, pfn) || !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1))
return false;
first = pfn - a->base;
second = pfn2 - a->base;
@@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
return false;
/* we can coalesce, remove both blocks from their freelists */
- li = list_remove((void *)(pfn2 << PAGE_SHIFT));
- assert(li);
- li = list_remove((void *)(pfn << PAGE_SHIFT));
- assert(li);
+ list_remove(pfn_to_virt(pfn2));
+ list_remove(pfn_to_virt(pfn));
/* check the metadata entries and update with the new size */
for (i = 0; i < (2ull << order); i++) {
assert(a->page_states[first + i] == order);
a->page_states[first + i] = order + 1;
}
/* finally add the newly coalesced block to the appropriate freelist */
- list_add(a->freelists + order + 1, li);
+ list_add(a->freelists + order + 1, pfn_to_virt(pfn));
return true;
}
@@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
*/
static void _free_pages(void *mem)
{
- uintptr_t pfn2, pfn = PFN(mem);
+ pfn_t pfn2, pfn = virt_to_pfn(mem);
struct mem_area *a = NULL;
uintptr_t i, p;
u8 order;
@@ -232,7 +243,7 @@ static void _free_pages(void *mem)
/* ensure that the block is aligned properly for its size */
assert(IS_ALIGNED_ORDER(pfn, order));
/* ensure that the area can contain the whole block */
- assert(area_contains(a, pfn + BIT(order) - 1));
+ assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
for (i = 0; i < BIT(order); i++) {
/* check that all pages of the block have consistent metadata */
@@ -268,63 +279,68 @@ void free_pages(void *mem)
spin_unlock(&lock);
}
-static void *_alloc_page_special(uintptr_t addr)
+static bool _alloc_page_special(pfn_t pfn)
{
struct mem_area *a;
- uintptr_t mask, i;
+ pfn_t mask, i;
- a = get_area(PFN(addr));
- assert(a);
- i = PFN(addr) - a->base;
+ a = get_area(pfn);
+ if (!a)
+ return false;
+ i = pfn - a->base;
if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
- return NULL;
+ return false;
while (a->page_states[i]) {
- mask = GENMASK_ULL(63, PAGE_SHIFT + a->page_states[i]);
- split(a, (void *)(addr & mask));
+ mask = GENMASK_ULL(63, a->page_states[i]);
+ split(a, pfn_to_virt(pfn & mask));
}
a->page_states[i] = SPECIAL_MASK;
- return (void *)addr;
+ return true;
}
-static void _free_page_special(uintptr_t addr)
+static void _free_page_special(pfn_t pfn)
{
struct mem_area *a;
- uintptr_t i;
+ pfn_t i;
- a = get_area(PFN(addr));
+ a = get_area(pfn);
assert(a);
- i = PFN(addr) - a->base;
+ i = pfn - a->base;
assert(a->page_states[i] == SPECIAL_MASK);
a->page_states[i] = ALLOC_MASK;
- _free_pages((void *)addr);
+ _free_pages(pfn_to_virt(pfn));
}
-void *alloc_pages_special(uintptr_t addr, size_t n)
+bool alloc_pages_special(phys_addr_t addr, size_t n)
{
- uintptr_t i;
+ pfn_t pfn;
+ size_t i;
assert(IS_ALIGNED(addr, PAGE_SIZE));
+ pfn = addr >> PAGE_SHIFT;
spin_lock(&lock);
for (i = 0; i < n; i++)
- if (!_alloc_page_special(addr + i * PAGE_SIZE))
+ if (!_alloc_page_special(pfn + i))
break;
if (i < n) {
for (n = 0 ; n < i; n++)
- _free_page_special(addr + n * PAGE_SIZE);
- addr = 0;
+ _free_page_special(pfn + n);
+ n = 0;
}
spin_unlock(&lock);
- return (void *)addr;
+ return n;
}
-void free_pages_special(uintptr_t addr, size_t n)
+void free_pages_special(phys_addr_t addr, size_t n)
{
- uintptr_t i;
+ pfn_t pfn;
+ size_t i;
assert(IS_ALIGNED(addr, PAGE_SIZE));
+ pfn = addr >> PAGE_SHIFT;
spin_lock(&lock);
for (i = 0; i < n; i++)
- _free_page_special(addr + i * PAGE_SIZE);
+ _free_page_special(pfn + i);
spin_unlock(&lock);
}
@@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area, unsigned int order)
return page_memalign_order_area(area, order, order);
}
-void *alloc_pages(unsigned int order)
-{
- return alloc_pages_area(AREA_ANY, order);
-}
-
/*
* Allocates (1 << order) physically contiguous aligned pages.
* Returns NULL if the allocation was not possible.
@@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
return page_memalign_order_area(area, size, alignment);
}
-void *memalign_pages(size_t alignment, size_t size)
-{
- return memalign_pages_area(AREA_ANY, alignment, size);
-}
-
-/*
- * Allocates one page
- */
-void *alloc_page()
-{
- return alloc_pages(0);
-}
static struct alloc_ops page_alloc_ops = {
.memalign = memalign_pages,
@@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
* - the memory area to add does not overlap with existing areas
* - the memory area to add has at least 5 pages available
*/
-static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
+static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
{
size_t table_size, npages, i;
struct mem_area *a;
@@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
/* fill in the values of the new area */
a = areas + n;
- a->page_states = (void *)(start_pfn << PAGE_SHIFT);
+ a->page_states = pfn_to_virt(start_pfn);
a->base = start_pfn + table_size;
a->top = top_pfn;
npages = top_pfn - a->base;
@@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
for (i = 0; i < MAX_AREAS; i++) {
if (!(areas_mask & BIT(i)))
continue;
- assert(!area_or_metadata_contains(areas + i, start_pfn));
- assert(!area_or_metadata_contains(areas + i, top_pfn - 1));
- assert(!area_or_metadata_contains(a, PFN(areas[i].page_states)));
- assert(!area_or_metadata_contains(a, areas[i].top - 1));
+ assert(!area_contains_pfn(areas + i, start_pfn));
+ assert(!area_contains_pfn(areas + i, top_pfn - 1));
+ assert(!area_contains_pfn(a, virt_to_pfn(areas[i].page_states)));
+ assert(!area_contains_pfn(a, areas[i].top - 1));
}
/* initialize all freelists for the new area */
for (i = 0; i < NLISTS; i++)
- a->freelists[i].next = a->freelists[i].prev = a->freelists + i;
+ a->freelists[i].prev = a->freelists[i].next = a->freelists + i;
/* initialize the metadata for the available memory */
for (i = a->base; i < a->top; i += 1ull << order) {
@@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
assert(order < NLISTS);
/* initialize the metadata and add to the freelist */
memset(a->page_states + (i - a->base), order, BIT(order));
- list_add(a->freelists + order, (void *)(i << PAGE_SHIFT));
+ list_add(a->freelists + order, pfn_to_virt(i));
}
/* finally mark the area as present */
areas_mask |= BIT(n);
}
-static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, uintptr_t *top_pfn)
+static void __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t *top_pfn)
{
if (*top_pfn > cutoff) {
spin_lock(&lock);
@@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, u
* Prerequisites:
* see _page_alloc_init_area
*/
-void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
+void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn)
{
if (n != AREA_ANY_NUMBER) {
__page_alloc_init_area(n, 0, base_pfn, &top_pfn);
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator Claudio Imbrenda
@ 2020-12-24 18:17 ` Krish Sadhukhan
2021-01-04 13:11 ` Claudio Imbrenda
2020-12-28 19:34 ` Sean Christopherson
1 sibling, 1 reply; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:17 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> This patch introduces some improvements to the code, mostly readability
> improvements, but also some semantic details, and improvements in the
> documentation.
>
> * introduce and use pfn_t to semantically tag parameters as PFNs
> * remove the PFN macro, use virt_to_pfn instead
> * rename area_or_metadata_contains and area_contains to area_contains_pfn
> and usable_area_contains_pfn respectively
> * fix/improve comments in lib/alloc_page.h
> * move some wrapper functions to the header
>
> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
> Fixes: 34c950651861 ("lib/alloc_page: allow reserving arbitrary memory ranges")
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
> lib/alloc_page.h | 49 +++++++++-----
> lib/alloc_page.c | 165 +++++++++++++++++++++++------------------------
> 2 files changed, 116 insertions(+), 98 deletions(-)
>
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index b6aace5..d8550c6 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -8,6 +8,7 @@
> #ifndef ALLOC_PAGE_H
> #define ALLOC_PAGE_H 1
>
> +#include <stdbool.h>
> #include <asm/memory_areas.h>
>
> #define AREA_ANY -1
> @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
> * top_pfn is the physical frame number of the first page immediately after
> * the end of the area to initialize
> */
> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn);
> +void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn);
>
> /* Enables the page allocator. At least one area must have been initialized */
> void page_alloc_ops_enable(void);
> @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
>
> /*
> * Allocate aligned memory from any area.
> - * Equivalent to memalign_pages_area(~0, alignment, size).
> + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
> */
> -void *memalign_pages(size_t alignment, size_t size);
> +static inline void *memalign_pages(size_t alignment, size_t size)
> +{
> + return memalign_pages_area(AREA_ANY, alignment, size);
> +}
>
> /*
> * Allocate naturally aligned memory from the specified areas.
> @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t size);
> void *alloc_pages_area(unsigned int areas, unsigned int order);
>
> /*
> - * Allocate one page from any area.
> - * Equivalent to alloc_pages(0);
> + * Allocate naturally aligned memory from any area.
This one allocates page size memory and the comment should reflect that.
> + * Equivalent to alloc_pages_area(AREA_ANY, order);
> */
> -void *alloc_page(void);
> +static inline void *alloc_pages(unsigned int order)
> +{
> + return alloc_pages_area(AREA_ANY, order);
> +}
>
> /*
> - * Allocate naturally aligned memory from any area.
> - * Equivalent to alloc_pages_area(~0, order);
> + * Allocate one page from any area.
> + * Equivalent to alloc_pages(0);
> */
> -void *alloc_pages(unsigned int order);
> +static inline void *alloc_page(void)
> +{
> + return alloc_pages(0);
> +}
>
> /*
> * Frees a memory block allocated with any of the memalign_pages* or
> @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
> */
> void free_pages(void *mem);
>
> -/* For backwards compatibility */
> +/*
> + * Free one page.
> + * Equivalent to free_pages(mem).
> + */
> static inline void free_page(void *mem)
> {
> return free_pages(mem);
> }
>
> -/* For backwards compatibility */
> +/*
> + * Free pages by order.
> + * Equivalent to free_pages(mem).
> + */
> static inline void free_pages_by_order(void *mem, unsigned int order)
> {
> free_pages(mem);
> }
>
> /*
> - * Allocates and reserves the specified memory range if possible.
> - * Returns NULL in case of failure.
> + * Allocates and reserves the specified physical memory range if possible.
> + * If the specified range cannot be reserved in its entirety, no action is
> + * performed and false is returned.
> + *
> + * Returns true in case of success, false otherwise.
> */
> -void *alloc_pages_special(uintptr_t addr, size_t npages);
> +bool alloc_pages_special(phys_addr_t addr, size_t npages);
>
> /*
> * Frees a reserved memory range that had been reserved with
> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
> * exactly, it can also be a subset, in which case only the specified
> * pages will be freed and unreserved.
> */
> -void free_pages_special(uintptr_t addr, size_t npages);
> +void free_pages_special(phys_addr_t addr, size_t npages);
>
> #endif
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index ed0ff02..8d2700d 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -17,25 +17,29 @@
>
> #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
> #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>
> #define ORDER_MASK 0x3f
> #define ALLOC_MASK 0x40
> #define SPECIAL_MASK 0x80
>
> +typedef phys_addr_t pfn_t;
> +
> struct mem_area {
> /* Physical frame number of the first usable frame in the area */
> - uintptr_t base;
> + pfn_t base;
> /* Physical frame number of the first frame outside the area */
> - uintptr_t top;
> - /* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
> + pfn_t top;
> + /* Per page metadata, each entry is a combination *_MASK and order */
> u8 *page_states;
> /* One freelist for each possible block size, up to NLISTS */
> struct linked_list freelists[NLISTS];
> };
>
> +/* Descriptors for each possible area */
> static struct mem_area areas[MAX_AREAS];
> +/* Mask of initialized areas */
> static unsigned int areas_mask;
> +/* Protects areas and areas mask */
> static struct spinlock lock;
>
> bool page_alloc_initialized(void)
> @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
> return areas_mask != 0;
> }
>
> -static inline bool area_or_metadata_contains(struct mem_area *a, uintptr_t pfn)
> +/*
> + * Each memory area contains an array of metadata entries at the very
> + * beginning. The usable memory follows immediately afterwards.
> + * This function returns true if the given pfn falls anywhere within the
> + * memory area, including the metadata area.
> + */
> +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
> {
> - return (pfn >= PFN(a->page_states)) && (pfn < a->top);
> + return (pfn >= virt_to_pfn(a->page_states)) && (pfn < a->top);
> }
>
> -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
> +/*
> + * Each memory area contains an array of metadata entries at the very
> + * beginning. The usable memory follows immediately afterwards.
> + * This function returns true if the given pfn falls in the usable range of
> + * the given memory area.
> + */
> +static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
> {
> return (pfn >= a->base) && (pfn < a->top);
> }
> @@ -69,21 +85,19 @@ static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
> */
> static void split(struct mem_area *a, void *addr)
> {
> - uintptr_t pfn = PFN(addr);
> - struct linked_list *p;
> - uintptr_t i, idx;
> + pfn_t pfn = virt_to_pfn(addr);
> + pfn_t i, idx;
> u8 order;
>
> - assert(a && area_contains(a, pfn));
> + assert(a && usable_area_contains_pfn(a, pfn));
> idx = pfn - a->base;
> order = a->page_states[idx];
> assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
> assert(IS_ALIGNED_ORDER(pfn, order));
> - assert(area_contains(a, pfn + BIT(order) - 1));
> + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>
> /* Remove the block from its free list */
> - p = list_remove(addr);
> - assert(p);
> + list_remove(addr);
>
> /* update the block size for each page in the block */
> for (i = 0; i < BIT(order); i++) {
> @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
> }
> order--;
> /* add the first half block to the appropriate free list */
> - list_add(a->freelists + order, p);
> + list_add(a->freelists + order, addr);
> /* add the second half block to the appropriate free list */
> - list_add(a->freelists + order, (void *)((pfn + BIT(order)) * PAGE_SIZE));
> + list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
> }
>
> /*
> @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void *addr)
> */
> static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
> {
> - struct linked_list *p, *res = NULL;
> + struct linked_list *p;
> u8 order;
>
> assert((al < NLISTS) && (sz < NLISTS));
> @@ -130,17 +144,17 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
> for (; order > sz; order--)
> split(a, p);
>
> - res = list_remove(p);
> - memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK | order, BIT(order));
> - return res;
> + list_remove(p);
> + memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
> + return p;
> }
>
> -static struct mem_area *get_area(uintptr_t pfn)
> +static struct mem_area *get_area(pfn_t pfn)
> {
> uintptr_t i;
>
> for (i = 0; i < MAX_AREAS; i++)
> - if ((areas_mask & BIT(i)) && area_contains(areas + i, pfn))
> + if ((areas_mask & BIT(i)) && usable_area_contains_pfn(areas + i, pfn))
> return areas + i;
> return NULL;
> }
> @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t pfn)
> * - all of the pages of the two blocks must have the same block size
> * - the function is called with the lock held
> */
> -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2)
> +static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
> {
> - uintptr_t first, second, i;
> - struct linked_list *li;
> + pfn_t first, second, i;
>
> assert(IS_ALIGNED_ORDER(pfn, order) && IS_ALIGNED_ORDER(pfn2, order));
> assert(pfn2 == pfn + BIT(order));
> assert(a);
>
> /* attempting to coalesce two blocks that belong to different areas */
> - if (!area_contains(a, pfn) || !area_contains(a, pfn2 + BIT(order) - 1))
> + if (!usable_area_contains_pfn(a, pfn) || !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1))
> return false;
> first = pfn - a->base;
> second = pfn2 - a->base;
> @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
> return false;
>
> /* we can coalesce, remove both blocks from their freelists */
> - li = list_remove((void *)(pfn2 << PAGE_SHIFT));
> - assert(li);
> - li = list_remove((void *)(pfn << PAGE_SHIFT));
> - assert(li);
> + list_remove(pfn_to_virt(pfn2));
> + list_remove(pfn_to_virt(pfn));
> /* check the metadata entries and update with the new size */
> for (i = 0; i < (2ull << order); i++) {
> assert(a->page_states[first + i] == order);
> a->page_states[first + i] = order + 1;
> }
> /* finally add the newly coalesced block to the appropriate freelist */
> - list_add(a->freelists + order + 1, li);
> + list_add(a->freelists + order + 1, pfn_to_virt(pfn));
> return true;
> }
>
> @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn, uintptr_t pfn2
> */
> static void _free_pages(void *mem)
> {
> - uintptr_t pfn2, pfn = PFN(mem);
> + pfn_t pfn2, pfn = virt_to_pfn(mem);
> struct mem_area *a = NULL;
> uintptr_t i, p;
> u8 order;
> @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
> /* ensure that the block is aligned properly for its size */
> assert(IS_ALIGNED_ORDER(pfn, order));
> /* ensure that the area can contain the whole block */
> - assert(area_contains(a, pfn + BIT(order) - 1));
> + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>
> for (i = 0; i < BIT(order); i++) {
> /* check that all pages of the block have consistent metadata */
> @@ -268,63 +279,68 @@ void free_pages(void *mem)
> spin_unlock(&lock);
> }
>
> -static void *_alloc_page_special(uintptr_t addr)
> +static bool _alloc_page_special(pfn_t pfn)
> {
> struct mem_area *a;
> - uintptr_t mask, i;
> + pfn_t mask, i;
>
> - a = get_area(PFN(addr));
> - assert(a);
> - i = PFN(addr) - a->base;
> + a = get_area(pfn);
> + if (!a)
> + return false;
> + i = pfn - a->base;
> if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
> - return NULL;
> + return false;
> while (a->page_states[i]) {
> - mask = GENMASK_ULL(63, PAGE_SHIFT + a->page_states[i]);
> - split(a, (void *)(addr & mask));
> + mask = GENMASK_ULL(63, a->page_states[i]);
> + split(a, pfn_to_virt(pfn & mask));
> }
> a->page_states[i] = SPECIAL_MASK;
> - return (void *)addr;
> + return true;
> }
>
> -static void _free_page_special(uintptr_t addr)
> +static void _free_page_special(pfn_t pfn)
> {
> struct mem_area *a;
> - uintptr_t i;
> + pfn_t i;
>
> - a = get_area(PFN(addr));
> + a = get_area(pfn);
> assert(a);
> - i = PFN(addr) - a->base;
> + i = pfn - a->base;
> assert(a->page_states[i] == SPECIAL_MASK);
> a->page_states[i] = ALLOC_MASK;
> - _free_pages((void *)addr);
> + _free_pages(pfn_to_virt(pfn));
> }
>
> -void *alloc_pages_special(uintptr_t addr, size_t n)
> +bool alloc_pages_special(phys_addr_t addr, size_t n)
The convention for these alloc functions seems to be that of returning
void *. For example, alloc_pages_area(), alloc_pages() etc. Probably we
should maintain the convention or change all of their return type.
> {
> - uintptr_t i;
> + pfn_t pfn;
> + size_t i;
>
> assert(IS_ALIGNED(addr, PAGE_SIZE));
> + pfn = addr >> PAGE_SHIFT;
> spin_lock(&lock);
> for (i = 0; i < n; i++)
> - if (!_alloc_page_special(addr + i * PAGE_SIZE))
> + if (!_alloc_page_special(pfn + i))
Can the PFN macro be used here instead of the 'pfn' variable ?
> break;
> if (i < n) {
> for (n = 0 ; n < i; n++)
> - _free_page_special(addr + n * PAGE_SIZE);
> - addr = 0;
> + _free_page_special(pfn + n);
> + n = 0;
> }
> spin_unlock(&lock);
> - return (void *)addr;
> + return n;
> }
>
> -void free_pages_special(uintptr_t addr, size_t n)
> +void free_pages_special(phys_addr_t addr, size_t n)
> {
> - uintptr_t i;
> + pfn_t pfn;
> + size_t i;
>
> assert(IS_ALIGNED(addr, PAGE_SIZE));
> + pfn = addr >> PAGE_SHIFT;
> spin_lock(&lock);
> for (i = 0; i < n; i++)
> - _free_page_special(addr + i * PAGE_SIZE);
> + _free_page_special(pfn + i);
Can the PFN macro be used here instead of the 'pfn' variable ?
> spin_unlock(&lock);
> }
>
> @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area, unsigned int order)
> return page_memalign_order_area(area, order, order);
> }
>
> -void *alloc_pages(unsigned int order)
> -{
> - return alloc_pages_area(AREA_ANY, order);
> -}
> -
> /*
> * Allocates (1 << order) physically contiguous aligned pages.
> * Returns NULL if the allocation was not possible.
> @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
> return page_memalign_order_area(area, size, alignment);
> }
>
> -void *memalign_pages(size_t alignment, size_t size)
> -{
> - return memalign_pages_area(AREA_ANY, alignment, size);
> -}
> -
> -/*
> - * Allocates one page
> - */
> -void *alloc_page()
> -{
> - return alloc_pages(0);
> -}
>
> static struct alloc_ops page_alloc_ops = {
> .memalign = memalign_pages,
> @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
> * - the memory area to add does not overlap with existing areas
> * - the memory area to add has at least 5 pages available
> */
> -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
> +static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
> {
> size_t table_size, npages, i;
> struct mem_area *a;
> @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
>
> /* fill in the values of the new area */
> a = areas + n;
> - a->page_states = (void *)(start_pfn << PAGE_SHIFT);
> + a->page_states = pfn_to_virt(start_pfn);
> a->base = start_pfn + table_size;
> a->top = top_pfn;
> npages = top_pfn - a->base;
> @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
> for (i = 0; i < MAX_AREAS; i++) {
> if (!(areas_mask & BIT(i)))
> continue;
> - assert(!area_or_metadata_contains(areas + i, start_pfn));
> - assert(!area_or_metadata_contains(areas + i, top_pfn - 1));
> - assert(!area_or_metadata_contains(a, PFN(areas[i].page_states)));
> - assert(!area_or_metadata_contains(a, areas[i].top - 1));
> + assert(!area_contains_pfn(areas + i, start_pfn));
> + assert(!area_contains_pfn(areas + i, top_pfn - 1));
> + assert(!area_contains_pfn(a, virt_to_pfn(areas[i].page_states)));
> + assert(!area_contains_pfn(a, areas[i].top - 1));
> }
> /* initialize all freelists for the new area */
> for (i = 0; i < NLISTS; i++)
> - a->freelists[i].next = a->freelists[i].prev = a->freelists + i;
> + a->freelists[i].prev = a->freelists[i].next = a->freelists + i;
>
> /* initialize the metadata for the available memory */
> for (i = a->base; i < a->top; i += 1ull << order) {
> @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n, uintptr_t start_pfn, uintptr_t top_pfn)
> assert(order < NLISTS);
> /* initialize the metadata and add to the freelist */
> memset(a->page_states + (i - a->base), order, BIT(order));
> - list_add(a->freelists + order, (void *)(i << PAGE_SHIFT));
> + list_add(a->freelists + order, pfn_to_virt(i));
> }
> /* finally mark the area as present */
> areas_mask |= BIT(n);
> }
>
> -static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, uintptr_t *top_pfn)
> +static void __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t *top_pfn)
> {
> if (*top_pfn > cutoff) {
> spin_lock(&lock);
> @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n, uintptr_t cutoff, uintptr_t base_pfn, u
> * Prerequisites:
> * see _page_alloc_init_area
> */
> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t top_pfn)
> +void page_alloc_init_area(u8 n, phys_addr_t base_pfn, phys_addr_t top_pfn)
> {
> if (n != AREA_ANY_NUMBER) {
> __page_alloc_init_area(n, 0, base_pfn, &top_pfn);
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2020-12-24 18:17 ` Krish Sadhukhan
@ 2021-01-04 13:11 ` Claudio Imbrenda
2021-01-05 1:15 ` Krish Sadhukhan
0 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 13:11 UTC (permalink / raw)
To: Krish Sadhukhan
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Thu, 24 Dec 2020 10:17:06 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > This patch introduces some improvements to the code, mostly
> > readability improvements, but also some semantic details, and
> > improvements in the documentation.
> >
> > * introduce and use pfn_t to semantically tag parameters as PFNs
> > * remove the PFN macro, use virt_to_pfn instead
> > * rename area_or_metadata_contains and area_contains to
> > area_contains_pfn and usable_area_contains_pfn respectively
> > * fix/improve comments in lib/alloc_page.h
> > * move some wrapper functions to the header
> >
> > Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
> > allocator") Fixes: 34c950651861 ("lib/alloc_page: allow reserving
> > arbitrary memory ranges")
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> > lib/alloc_page.h | 49 +++++++++-----
> > lib/alloc_page.c | 165
> > +++++++++++++++++++++++------------------------ 2 files changed,
> > 116 insertions(+), 98 deletions(-)
> >
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index b6aace5..d8550c6 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -8,6 +8,7 @@
> > #ifndef ALLOC_PAGE_H
> > #define ALLOC_PAGE_H 1
> >
> > +#include <stdbool.h>
> > #include <asm/memory_areas.h>
> >
> > #define AREA_ANY -1
> > @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
> > * top_pfn is the physical frame number of the first page
> > immediately after
> > * the end of the area to initialize
> > */
> > -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
> > top_pfn); +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
> > phys_addr_t top_pfn);
> > /* Enables the page allocator. At least one area must have been
> > initialized */ void page_alloc_ops_enable(void);
> > @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas,
> > size_t alignment, size_t size);
> > /*
> > * Allocate aligned memory from any area.
> > - * Equivalent to memalign_pages_area(~0, alignment, size).
> > + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
> > */
> > -void *memalign_pages(size_t alignment, size_t size);
> > +static inline void *memalign_pages(size_t alignment, size_t size)
> > +{
> > + return memalign_pages_area(AREA_ANY, alignment, size);
> > +}
> >
> > /*
> > * Allocate naturally aligned memory from the specified areas.
> > @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t
> > size); void *alloc_pages_area(unsigned int areas, unsigned int
> > order);
> > /*
> > - * Allocate one page from any area.
> > - * Equivalent to alloc_pages(0);
> > + * Allocate naturally aligned memory from any area.
>
>
> This one allocates page size memory and the comment should reflect
> that.
I'll fix the comment
> > + * Equivalent to alloc_pages_area(AREA_ANY, order);
> > */
> > -void *alloc_page(void);
> > +static inline void *alloc_pages(unsigned int order)
> > +{
> > + return alloc_pages_area(AREA_ANY, order);
> > +}
> >
> > /*
> > - * Allocate naturally aligned memory from any area.
> > - * Equivalent to alloc_pages_area(~0, order);
> > + * Allocate one page from any area.
> > + * Equivalent to alloc_pages(0);
> > */
> > -void *alloc_pages(unsigned int order);
> > +static inline void *alloc_page(void)
> > +{
> > + return alloc_pages(0);
> > +}
> >
> > /*
> > * Frees a memory block allocated with any of the memalign_pages*
> > or @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
> > */
> > void free_pages(void *mem);
> >
> > -/* For backwards compatibility */
> > +/*
> > + * Free one page.
> > + * Equivalent to free_pages(mem).
> > + */
> > static inline void free_page(void *mem)
> > {
> > return free_pages(mem);
> > }
> >
> > -/* For backwards compatibility */
> > +/*
> > + * Free pages by order.
> > + * Equivalent to free_pages(mem).
> > + */
> > static inline void free_pages_by_order(void *mem, unsigned int
> > order) {
> > free_pages(mem);
> > }
> >
> > /*
> > - * Allocates and reserves the specified memory range if possible.
> > - * Returns NULL in case of failure.
> > + * Allocates and reserves the specified physical memory range if
> > possible.
> > + * If the specified range cannot be reserved in its entirety, no
> > action is
> > + * performed and false is returned.
> > + *
> > + * Returns true in case of success, false otherwise.
> > */
> > -void *alloc_pages_special(uintptr_t addr, size_t npages);
> > +bool alloc_pages_special(phys_addr_t addr, size_t npages);
> >
> > /*
> > * Frees a reserved memory range that had been reserved with
> > @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
> > npages);
> > * exactly, it can also be a subset, in which case only the
> > specified
> > * pages will be freed and unreserved.
> > */
> > -void free_pages_special(uintptr_t addr, size_t npages);
> > +void free_pages_special(phys_addr_t addr, size_t npages);
> >
> > #endif
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index ed0ff02..8d2700d 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -17,25 +17,29 @@
> >
> > #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
> > #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
> > -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
> >
> > #define ORDER_MASK 0x3f
> > #define ALLOC_MASK 0x40
> > #define SPECIAL_MASK 0x80
> >
> > +typedef phys_addr_t pfn_t;
> > +
> > struct mem_area {
> > /* Physical frame number of the first usable frame in the
> > area */
> > - uintptr_t base;
> > + pfn_t base;
> > /* Physical frame number of the first frame outside the
> > area */
> > - uintptr_t top;
> > - /* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
> > + pfn_t top;
> > + /* Per page metadata, each entry is a combination *_MASK
> > and order */ u8 *page_states;
> > /* One freelist for each possible block size, up to
> > NLISTS */ struct linked_list freelists[NLISTS];
> > };
> >
> > +/* Descriptors for each possible area */
> > static struct mem_area areas[MAX_AREAS];
> > +/* Mask of initialized areas */
> > static unsigned int areas_mask;
> > +/* Protects areas and areas mask */
> > static struct spinlock lock;
> >
> > bool page_alloc_initialized(void)
> > @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
> > return areas_mask != 0;
> > }
> >
> > -static inline bool area_or_metadata_contains(struct mem_area *a,
> > uintptr_t pfn) +/*
> > + * Each memory area contains an array of metadata entries at the
> > very
> > + * beginning. The usable memory follows immediately afterwards.
> > + * This function returns true if the given pfn falls anywhere
> > within the
> > + * memory area, including the metadata area.
> > + */
> > +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
> > {
> > - return (pfn >= PFN(a->page_states)) && (pfn < a->top);
> > + return (pfn >= virt_to_pfn(a->page_states)) && (pfn <
> > a->top); }
> >
> > -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
> > +/*
> > + * Each memory area contains an array of metadata entries at the
> > very
> > + * beginning. The usable memory follows immediately afterwards.
> > + * This function returns true if the given pfn falls in the usable
> > range of
> > + * the given memory area.
> > + */
> > +static inline bool usable_area_contains_pfn(struct mem_area *a,
> > pfn_t pfn) {
> > return (pfn >= a->base) && (pfn < a->top);
> > }
> > @@ -69,21 +85,19 @@ static inline bool area_contains(struct
> > mem_area *a, uintptr_t pfn) */
> > static void split(struct mem_area *a, void *addr)
> > {
> > - uintptr_t pfn = PFN(addr);
> > - struct linked_list *p;
> > - uintptr_t i, idx;
> > + pfn_t pfn = virt_to_pfn(addr);
> > + pfn_t i, idx;
> > u8 order;
> >
> > - assert(a && area_contains(a, pfn));
> > + assert(a && usable_area_contains_pfn(a, pfn));
> > idx = pfn - a->base;
> > order = a->page_states[idx];
> > assert(!(order & ~ORDER_MASK) && order && (order <
> > NLISTS)); assert(IS_ALIGNED_ORDER(pfn, order));
> > - assert(area_contains(a, pfn + BIT(order) - 1));
> > + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
> >
> > /* Remove the block from its free list */
> > - p = list_remove(addr);
> > - assert(p);
> > + list_remove(addr);
> >
> > /* update the block size for each page in the block */
> > for (i = 0; i < BIT(order); i++) {
> > @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
> > }
> > order--;
> > /* add the first half block to the appropriate free list
> > */
> > - list_add(a->freelists + order, p);
> > + list_add(a->freelists + order, addr);
> > /* add the second half block to the appropriate free list
> > */
> > - list_add(a->freelists + order, (void *)((pfn + BIT(order))
> > * PAGE_SIZE));
> > + list_add(a->freelists + order, pfn_to_virt(pfn +
> > BIT(order))); }
> >
> > /*
> > @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void
> > *addr) */
> > static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
> > {
> > - struct linked_list *p, *res = NULL;
> > + struct linked_list *p;
> > u8 order;
> >
> > assert((al < NLISTS) && (sz < NLISTS));
> > @@ -130,17 +144,17 @@ static void *page_memalign_order(struct
> > mem_area *a, u8 al, u8 sz) for (; order > sz; order--)
> > split(a, p);
> >
> > - res = list_remove(p);
> > - memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK |
> > order, BIT(order));
> > - return res;
> > + list_remove(p);
> > + memset(a->page_states + (virt_to_pfn(p) - a->base),
> > ALLOC_MASK | order, BIT(order));
> > + return p;
> > }
> >
> > -static struct mem_area *get_area(uintptr_t pfn)
> > +static struct mem_area *get_area(pfn_t pfn)
> > {
> > uintptr_t i;
> >
> > for (i = 0; i < MAX_AREAS; i++)
> > - if ((areas_mask & BIT(i)) && area_contains(areas +
> > i, pfn))
> > + if ((areas_mask & BIT(i)) &&
> > usable_area_contains_pfn(areas + i, pfn)) return areas + i;
> > return NULL;
> > }
> > @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t
> > pfn)
> > * - all of the pages of the two blocks must have the same block
> > size
> > * - the function is called with the lock held
> > */
> > -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn,
> > uintptr_t pfn2) +static bool coalesce(struct mem_area *a, u8 order,
> > pfn_t pfn, pfn_t pfn2) {
> > - uintptr_t first, second, i;
> > - struct linked_list *li;
> > + pfn_t first, second, i;
> >
> > assert(IS_ALIGNED_ORDER(pfn, order) &&
> > IS_ALIGNED_ORDER(pfn2, order)); assert(pfn2 == pfn + BIT(order));
> > assert(a);
> >
> > /* attempting to coalesce two blocks that belong to
> > different areas */
> > - if (!area_contains(a, pfn) || !area_contains(a, pfn2 +
> > BIT(order) - 1))
> > + if (!usable_area_contains_pfn(a, pfn) ||
> > !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1)) return false;
> > first = pfn - a->base;
> > second = pfn2 - a->base;
> > @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8
> > order, uintptr_t pfn, uintptr_t pfn2 return false;
> >
> > /* we can coalesce, remove both blocks from their
> > freelists */
> > - li = list_remove((void *)(pfn2 << PAGE_SHIFT));
> > - assert(li);
> > - li = list_remove((void *)(pfn << PAGE_SHIFT));
> > - assert(li);
> > + list_remove(pfn_to_virt(pfn2));
> > + list_remove(pfn_to_virt(pfn));
> > /* check the metadata entries and update with the new
> > size */ for (i = 0; i < (2ull << order); i++) {
> > assert(a->page_states[first + i] == order);
> > a->page_states[first + i] = order + 1;
> > }
> > /* finally add the newly coalesced block to the
> > appropriate freelist */
> > - list_add(a->freelists + order + 1, li);
> > + list_add(a->freelists + order + 1, pfn_to_virt(pfn));
> > return true;
> > }
> >
> > @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8
> > order, uintptr_t pfn, uintptr_t pfn2 */
> > static void _free_pages(void *mem)
> > {
> > - uintptr_t pfn2, pfn = PFN(mem);
> > + pfn_t pfn2, pfn = virt_to_pfn(mem);
> > struct mem_area *a = NULL;
> > uintptr_t i, p;
> > u8 order;
> > @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
> > /* ensure that the block is aligned properly for its size
> > */ assert(IS_ALIGNED_ORDER(pfn, order));
> > /* ensure that the area can contain the whole block */
> > - assert(area_contains(a, pfn + BIT(order) - 1));
> > + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
> >
> > for (i = 0; i < BIT(order); i++) {
> > /* check that all pages of the block have
> > consistent metadata */ @@ -268,63 +279,68 @@ void free_pages(void
> > *mem) spin_unlock(&lock);
> > }
> >
> > -static void *_alloc_page_special(uintptr_t addr)
> > +static bool _alloc_page_special(pfn_t pfn)
> > {
> > struct mem_area *a;
> > - uintptr_t mask, i;
> > + pfn_t mask, i;
> >
> > - a = get_area(PFN(addr));
> > - assert(a);
> > - i = PFN(addr) - a->base;
> > + a = get_area(pfn);
> > + if (!a)
> > + return false;
> > + i = pfn - a->base;
> > if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
> > - return NULL;
> > + return false;
> > while (a->page_states[i]) {
> > - mask = GENMASK_ULL(63, PAGE_SHIFT +
> > a->page_states[i]);
> > - split(a, (void *)(addr & mask));
> > + mask = GENMASK_ULL(63, a->page_states[i]);
> > + split(a, pfn_to_virt(pfn & mask));
> > }
> > a->page_states[i] = SPECIAL_MASK;
> > - return (void *)addr;
> > + return true;
> > }
> >
> > -static void _free_page_special(uintptr_t addr)
> > +static void _free_page_special(pfn_t pfn)
> > {
> > struct mem_area *a;
> > - uintptr_t i;
> > + pfn_t i;
> >
> > - a = get_area(PFN(addr));
> > + a = get_area(pfn);
> > assert(a);
> > - i = PFN(addr) - a->base;
> > + i = pfn - a->base;
> > assert(a->page_states[i] == SPECIAL_MASK);
> > a->page_states[i] = ALLOC_MASK;
> > - _free_pages((void *)addr);
> > + _free_pages(pfn_to_virt(pfn));
> > }
> >
> > -void *alloc_pages_special(uintptr_t addr, size_t n)
> > +bool alloc_pages_special(phys_addr_t addr, size_t n)
>
>
> The convention for these alloc functions seems to be that of
> returning void *. For example, alloc_pages_area(), alloc_pages() etc.
> Probably we should maintain the convention or change all of their
> return type.
what if you try to allocate memory that is not directly addressable?
(e.g. on x86_32)
you pass a phys_addr_t and it succeeds, but you can't get a pointer to
it, how should I indicate success/failure?
> > {
> > - uintptr_t i;
> > + pfn_t pfn;
> > + size_t i;
> >
> > assert(IS_ALIGNED(addr, PAGE_SIZE));
> > + pfn = addr >> PAGE_SHIFT;
> > spin_lock(&lock);
> > for (i = 0; i < n; i++)
> > - if (!_alloc_page_special(addr + i * PAGE_SIZE))
> > + if (!_alloc_page_special(pfn + i))
>
>
> Can the PFN macro be used here instead of the 'pfn' variable ?
I remove the PFN macro in this patch, also addr is not a virtual
address.
> > break;
> > if (i < n) {
> > for (n = 0 ; n < i; n++)
> > - _free_page_special(addr + n * PAGE_SIZE);
> > - addr = 0;
> > + _free_page_special(pfn + n);
> > + n = 0;
> > }
> > spin_unlock(&lock);
> > - return (void *)addr;
> > + return n;
> > }
> >
> > -void free_pages_special(uintptr_t addr, size_t n)
> > +void free_pages_special(phys_addr_t addr, size_t n)
> > {
> > - uintptr_t i;
> > + pfn_t pfn;
> > + size_t i;
> >
> > assert(IS_ALIGNED(addr, PAGE_SIZE));
> > + pfn = addr >> PAGE_SHIFT;
> > spin_lock(&lock);
> > for (i = 0; i < n; i++)
> > - _free_page_special(addr + i * PAGE_SIZE);
> > + _free_page_special(pfn + i);
>
>
> Can the PFN macro be used here instead of the 'pfn' variable ?
same as above
> > spin_unlock(&lock);
> > }
> >
> > @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area,
> > unsigned int order) return page_memalign_order_area(area, order,
> > order); }
> >
> > -void *alloc_pages(unsigned int order)
> > -{
> > - return alloc_pages_area(AREA_ANY, order);
> > -}
> > -
> > /*
> > * Allocates (1 << order) physically contiguous aligned pages.
> > * Returns NULL if the allocation was not possible.
> > @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area,
> > size_t alignment, size_t size) return
> > page_memalign_order_area(area, size, alignment); }
> >
> > -void *memalign_pages(size_t alignment, size_t size)
> > -{
> > - return memalign_pages_area(AREA_ANY, alignment, size);
> > -}
> > -
> > -/*
> > - * Allocates one page
> > - */
> > -void *alloc_page()
> > -{
> > - return alloc_pages(0);
> > -}
> >
> > static struct alloc_ops page_alloc_ops = {
> > .memalign = memalign_pages,
> > @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
> > * - the memory area to add does not overlap with existing areas
> > * - the memory area to add has at least 5 pages available
> > */
> > -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn,
> > uintptr_t top_pfn) +static void _page_alloc_init_area(u8 n, pfn_t
> > start_pfn, pfn_t top_pfn) {
> > size_t table_size, npages, i;
> > struct mem_area *a;
> > @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn)
> > /* fill in the values of the new area */
> > a = areas + n;
> > - a->page_states = (void *)(start_pfn << PAGE_SHIFT);
> > + a->page_states = pfn_to_virt(start_pfn);
> > a->base = start_pfn + table_size;
> > a->top = top_pfn;
> > npages = top_pfn - a->base;
> > @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn) for (i = 0; i < MAX_AREAS;
> > i++) { if (!(areas_mask & BIT(i)))
> > continue;
> > - assert(!area_or_metadata_contains(areas + i,
> > start_pfn));
> > - assert(!area_or_metadata_contains(areas + i,
> > top_pfn - 1));
> > - assert(!area_or_metadata_contains(a,
> > PFN(areas[i].page_states)));
> > - assert(!area_or_metadata_contains(a, areas[i].top
> > - 1));
> > + assert(!area_contains_pfn(areas + i, start_pfn));
> > + assert(!area_contains_pfn(areas + i, top_pfn - 1));
> > + assert(!area_contains_pfn(a,
> > virt_to_pfn(areas[i].page_states)));
> > + assert(!area_contains_pfn(a, areas[i].top - 1));
> > }
> > /* initialize all freelists for the new area */
> > for (i = 0; i < NLISTS; i++)
> > - a->freelists[i].next = a->freelists[i].prev =
> > a->freelists + i;
> > + a->freelists[i].prev = a->freelists[i].next =
> > a->freelists + i;
> > /* initialize the metadata for the available memory */
> > for (i = a->base; i < a->top; i += 1ull << order) {
> > @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n,
> > uintptr_t start_pfn, uintptr_t top_pfn) assert(order < NLISTS);
> > /* initialize the metadata and add to the
> > freelist */ memset(a->page_states + (i - a->base), order,
> > BIT(order));
> > - list_add(a->freelists + order, (void *)(i <<
> > PAGE_SHIFT));
> > + list_add(a->freelists + order, pfn_to_virt(i));
> > }
> > /* finally mark the area as present */
> > areas_mask |= BIT(n);
> > }
> >
> > -static void __page_alloc_init_area(u8 n, uintptr_t cutoff,
> > uintptr_t base_pfn, uintptr_t *top_pfn) +static void
> > __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t
> > *top_pfn) { if (*top_pfn > cutoff) {
> > spin_lock(&lock);
> > @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n,
> > uintptr_t cutoff, uintptr_t base_pfn, u
> > * Prerequisites:
> > * see _page_alloc_init_area
> > */
> > -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
> > top_pfn) +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
> > phys_addr_t top_pfn) {
> > if (n != AREA_ANY_NUMBER) {
> > __page_alloc_init_area(n, 0, base_pfn, &top_pfn);
> >
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2021-01-04 13:11 ` Claudio Imbrenda
@ 2021-01-05 1:15 ` Krish Sadhukhan
0 siblings, 0 replies; 35+ messages in thread
From: Krish Sadhukhan @ 2021-01-05 1:15 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 1/4/21 5:11 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:06 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> This patch introduces some improvements to the code, mostly
>>> readability improvements, but also some semantic details, and
>>> improvements in the documentation.
>>>
>>> * introduce and use pfn_t to semantically tag parameters as PFNs
>>> * remove the PFN macro, use virt_to_pfn instead
>>> * rename area_or_metadata_contains and area_contains to
>>> area_contains_pfn and usable_area_contains_pfn respectively
>>> * fix/improve comments in lib/alloc_page.h
>>> * move some wrapper functions to the header
>>>
>>> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
>>> allocator") Fixes: 34c950651861 ("lib/alloc_page: allow reserving
>>> arbitrary memory ranges")
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>> lib/alloc_page.h | 49 +++++++++-----
>>> lib/alloc_page.c | 165
>>> +++++++++++++++++++++++------------------------ 2 files changed,
>>> 116 insertions(+), 98 deletions(-)
>>>
>>> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
>>> index b6aace5..d8550c6 100644
>>> --- a/lib/alloc_page.h
>>> +++ b/lib/alloc_page.h
>>> @@ -8,6 +8,7 @@
>>> #ifndef ALLOC_PAGE_H
>>> #define ALLOC_PAGE_H 1
>>>
>>> +#include <stdbool.h>
>>> #include <asm/memory_areas.h>
>>>
>>> #define AREA_ANY -1
>>> @@ -23,7 +24,7 @@ bool page_alloc_initialized(void);
>>> * top_pfn is the physical frame number of the first page
>>> immediately after
>>> * the end of the area to initialize
>>> */
>>> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
>>> top_pfn); +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
>>> phys_addr_t top_pfn);
>>> /* Enables the page allocator. At least one area must have been
>>> initialized */ void page_alloc_ops_enable(void);
>>> @@ -37,9 +38,12 @@ void *memalign_pages_area(unsigned int areas,
>>> size_t alignment, size_t size);
>>> /*
>>> * Allocate aligned memory from any area.
>>> - * Equivalent to memalign_pages_area(~0, alignment, size).
>>> + * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
>>> */
>>> -void *memalign_pages(size_t alignment, size_t size);
>>> +static inline void *memalign_pages(size_t alignment, size_t size)
>>> +{
>>> + return memalign_pages_area(AREA_ANY, alignment, size);
>>> +}
>>>
>>> /*
>>> * Allocate naturally aligned memory from the specified areas.
>>> @@ -48,16 +52,22 @@ void *memalign_pages(size_t alignment, size_t
>>> size); void *alloc_pages_area(unsigned int areas, unsigned int
>>> order);
>>> /*
>>> - * Allocate one page from any area.
>>> - * Equivalent to alloc_pages(0);
>>> + * Allocate naturally aligned memory from any area.
>>
>> This one allocates page size memory and the comment should reflect
>> that.
> I'll fix the comment
>
>>> + * Equivalent to alloc_pages_area(AREA_ANY, order);
>>> */
>>> -void *alloc_page(void);
>>> +static inline void *alloc_pages(unsigned int order)
>>> +{
>>> + return alloc_pages_area(AREA_ANY, order);
>>> +}
>>>
>>> /*
>>> - * Allocate naturally aligned memory from any area.
>>> - * Equivalent to alloc_pages_area(~0, order);
>>> + * Allocate one page from any area.
>>> + * Equivalent to alloc_pages(0);
>>> */
>>> -void *alloc_pages(unsigned int order);
>>> +static inline void *alloc_page(void)
>>> +{
>>> + return alloc_pages(0);
>>> +}
>>>
>>> /*
>>> * Frees a memory block allocated with any of the memalign_pages*
>>> or @@ -66,23 +76,32 @@ void *alloc_pages(unsigned int order);
>>> */
>>> void free_pages(void *mem);
>>>
>>> -/* For backwards compatibility */
>>> +/*
>>> + * Free one page.
>>> + * Equivalent to free_pages(mem).
>>> + */
>>> static inline void free_page(void *mem)
>>> {
>>> return free_pages(mem);
>>> }
>>>
>>> -/* For backwards compatibility */
>>> +/*
>>> + * Free pages by order.
>>> + * Equivalent to free_pages(mem).
>>> + */
>>> static inline void free_pages_by_order(void *mem, unsigned int
>>> order) {
>>> free_pages(mem);
>>> }
>>>
>>> /*
>>> - * Allocates and reserves the specified memory range if possible.
>>> - * Returns NULL in case of failure.
>>> + * Allocates and reserves the specified physical memory range if
>>> possible.
>>> + * If the specified range cannot be reserved in its entirety, no
>>> action is
>>> + * performed and false is returned.
>>> + *
>>> + * Returns true in case of success, false otherwise.
>>> */
>>> -void *alloc_pages_special(uintptr_t addr, size_t npages);
>>> +bool alloc_pages_special(phys_addr_t addr, size_t npages);
>>>
>>> /*
>>> * Frees a reserved memory range that had been reserved with
>>> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
>>> npages);
>>> * exactly, it can also be a subset, in which case only the
>>> specified
>>> * pages will be freed and unreserved.
>>> */
>>> -void free_pages_special(uintptr_t addr, size_t npages);
>>> +void free_pages_special(phys_addr_t addr, size_t npages);
>>>
>>> #endif
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index ed0ff02..8d2700d 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -17,25 +17,29 @@
>>>
>>> #define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
>>> #define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
>>> -#define PFN(x) ((uintptr_t)(x) >> PAGE_SHIFT)
>>>
>>> #define ORDER_MASK 0x3f
>>> #define ALLOC_MASK 0x40
>>> #define SPECIAL_MASK 0x80
>>>
>>> +typedef phys_addr_t pfn_t;
>>> +
>>> struct mem_area {
>>> /* Physical frame number of the first usable frame in the
>>> area */
>>> - uintptr_t base;
>>> + pfn_t base;
>>> /* Physical frame number of the first frame outside the
>>> area */
>>> - uintptr_t top;
>>> - /* Combination of SPECIAL_MASK, ALLOC_MASK, and order */
>>> + pfn_t top;
>>> + /* Per page metadata, each entry is a combination *_MASK
>>> and order */ u8 *page_states;
>>> /* One freelist for each possible block size, up to
>>> NLISTS */ struct linked_list freelists[NLISTS];
>>> };
>>>
>>> +/* Descriptors for each possible area */
>>> static struct mem_area areas[MAX_AREAS];
>>> +/* Mask of initialized areas */
>>> static unsigned int areas_mask;
>>> +/* Protects areas and areas mask */
>>> static struct spinlock lock;
>>>
>>> bool page_alloc_initialized(void)
>>> @@ -43,12 +47,24 @@ bool page_alloc_initialized(void)
>>> return areas_mask != 0;
>>> }
>>>
>>> -static inline bool area_or_metadata_contains(struct mem_area *a,
>>> uintptr_t pfn) +/*
>>> + * Each memory area contains an array of metadata entries at the
>>> very
>>> + * beginning. The usable memory follows immediately afterwards.
>>> + * This function returns true if the given pfn falls anywhere
>>> within the
>>> + * memory area, including the metadata area.
>>> + */
>>> +static inline bool area_contains_pfn(struct mem_area *a, pfn_t pfn)
>>> {
>>> - return (pfn >= PFN(a->page_states)) && (pfn < a->top);
>>> + return (pfn >= virt_to_pfn(a->page_states)) && (pfn <
>>> a->top); }
>>>
>>> -static inline bool area_contains(struct mem_area *a, uintptr_t pfn)
>>> +/*
>>> + * Each memory area contains an array of metadata entries at the
>>> very
>>> + * beginning. The usable memory follows immediately afterwards.
>>> + * This function returns true if the given pfn falls in the usable
>>> range of
>>> + * the given memory area.
>>> + */
>>> +static inline bool usable_area_contains_pfn(struct mem_area *a,
>>> pfn_t pfn) {
>>> return (pfn >= a->base) && (pfn < a->top);
>>> }
>>> @@ -69,21 +85,19 @@ static inline bool area_contains(struct
>>> mem_area *a, uintptr_t pfn) */
>>> static void split(struct mem_area *a, void *addr)
>>> {
>>> - uintptr_t pfn = PFN(addr);
>>> - struct linked_list *p;
>>> - uintptr_t i, idx;
>>> + pfn_t pfn = virt_to_pfn(addr);
>>> + pfn_t i, idx;
>>> u8 order;
>>>
>>> - assert(a && area_contains(a, pfn));
>>> + assert(a && usable_area_contains_pfn(a, pfn));
>>> idx = pfn - a->base;
>>> order = a->page_states[idx];
>>> assert(!(order & ~ORDER_MASK) && order && (order <
>>> NLISTS)); assert(IS_ALIGNED_ORDER(pfn, order));
>>> - assert(area_contains(a, pfn + BIT(order) - 1));
>>> + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>>>
>>> /* Remove the block from its free list */
>>> - p = list_remove(addr);
>>> - assert(p);
>>> + list_remove(addr);
>>>
>>> /* update the block size for each page in the block */
>>> for (i = 0; i < BIT(order); i++) {
>>> @@ -92,9 +106,9 @@ static void split(struct mem_area *a, void *addr)
>>> }
>>> order--;
>>> /* add the first half block to the appropriate free list
>>> */
>>> - list_add(a->freelists + order, p);
>>> + list_add(a->freelists + order, addr);
>>> /* add the second half block to the appropriate free list
>>> */
>>> - list_add(a->freelists + order, (void *)((pfn + BIT(order))
>>> * PAGE_SIZE));
>>> + list_add(a->freelists + order, pfn_to_virt(pfn +
>>> BIT(order))); }
>>>
>>> /*
>>> @@ -105,7 +119,7 @@ static void split(struct mem_area *a, void
>>> *addr) */
>>> static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
>>> {
>>> - struct linked_list *p, *res = NULL;
>>> + struct linked_list *p;
>>> u8 order;
>>>
>>> assert((al < NLISTS) && (sz < NLISTS));
>>> @@ -130,17 +144,17 @@ static void *page_memalign_order(struct
>>> mem_area *a, u8 al, u8 sz) for (; order > sz; order--)
>>> split(a, p);
>>>
>>> - res = list_remove(p);
>>> - memset(a->page_states + (PFN(res) - a->base), ALLOC_MASK |
>>> order, BIT(order));
>>> - return res;
>>> + list_remove(p);
>>> + memset(a->page_states + (virt_to_pfn(p) - a->base),
>>> ALLOC_MASK | order, BIT(order));
>>> + return p;
>>> }
>>>
>>> -static struct mem_area *get_area(uintptr_t pfn)
>>> +static struct mem_area *get_area(pfn_t pfn)
>>> {
>>> uintptr_t i;
>>>
>>> for (i = 0; i < MAX_AREAS; i++)
>>> - if ((areas_mask & BIT(i)) && area_contains(areas +
>>> i, pfn))
>>> + if ((areas_mask & BIT(i)) &&
>>> usable_area_contains_pfn(areas + i, pfn)) return areas + i;
>>> return NULL;
>>> }
>>> @@ -160,17 +174,16 @@ static struct mem_area *get_area(uintptr_t
>>> pfn)
>>> * - all of the pages of the two blocks must have the same block
>>> size
>>> * - the function is called with the lock held
>>> */
>>> -static bool coalesce(struct mem_area *a, u8 order, uintptr_t pfn,
>>> uintptr_t pfn2) +static bool coalesce(struct mem_area *a, u8 order,
>>> pfn_t pfn, pfn_t pfn2) {
>>> - uintptr_t first, second, i;
>>> - struct linked_list *li;
>>> + pfn_t first, second, i;
>>>
>>> assert(IS_ALIGNED_ORDER(pfn, order) &&
>>> IS_ALIGNED_ORDER(pfn2, order)); assert(pfn2 == pfn + BIT(order));
>>> assert(a);
>>>
>>> /* attempting to coalesce two blocks that belong to
>>> different areas */
>>> - if (!area_contains(a, pfn) || !area_contains(a, pfn2 +
>>> BIT(order) - 1))
>>> + if (!usable_area_contains_pfn(a, pfn) ||
>>> !usable_area_contains_pfn(a, pfn2 + BIT(order) - 1)) return false;
>>> first = pfn - a->base;
>>> second = pfn2 - a->base;
>>> @@ -179,17 +192,15 @@ static bool coalesce(struct mem_area *a, u8
>>> order, uintptr_t pfn, uintptr_t pfn2 return false;
>>>
>>> /* we can coalesce, remove both blocks from their
>>> freelists */
>>> - li = list_remove((void *)(pfn2 << PAGE_SHIFT));
>>> - assert(li);
>>> - li = list_remove((void *)(pfn << PAGE_SHIFT));
>>> - assert(li);
>>> + list_remove(pfn_to_virt(pfn2));
>>> + list_remove(pfn_to_virt(pfn));
>>> /* check the metadata entries and update with the new
>>> size */ for (i = 0; i < (2ull << order); i++) {
>>> assert(a->page_states[first + i] == order);
>>> a->page_states[first + i] = order + 1;
>>> }
>>> /* finally add the newly coalesced block to the
>>> appropriate freelist */
>>> - list_add(a->freelists + order + 1, li);
>>> + list_add(a->freelists + order + 1, pfn_to_virt(pfn));
>>> return true;
>>> }
>>>
>>> @@ -209,7 +220,7 @@ static bool coalesce(struct mem_area *a, u8
>>> order, uintptr_t pfn, uintptr_t pfn2 */
>>> static void _free_pages(void *mem)
>>> {
>>> - uintptr_t pfn2, pfn = PFN(mem);
>>> + pfn_t pfn2, pfn = virt_to_pfn(mem);
>>> struct mem_area *a = NULL;
>>> uintptr_t i, p;
>>> u8 order;
>>> @@ -232,7 +243,7 @@ static void _free_pages(void *mem)
>>> /* ensure that the block is aligned properly for its size
>>> */ assert(IS_ALIGNED_ORDER(pfn, order));
>>> /* ensure that the area can contain the whole block */
>>> - assert(area_contains(a, pfn + BIT(order) - 1));
>>> + assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
>>>
>>> for (i = 0; i < BIT(order); i++) {
>>> /* check that all pages of the block have
>>> consistent metadata */ @@ -268,63 +279,68 @@ void free_pages(void
>>> *mem) spin_unlock(&lock);
>>> }
>>>
>>> -static void *_alloc_page_special(uintptr_t addr)
>>> +static bool _alloc_page_special(pfn_t pfn)
>>> {
>>> struct mem_area *a;
>>> - uintptr_t mask, i;
>>> + pfn_t mask, i;
>>>
>>> - a = get_area(PFN(addr));
>>> - assert(a);
>>> - i = PFN(addr) - a->base;
>>> + a = get_area(pfn);
>>> + if (!a)
>>> + return false;
>>> + i = pfn - a->base;
>>> if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
>>> - return NULL;
>>> + return false;
>>> while (a->page_states[i]) {
>>> - mask = GENMASK_ULL(63, PAGE_SHIFT +
>>> a->page_states[i]);
>>> - split(a, (void *)(addr & mask));
>>> + mask = GENMASK_ULL(63, a->page_states[i]);
>>> + split(a, pfn_to_virt(pfn & mask));
>>> }
>>> a->page_states[i] = SPECIAL_MASK;
>>> - return (void *)addr;
>>> + return true;
>>> }
>>>
>>> -static void _free_page_special(uintptr_t addr)
>>> +static void _free_page_special(pfn_t pfn)
>>> {
>>> struct mem_area *a;
>>> - uintptr_t i;
>>> + pfn_t i;
>>>
>>> - a = get_area(PFN(addr));
>>> + a = get_area(pfn);
>>> assert(a);
>>> - i = PFN(addr) - a->base;
>>> + i = pfn - a->base;
>>> assert(a->page_states[i] == SPECIAL_MASK);
>>> a->page_states[i] = ALLOC_MASK;
>>> - _free_pages((void *)addr);
>>> + _free_pages(pfn_to_virt(pfn));
>>> }
>>>
>>> -void *alloc_pages_special(uintptr_t addr, size_t n)
>>> +bool alloc_pages_special(phys_addr_t addr, size_t n)
>>
>> The convention for these alloc functions seems to be that of
>> returning void *. For example, alloc_pages_area(), alloc_pages() etc.
>> Probably we should maintain the convention or change all of their
>> return type.
> what if you try to allocate memory that is not directly addressable?
> (e.g. on x86_32)
>
> you pass a phys_addr_t and it succeeds, but you can't get a pointer to
> it, how should I indicate success/failure?
The function can perhaps return an error code via a parameter to
indicate why NULL was returned.
>
>>> {
>>> - uintptr_t i;
>>> + pfn_t pfn;
>>> + size_t i;
>>>
>>> assert(IS_ALIGNED(addr, PAGE_SIZE));
>>> + pfn = addr >> PAGE_SHIFT;
>>> spin_lock(&lock);
>>> for (i = 0; i < n; i++)
>>> - if (!_alloc_page_special(addr + i * PAGE_SIZE))
>>> + if (!_alloc_page_special(pfn + i))
>>
>> Can the PFN macro be used here instead of the 'pfn' variable ?
> I remove the PFN macro in this patch, also addr is not a virtual
> address.
>
>>> break;
>>> if (i < n) {
>>> for (n = 0 ; n < i; n++)
>>> - _free_page_special(addr + n * PAGE_SIZE);
>>> - addr = 0;
>>> + _free_page_special(pfn + n);
>>> + n = 0;
>>> }
>>> spin_unlock(&lock);
>>> - return (void *)addr;
>>> + return n;
>>> }
>>>
>>> -void free_pages_special(uintptr_t addr, size_t n)
>>> +void free_pages_special(phys_addr_t addr, size_t n)
>>> {
>>> - uintptr_t i;
>>> + pfn_t pfn;
>>> + size_t i;
>>>
>>> assert(IS_ALIGNED(addr, PAGE_SIZE));
>>> + pfn = addr >> PAGE_SHIFT;
>>> spin_lock(&lock);
>>> for (i = 0; i < n; i++)
>>> - _free_page_special(addr + i * PAGE_SIZE);
>>> + _free_page_special(pfn + i);
>>
>> Can the PFN macro be used here instead of the 'pfn' variable ?
> same as above
>
>>> spin_unlock(&lock);
>>> }
>>>
>>> @@ -351,11 +367,6 @@ void *alloc_pages_area(unsigned int area,
>>> unsigned int order) return page_memalign_order_area(area, order,
>>> order); }
>>>
>>> -void *alloc_pages(unsigned int order)
>>> -{
>>> - return alloc_pages_area(AREA_ANY, order);
>>> -}
>>> -
>>> /*
>>> * Allocates (1 << order) physically contiguous aligned pages.
>>> * Returns NULL if the allocation was not possible.
>>> @@ -370,18 +381,6 @@ void *memalign_pages_area(unsigned int area,
>>> size_t alignment, size_t size) return
>>> page_memalign_order_area(area, size, alignment); }
>>>
>>> -void *memalign_pages(size_t alignment, size_t size)
>>> -{
>>> - return memalign_pages_area(AREA_ANY, alignment, size);
>>> -}
>>> -
>>> -/*
>>> - * Allocates one page
>>> - */
>>> -void *alloc_page()
>>> -{
>>> - return alloc_pages(0);
>>> -}
>>>
>>> static struct alloc_ops page_alloc_ops = {
>>> .memalign = memalign_pages,
>>> @@ -416,7 +415,7 @@ void page_alloc_ops_enable(void)
>>> * - the memory area to add does not overlap with existing areas
>>> * - the memory area to add has at least 5 pages available
>>> */
>>> -static void _page_alloc_init_area(u8 n, uintptr_t start_pfn,
>>> uintptr_t top_pfn) +static void _page_alloc_init_area(u8 n, pfn_t
>>> start_pfn, pfn_t top_pfn) {
>>> size_t table_size, npages, i;
>>> struct mem_area *a;
>>> @@ -437,7 +436,7 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn)
>>> /* fill in the values of the new area */
>>> a = areas + n;
>>> - a->page_states = (void *)(start_pfn << PAGE_SHIFT);
>>> + a->page_states = pfn_to_virt(start_pfn);
>>> a->base = start_pfn + table_size;
>>> a->top = top_pfn;
>>> npages = top_pfn - a->base;
>>> @@ -447,14 +446,14 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn) for (i = 0; i < MAX_AREAS;
>>> i++) { if (!(areas_mask & BIT(i)))
>>> continue;
>>> - assert(!area_or_metadata_contains(areas + i,
>>> start_pfn));
>>> - assert(!area_or_metadata_contains(areas + i,
>>> top_pfn - 1));
>>> - assert(!area_or_metadata_contains(a,
>>> PFN(areas[i].page_states)));
>>> - assert(!area_or_metadata_contains(a, areas[i].top
>>> - 1));
>>> + assert(!area_contains_pfn(areas + i, start_pfn));
>>> + assert(!area_contains_pfn(areas + i, top_pfn - 1));
>>> + assert(!area_contains_pfn(a,
>>> virt_to_pfn(areas[i].page_states)));
>>> + assert(!area_contains_pfn(a, areas[i].top - 1));
>>> }
>>> /* initialize all freelists for the new area */
>>> for (i = 0; i < NLISTS; i++)
>>> - a->freelists[i].next = a->freelists[i].prev =
>>> a->freelists + i;
>>> + a->freelists[i].prev = a->freelists[i].next =
>>> a->freelists + i;
>>> /* initialize the metadata for the available memory */
>>> for (i = a->base; i < a->top; i += 1ull << order) {
>>> @@ -473,13 +472,13 @@ static void _page_alloc_init_area(u8 n,
>>> uintptr_t start_pfn, uintptr_t top_pfn) assert(order < NLISTS);
>>> /* initialize the metadata and add to the
>>> freelist */ memset(a->page_states + (i - a->base), order,
>>> BIT(order));
>>> - list_add(a->freelists + order, (void *)(i <<
>>> PAGE_SHIFT));
>>> + list_add(a->freelists + order, pfn_to_virt(i));
>>> }
>>> /* finally mark the area as present */
>>> areas_mask |= BIT(n);
>>> }
>>>
>>> -static void __page_alloc_init_area(u8 n, uintptr_t cutoff,
>>> uintptr_t base_pfn, uintptr_t *top_pfn) +static void
>>> __page_alloc_init_area(u8 n, pfn_t cutoff, pfn_t base_pfn, pfn_t
>>> *top_pfn) { if (*top_pfn > cutoff) {
>>> spin_lock(&lock);
>>> @@ -500,7 +499,7 @@ static void __page_alloc_init_area(u8 n,
>>> uintptr_t cutoff, uintptr_t base_pfn, u
>>> * Prerequisites:
>>> * see _page_alloc_init_area
>>> */
>>> -void page_alloc_init_area(u8 n, uintptr_t base_pfn, uintptr_t
>>> top_pfn) +void page_alloc_init_area(u8 n, phys_addr_t base_pfn,
>>> phys_addr_t top_pfn) {
>>> if (n != AREA_ANY_NUMBER) {
>>> __page_alloc_init_area(n, 0, base_pfn, &top_pfn);
>>>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator Claudio Imbrenda
2020-12-24 18:17 ` Krish Sadhukhan
@ 2020-12-28 19:34 ` Sean Christopherson
2021-01-04 17:23 ` Claudio Imbrenda
1 sibling, 1 reply; 35+ messages in thread
From: Sean Christopherson @ 2020-12-28 19:34 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Wed, Dec 16, 2020, Claudio Imbrenda wrote:
> /*
> - * Allocates and reserves the specified memory range if possible.
> - * Returns NULL in case of failure.
> + * Allocates and reserves the specified physical memory range if possible.
> + * If the specified range cannot be reserved in its entirety, no action is
> + * performed and false is returned.
> + *
> + * Returns true in case of success, false otherwise.
> */
> -void *alloc_pages_special(uintptr_t addr, size_t npages);
> +bool alloc_pages_special(phys_addr_t addr, size_t npages);
The boolean return is a bit awkward as kernel programmers will likely expect a
non-zero return to mean failure. But, since there are no users, can we simply
drop the entire *_pages_special() API? Allocating a specific PFN that isn't
MMIO seems doomed to fail anyways; I'm having a hard time envisioning a test
that would be able to use such an API without being horribly fragile.
>
> /*
> * Frees a reserved memory range that had been reserved with
> @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t npages);
> * exactly, it can also be a subset, in which case only the specified
> * pages will be freed and unreserved.
> */
> -void free_pages_special(uintptr_t addr, size_t npages);
> +void free_pages_special(phys_addr_t addr, size_t npages);
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator
2020-12-28 19:34 ` Sean Christopherson
@ 2021-01-04 17:23 ` Claudio Imbrenda
0 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 17:23 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Mon, 28 Dec 2020 11:34:38 -0800
Sean Christopherson <seanjc@google.com> wrote:
> On Wed, Dec 16, 2020, Claudio Imbrenda wrote:
> > /*
> > - * Allocates and reserves the specified memory range if possible.
> > - * Returns NULL in case of failure.
> > + * Allocates and reserves the specified physical memory range if
> > possible.
> > + * If the specified range cannot be reserved in its entirety, no
> > action is
> > + * performed and false is returned.
> > + *
> > + * Returns true in case of success, false otherwise.
> > */
> > -void *alloc_pages_special(uintptr_t addr, size_t npages);
> > +bool alloc_pages_special(phys_addr_t addr, size_t npages);
>
> The boolean return is a bit awkward as kernel programmers will likely
do you prefer int, with 0 for success and -1 for failure?
that's surely not a problem
> expect a non-zero return to mean failure. But, since there are no
> users, can we simply drop the entire *_pages_special() API?
> Allocating a specific PFN that isn't MMIO seems doomed to fail
> anyways; I'm having a hard time envisioning a test that would be able
> to use such an API without being horribly fragile.
I can. s390x can use this for some tests, where we need to allocate
memory at within or outside of specific areas, which might only be
known at run time (so we can't use the memory areas)
the alternative would be to allocate all the memory, take what is
needed, and then free the rest.... not very elegant
> >
> > /*
> > * Frees a reserved memory range that had been reserved with
> > @@ -91,6 +110,6 @@ void *alloc_pages_special(uintptr_t addr, size_t
> > npages);
> > * exactly, it can also be a subset, in which case only the
> > specified
> > * pages will be freed and unreserved.
> > */
> > -void free_pages_special(uintptr_t addr, size_t npages);
> > +void free_pages_special(phys_addr_t addr, size_t npages);
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (4 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 05/12] lib/alloc_page: fix and improve the page allocator Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-24 18:17 ` Krish Sadhukhan
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 07/12] lib/alloc_page: Optimization to skip known empty freelists Claudio Imbrenda
` (7 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Remove align_min from struct alloc_ops, since it is no longer used.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc.h | 1 -
lib/alloc_page.c | 1 -
lib/alloc_phys.c | 9 +++++----
lib/vmalloc.c | 1 -
4 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/lib/alloc.h b/lib/alloc.h
index 9b4b634..db90b01 100644
--- a/lib/alloc.h
+++ b/lib/alloc.h
@@ -25,7 +25,6 @@
struct alloc_ops {
void *(*memalign)(size_t alignment, size_t size);
void (*free)(void *ptr);
- size_t align_min;
};
extern struct alloc_ops *alloc_ops;
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 8d2700d..b1cdf21 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
static struct alloc_ops page_alloc_ops = {
.memalign = memalign_pages,
.free = free_pages,
- .align_min = PAGE_SIZE,
};
/*
diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
index 72e20f7..a4d2bf2 100644
--- a/lib/alloc_phys.c
+++ b/lib/alloc_phys.c
@@ -29,8 +29,8 @@ static phys_addr_t base, top;
static void *early_memalign(size_t alignment, size_t size);
static struct alloc_ops early_alloc_ops = {
.memalign = early_memalign,
- .align_min = DEFAULT_MINIMUM_ALIGNMENT
};
+static size_t align_min;
struct alloc_ops *alloc_ops = &early_alloc_ops;
@@ -39,8 +39,7 @@ void phys_alloc_show(void)
int i;
spin_lock(&lock);
- printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
- (u64)early_alloc_ops.align_min);
+ printf("phys_alloc minimum alignment: %#" PRIx64 "\n", (u64)align_min);
for (i = 0; i < nr_regions; ++i)
printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
(u64)regions[i].base,
@@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t align)
{
assert(align && !(align & (align - 1)));
spin_lock(&lock);
- early_alloc_ops.align_min = align;
+ align_min = align;
spin_unlock(&lock);
}
@@ -83,6 +82,8 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
top_safe = MIN(top_safe, 1ULL << 32);
assert(base < top_safe);
+ if (align < align_min)
+ align = align_min;
addr = ALIGN(base, align);
size += addr - base;
diff --git a/lib/vmalloc.c b/lib/vmalloc.c
index 7a49adf..e146162 100644
--- a/lib/vmalloc.c
+++ b/lib/vmalloc.c
@@ -190,7 +190,6 @@ static void vm_free(void *mem)
static struct alloc_ops vmalloc_ops = {
.memalign = vm_memalign,
.free = vm_free,
- .align_min = PAGE_SIZE,
};
void __attribute__((__weak__)) find_highmem(void)
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops Claudio Imbrenda
@ 2020-12-24 18:17 ` Krish Sadhukhan
2021-01-04 13:05 ` Claudio Imbrenda
0 siblings, 1 reply; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:17 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> Remove align_min from struct alloc_ops, since it is no longer used.
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
> lib/alloc.h | 1 -
> lib/alloc_page.c | 1 -
> lib/alloc_phys.c | 9 +++++----
> lib/vmalloc.c | 1 -
> 4 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/lib/alloc.h b/lib/alloc.h
> index 9b4b634..db90b01 100644
> --- a/lib/alloc.h
> +++ b/lib/alloc.h
> @@ -25,7 +25,6 @@
> struct alloc_ops {
> void *(*memalign)(size_t alignment, size_t size);
> void (*free)(void *ptr);
> - size_t align_min;
> };
>
> extern struct alloc_ops *alloc_ops;
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 8d2700d..b1cdf21 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
> static struct alloc_ops page_alloc_ops = {
> .memalign = memalign_pages,
> .free = free_pages,
> - .align_min = PAGE_SIZE,
> };
>
> /*
> diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
> index 72e20f7..a4d2bf2 100644
> --- a/lib/alloc_phys.c
> +++ b/lib/alloc_phys.c
> @@ -29,8 +29,8 @@ static phys_addr_t base, top;
> static void *early_memalign(size_t alignment, size_t size);
> static struct alloc_ops early_alloc_ops = {
> .memalign = early_memalign,
> - .align_min = DEFAULT_MINIMUM_ALIGNMENT
> };
> +static size_t align_min;
I don't see any caller of phys_alloc_set_minimum_alignment(). So when
you are comparing against this variable in phys_alloc_aligned_safe()
below, you are comparing against zero. Is that what you is intended or
should 'align_min' be set some default ?
>
> struct alloc_ops *alloc_ops = &early_alloc_ops;
>
> @@ -39,8 +39,7 @@ void phys_alloc_show(void)
> int i;
>
> spin_lock(&lock);
> - printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> - (u64)early_alloc_ops.align_min);
> + printf("phys_alloc minimum alignment: %#" PRIx64 "\n", (u64)align_min);
> for (i = 0; i < nr_regions; ++i)
> printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
> (u64)regions[i].base,
> @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t align)
> {
> assert(align && !(align & (align - 1)));
> spin_lock(&lock);
> - early_alloc_ops.align_min = align;
> + align_min = align;
> spin_unlock(&lock);
> }
>
> @@ -83,6 +82,8 @@ static phys_addr_t phys_alloc_aligned_safe(phys_addr_t size,
> top_safe = MIN(top_safe, 1ULL << 32);
>
> assert(base < top_safe);
> + if (align < align_min)
> + align = align_min;
>
> addr = ALIGN(base, align);
> size += addr - base;
> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> index 7a49adf..e146162 100644
> --- a/lib/vmalloc.c
> +++ b/lib/vmalloc.c
> @@ -190,7 +190,6 @@ static void vm_free(void *mem)
> static struct alloc_ops vmalloc_ops = {
> .memalign = vm_memalign,
> .free = vm_free,
> - .align_min = PAGE_SIZE,
> };
>
> void __attribute__((__weak__)) find_highmem(void)
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops
2020-12-24 18:17 ` Krish Sadhukhan
@ 2021-01-04 13:05 ` Claudio Imbrenda
2021-01-05 0:39 ` Krish Sadhukhan
0 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 13:05 UTC (permalink / raw)
To: Krish Sadhukhan
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Thu, 24 Dec 2020 10:17:20 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> > Remove align_min from struct alloc_ops, since it is no longer used.
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> > lib/alloc.h | 1 -
> > lib/alloc_page.c | 1 -
> > lib/alloc_phys.c | 9 +++++----
> > lib/vmalloc.c | 1 -
> > 4 files changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/alloc.h b/lib/alloc.h
> > index 9b4b634..db90b01 100644
> > --- a/lib/alloc.h
> > +++ b/lib/alloc.h
> > @@ -25,7 +25,6 @@
> > struct alloc_ops {
> > void *(*memalign)(size_t alignment, size_t size);
> > void (*free)(void *ptr);
> > - size_t align_min;
> > };
> >
> > extern struct alloc_ops *alloc_ops;
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 8d2700d..b1cdf21 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area,
> > size_t alignment, size_t size) static struct alloc_ops
> > page_alloc_ops = { .memalign = memalign_pages,
> > .free = free_pages,
> > - .align_min = PAGE_SIZE,
> > };
> >
> > /*
> > diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
> > index 72e20f7..a4d2bf2 100644
> > --- a/lib/alloc_phys.c
> > +++ b/lib/alloc_phys.c
> > @@ -29,8 +29,8 @@ static phys_addr_t base, top;
> > static void *early_memalign(size_t alignment, size_t size);
> > static struct alloc_ops early_alloc_ops = {
> > .memalign = early_memalign,
> > - .align_min = DEFAULT_MINIMUM_ALIGNMENT
> > };
> > +static size_t align_min;
>
>
> I don't see any caller of phys_alloc_set_minimum_alignment(). So when
lib/arm/setup.c:150
lib/powerpc/setup.c:150
> you are comparing against this variable in phys_alloc_aligned_safe()
> below, you are comparing against zero. Is that what you is intended
> or should 'align_min' be set some default ?
if the architecture specific code did not specify anything better, 0 is
ok.
> >
> > struct alloc_ops *alloc_ops = &early_alloc_ops;
> >
> > @@ -39,8 +39,7 @@ void phys_alloc_show(void)
> > int i;
> >
> > spin_lock(&lock);
> > - printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> > - (u64)early_alloc_ops.align_min);
> > + printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
> > (u64)align_min); for (i = 0; i < nr_regions; ++i)
> > printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
> > (u64)regions[i].base,
> > @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t
> > align) {
> > assert(align && !(align & (align - 1)));
> > spin_lock(&lock);
> > - early_alloc_ops.align_min = align;
> > + align_min = align;
> > spin_unlock(&lock);
> > }
> >
> > @@ -83,6 +82,8 @@ static phys_addr_t
> > phys_alloc_aligned_safe(phys_addr_t size, top_safe = MIN(top_safe,
> > 1ULL << 32);
> > assert(base < top_safe);
> > + if (align < align_min)
> > + align = align_min;
> >
> > addr = ALIGN(base, align);
> > size += addr - base;
> > diff --git a/lib/vmalloc.c b/lib/vmalloc.c
> > index 7a49adf..e146162 100644
> > --- a/lib/vmalloc.c
> > +++ b/lib/vmalloc.c
> > @@ -190,7 +190,6 @@ static void vm_free(void *mem)
> > static struct alloc_ops vmalloc_ops = {
> > .memalign = vm_memalign,
> > .free = vm_free,
> > - .align_min = PAGE_SIZE,
> > };
> >
> > void __attribute__((__weak__)) find_highmem(void)
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops
2021-01-04 13:05 ` Claudio Imbrenda
@ 2021-01-05 0:39 ` Krish Sadhukhan
0 siblings, 0 replies; 35+ messages in thread
From: Krish Sadhukhan @ 2021-01-05 0:39 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 1/4/21 5:05 AM, Claudio Imbrenda wrote:
> On Thu, 24 Dec 2020 10:17:20 -0800
> Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>
>> On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
>>> Remove align_min from struct alloc_ops, since it is no longer used.
>>>
>>> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
>>> ---
>>> lib/alloc.h | 1 -
>>> lib/alloc_page.c | 1 -
>>> lib/alloc_phys.c | 9 +++++----
>>> lib/vmalloc.c | 1 -
>>> 4 files changed, 5 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/alloc.h b/lib/alloc.h
>>> index 9b4b634..db90b01 100644
>>> --- a/lib/alloc.h
>>> +++ b/lib/alloc.h
>>> @@ -25,7 +25,6 @@
>>> struct alloc_ops {
>>> void *(*memalign)(size_t alignment, size_t size);
>>> void (*free)(void *ptr);
>>> - size_t align_min;
>>> };
>>>
>>> extern struct alloc_ops *alloc_ops;
>>> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
>>> index 8d2700d..b1cdf21 100644
>>> --- a/lib/alloc_page.c
>>> +++ b/lib/alloc_page.c
>>> @@ -385,7 +385,6 @@ void *memalign_pages_area(unsigned int area,
>>> size_t alignment, size_t size) static struct alloc_ops
>>> page_alloc_ops = { .memalign = memalign_pages,
>>> .free = free_pages,
>>> - .align_min = PAGE_SIZE,
>>> };
>>>
>>> /*
>>> diff --git a/lib/alloc_phys.c b/lib/alloc_phys.c
>>> index 72e20f7..a4d2bf2 100644
>>> --- a/lib/alloc_phys.c
>>> +++ b/lib/alloc_phys.c
>>> @@ -29,8 +29,8 @@ static phys_addr_t base, top;
>>> static void *early_memalign(size_t alignment, size_t size);
>>> static struct alloc_ops early_alloc_ops = {
>>> .memalign = early_memalign,
>>> - .align_min = DEFAULT_MINIMUM_ALIGNMENT
>>> };
>>> +static size_t align_min;
>>
>> I don't see any caller of phys_alloc_set_minimum_alignment(). So when
> lib/arm/setup.c:150
> lib/powerpc/setup.c:150
>
>> you are comparing against this variable in phys_alloc_aligned_safe()
>> below, you are comparing against zero. Is that what you is intended
>> or should 'align_min' be set some default ?
> if the architecture specific code did not specify anything better, 0 is
> ok.
>
>>>
>>> struct alloc_ops *alloc_ops = &early_alloc_ops;
>>>
>>> @@ -39,8 +39,7 @@ void phys_alloc_show(void)
>>> int i;
>>>
>>> spin_lock(&lock);
>>> - printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
>>> - (u64)early_alloc_ops.align_min);
>>> + printf("phys_alloc minimum alignment: %#" PRIx64 "\n",
>>> (u64)align_min); for (i = 0; i < nr_regions; ++i)
>>> printf("%016" PRIx64 "-%016" PRIx64 " [%s]\n",
>>> (u64)regions[i].base,
>>> @@ -64,7 +63,7 @@ void phys_alloc_set_minimum_alignment(phys_addr_t
>>> align) {
>>> assert(align && !(align & (align - 1)));
>>> spin_lock(&lock);
>>> - early_alloc_ops.align_min = align;
>>> + align_min = align;
>>> spin_unlock(&lock);
>>> }
>>>
>>> @@ -83,6 +82,8 @@ static phys_addr_t
>>> phys_alloc_aligned_safe(phys_addr_t size, top_safe = MIN(top_safe,
>>> 1ULL << 32);
>>> assert(base < top_safe);
>>> + if (align < align_min)
>>> + align = align_min;
>>>
>>> addr = ALIGN(base, align);
>>> size += addr - base;
>>> diff --git a/lib/vmalloc.c b/lib/vmalloc.c
>>> index 7a49adf..e146162 100644
>>> --- a/lib/vmalloc.c
>>> +++ b/lib/vmalloc.c
>>> @@ -190,7 +190,6 @@ static void vm_free(void *mem)
>>> static struct alloc_ops vmalloc_ops = {
>>> .memalign = vm_memalign,
>>> .free = vm_free,
>>> - .align_min = PAGE_SIZE,
>>> };
>>>
>>> void __attribute__((__weak__)) find_highmem(void)
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [kvm-unit-tests PATCH v1 07/12] lib/alloc_page: Optimization to skip known empty freelists
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (5 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 06/12] lib/alloc.h: remove align_min from struct alloc_ops Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 08/12] lib/alloc_page: rework metadata format Claudio Imbrenda
` (6 subsequent siblings)
13 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Keep track of the largest block order available in each area, and do not
search past it when looking for free memory.
This will avoid needlessly scanning the freelists for the largest block
orders, which will be empty in most cases.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index b1cdf21..6a76b45 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -31,6 +31,8 @@ struct mem_area {
pfn_t top;
/* Per page metadata, each entry is a combination *_MASK and order */
u8 *page_states;
+ /* Highest block order available in this area */
+ u8 max_order;
/* One freelist for each possible block size, up to NLISTS */
struct linked_list freelists[NLISTS];
};
@@ -104,6 +106,8 @@ static void split(struct mem_area *a, void *addr)
assert(a->page_states[idx + i] == order);
a->page_states[idx + i] = order - 1;
}
+ if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
+ a->max_order--;
order--;
/* add the first half block to the appropriate free list */
list_add(a->freelists + order, addr);
@@ -127,13 +131,13 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
order = sz > al ? sz : al;
/* search all free lists for some memory */
- for ( ; order < NLISTS; order++) {
+ for ( ; order <= a->max_order; order++) {
p = a->freelists[order].next;
if (!is_list_empty(p))
break;
}
/* out of memory */
- if (order >= NLISTS)
+ if (order > a->max_order)
return NULL;
/*
@@ -201,6 +205,8 @@ static bool coalesce(struct mem_area *a, u8 order, pfn_t pfn, pfn_t pfn2)
}
/* finally add the newly coalesced block to the appropriate freelist */
list_add(a->freelists + order + 1, pfn_to_virt(pfn));
+ if (order + 1 > a->max_order)
+ a->max_order = order + 1;
return true;
}
@@ -438,6 +444,7 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
a->page_states = pfn_to_virt(start_pfn);
a->base = start_pfn + table_size;
a->top = top_pfn;
+ a->max_order = 0;
npages = top_pfn - a->base;
assert((a->base - start_pfn) * PAGE_SIZE >= npages);
@@ -472,6 +479,8 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
/* initialize the metadata and add to the freelist */
memset(a->page_states + (i - a->base), order, BIT(order));
list_add(a->freelists + order, pfn_to_virt(i));
+ if (order > a->max_order)
+ a->max_order = order;
}
/* finally mark the area as present */
areas_mask |= BIT(n);
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* [kvm-unit-tests PATCH v1 08/12] lib/alloc_page: rework metadata format
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (6 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 07/12] lib/alloc_page: Optimization to skip known empty freelists Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 09/12] lib/alloc: replace areas with more generic flags Claudio Imbrenda
` (5 subsequent siblings)
13 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
This patch changes the format of the metadata so that the metadata is
now a 2-bit field instead of two separate flags.
This allows to have 4 different states for memory:
STATUS_FRESH: the memory is free and has not been touched at all since
boot (not even read from!)
STATUS_FREE: the memory is free, but it is probably not fresh any more
STATUS_ALLOCATED: the memory has been allocated and is in use
STATUS_SPECIAL: the memory has been removed from the pool of allocated
memory for some kind of special purpose according to
the needs of the caller
Some macros are also introduced to test the status of a specific
metadata item.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.c | 49 +++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 19 deletions(-)
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 6a76b45..dfa43d5 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -18,9 +18,20 @@
#define IS_ALIGNED_ORDER(x,order) IS_ALIGNED((x),BIT_ULL(order))
#define NLISTS ((BITS_PER_LONG) - (PAGE_SHIFT))
-#define ORDER_MASK 0x3f
-#define ALLOC_MASK 0x40
-#define SPECIAL_MASK 0x80
+#define ORDER_MASK 0x3f
+#define STATUS_MASK 0xc0
+
+#define STATUS_FRESH 0x00
+#define STATUS_FREE 0x40
+#define STATUS_ALLOCATED 0x80
+#define STATUS_SPECIAL 0xc0
+
+#define IS_FRESH(x) (((x) & STATUS_MASK) == STATUS_FRESH)
+#define IS_FREE(x) (((x) & STATUS_MASK) == STATUS_FREE)
+#define IS_ALLOCATED(x) (((x) & STATUS_MASK) == STATUS_ALLOCATED)
+#define IS_SPECIAL(x) (((x) & STATUS_MASK) == STATUS_SPECIAL)
+
+#define IS_USABLE(x) (IS_FREE(x) || IS_FRESH(x))
typedef phys_addr_t pfn_t;
@@ -87,14 +98,14 @@ static inline bool usable_area_contains_pfn(struct mem_area *a, pfn_t pfn)
*/
static void split(struct mem_area *a, void *addr)
{
- pfn_t pfn = virt_to_pfn(addr);
- pfn_t i, idx;
- u8 order;
+ pfn_t i, idx, pfn = virt_to_pfn(addr);
+ u8 metadata, order;
assert(a && usable_area_contains_pfn(a, pfn));
idx = pfn - a->base;
- order = a->page_states[idx];
- assert(!(order & ~ORDER_MASK) && order && (order < NLISTS));
+ metadata = a->page_states[idx];
+ order = metadata & ORDER_MASK;
+ assert(IS_USABLE(metadata) && order && (order < NLISTS));
assert(IS_ALIGNED_ORDER(pfn, order));
assert(usable_area_contains_pfn(a, pfn + BIT(order) - 1));
@@ -103,8 +114,8 @@ static void split(struct mem_area *a, void *addr)
/* update the block size for each page in the block */
for (i = 0; i < BIT(order); i++) {
- assert(a->page_states[idx + i] == order);
- a->page_states[idx + i] = order - 1;
+ assert(a->page_states[idx + i] == metadata);
+ a->page_states[idx + i] = metadata - 1;
}
if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
a->max_order--;
@@ -149,7 +160,7 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
split(a, p);
list_remove(p);
- memset(a->page_states + (virt_to_pfn(p) - a->base), ALLOC_MASK | order, BIT(order));
+ memset(a->page_states + (virt_to_pfn(p) - a->base), STATUS_ALLOCATED | order, BIT(order));
return p;
}
@@ -243,7 +254,7 @@ static void _free_pages(void *mem)
order = a->page_states[p] & ORDER_MASK;
/* ensure that the first page is allocated and not special */
- assert(a->page_states[p] == (order | ALLOC_MASK));
+ assert(IS_ALLOCATED(a->page_states[p]));
/* ensure that the order has a sane value */
assert(order < NLISTS);
/* ensure that the block is aligned properly for its size */
@@ -253,9 +264,9 @@ static void _free_pages(void *mem)
for (i = 0; i < BIT(order); i++) {
/* check that all pages of the block have consistent metadata */
- assert(a->page_states[p + i] == (ALLOC_MASK | order));
+ assert(a->page_states[p + i] == (STATUS_ALLOCATED | order));
/* set the page as free */
- a->page_states[p + i] &= ~ALLOC_MASK;
+ a->page_states[p + i] = STATUS_FREE | order;
}
/* provisionally add the block to the appropriate free list */
list_add(a->freelists + order, mem);
@@ -294,13 +305,13 @@ static bool _alloc_page_special(pfn_t pfn)
if (!a)
return false;
i = pfn - a->base;
- if (a->page_states[i] & (ALLOC_MASK | SPECIAL_MASK))
+ if (!IS_USABLE(a->page_states[i]))
return false;
while (a->page_states[i]) {
mask = GENMASK_ULL(63, a->page_states[i]);
split(a, pfn_to_virt(pfn & mask));
}
- a->page_states[i] = SPECIAL_MASK;
+ a->page_states[i] = STATUS_SPECIAL;
return true;
}
@@ -312,8 +323,8 @@ static void _free_page_special(pfn_t pfn)
a = get_area(pfn);
assert(a);
i = pfn - a->base;
- assert(a->page_states[i] == SPECIAL_MASK);
- a->page_states[i] = ALLOC_MASK;
+ assert(a->page_states[i] == STATUS_SPECIAL);
+ a->page_states[i] = STATUS_ALLOCATED;
_free_pages(pfn_to_virt(pfn));
}
@@ -477,7 +488,7 @@ static void _page_alloc_init_area(u8 n, pfn_t start_pfn, pfn_t top_pfn)
order++;
assert(order < NLISTS);
/* initialize the metadata and add to the freelist */
- memset(a->page_states + (i - a->base), order, BIT(order));
+ memset(a->page_states + (i - a->base), STATUS_FRESH | order, BIT(order));
list_add(a->freelists + order, pfn_to_virt(i));
if (order > a->max_order)
a->max_order = order;
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* [kvm-unit-tests PATCH v1 09/12] lib/alloc: replace areas with more generic flags
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (7 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 08/12] lib/alloc_page: rework metadata format Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 10/12] lib/alloc_page: Wire up ZERO_FLAG Claudio Imbrenda
` (4 subsequent siblings)
13 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Replace the areas parameter with a more generic flags parameter.
This allows for up to 16 allocation areas and 16 allocation flags.
This patch introduces the flags and changes the names of the funcions,
subsequent patches will actually wire up the flags to do something.
The first two flags introduced are:
FLAG_ZERO to ask the allocated memory to be zeroed
FLAG_FRESH to indicate that the allocated memory should have not been
touched (READ or written to) in any way since boot.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.h | 21 +++++++++++++--------
lib/alloc_page.c | 14 +++++++-------
lib/s390x/smp.c | 2 +-
3 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index d8550c6..1039814 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -11,8 +11,13 @@
#include <stdbool.h>
#include <asm/memory_areas.h>
-#define AREA_ANY -1
-#define AREA_ANY_NUMBER 0xff
+#define AREA_ANY_NUMBER 0xff
+
+#define AREA_ANY 0x00000
+#define AREA_MASK 0x0ffff
+
+#define FLAG_ZERO 0x10000
+#define FLAG_FRESH 0x20000
/* Returns true if the page allocator has been initialized */
bool page_alloc_initialized(void);
@@ -34,22 +39,22 @@ void page_alloc_ops_enable(void);
* areas is a bitmap of allowed areas
* alignment must be a power of 2
*/
-void *memalign_pages_area(unsigned int areas, size_t alignment, size_t size);
+void *memalign_pages_flags(size_t alignment, size_t size, unsigned int flags);
/*
* Allocate aligned memory from any area.
- * Equivalent to memalign_pages_area(AREA_ANY, alignment, size).
+ * Equivalent to memalign_pages_flags(alignment, size, AREA_ANY).
*/
static inline void *memalign_pages(size_t alignment, size_t size)
{
- return memalign_pages_area(AREA_ANY, alignment, size);
+ return memalign_pages_flags(alignment, size, AREA_ANY);
}
/*
* Allocate naturally aligned memory from the specified areas.
- * Equivalent to memalign_pages_area(areas, 1ull << order, 1ull << order).
+ * Equivalent to memalign_pages_flags(1ull << order, 1ull << order, flags).
*/
-void *alloc_pages_area(unsigned int areas, unsigned int order);
+void *alloc_pages_flags(unsigned int order, unsigned int flags);
/*
* Allocate naturally aligned memory from any area.
@@ -57,7 +62,7 @@ void *alloc_pages_area(unsigned int areas, unsigned int order);
*/
static inline void *alloc_pages(unsigned int order)
{
- return alloc_pages_area(AREA_ANY, order);
+ return alloc_pages_flags(order, AREA_ANY);
}
/*
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index dfa43d5..d850b6a 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -361,13 +361,13 @@ void free_pages_special(phys_addr_t addr, size_t n)
spin_unlock(&lock);
}
-static void *page_memalign_order_area(unsigned area, u8 ord, u8 al)
+static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
{
void *res = NULL;
- int i;
+ int i, area;
spin_lock(&lock);
- area &= areas_mask;
+ area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
for (i = 0; !res && (i < MAX_AREAS); i++)
if (area & BIT(i))
res = page_memalign_order(areas + i, ord, al);
@@ -379,23 +379,23 @@ static void *page_memalign_order_area(unsigned area, u8 ord, u8 al)
* Allocates (1 << order) physically contiguous and naturally aligned pages.
* Returns NULL if the allocation was not possible.
*/
-void *alloc_pages_area(unsigned int area, unsigned int order)
+void *alloc_pages_flags(unsigned int order, unsigned int flags)
{
- return page_memalign_order_area(area, order, order);
+ return page_memalign_order_flags(order, order, flags);
}
/*
* Allocates (1 << order) physically contiguous aligned pages.
* Returns NULL if the allocation was not possible.
*/
-void *memalign_pages_area(unsigned int area, size_t alignment, size_t size)
+void *memalign_pages_flags(size_t alignment, size_t size, unsigned int flags)
{
assert(is_power_of_2(alignment));
alignment = get_order(PAGE_ALIGN(alignment) >> PAGE_SHIFT);
size = get_order(PAGE_ALIGN(size) >> PAGE_SHIFT);
assert(alignment < NLISTS);
assert(size < NLISTS);
- return page_memalign_order_area(area, size, alignment);
+ return page_memalign_order_flags(size, alignment, flags);
}
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
index 77d80ca..44b2eb4 100644
--- a/lib/s390x/smp.c
+++ b/lib/s390x/smp.c
@@ -190,7 +190,7 @@ int smp_cpu_setup(uint16_t addr, struct psw psw)
sigp_retry(cpu->addr, SIGP_INITIAL_CPU_RESET, 0, NULL);
- lc = alloc_pages_area(AREA_DMA31, 1);
+ lc = alloc_pages_flags(1, AREA_DMA31);
cpu->lowcore = lc;
memset(lc, 0, PAGE_SIZE * 2);
sigp_retry(cpu->addr, SIGP_SET_PREFIX, (unsigned long )lc, NULL);
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* [kvm-unit-tests PATCH v1 10/12] lib/alloc_page: Wire up ZERO_FLAG
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (8 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 09/12] lib/alloc: replace areas with more generic flags Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 11/12] lib/alloc_page: Properly handle requests for fresh blocks Claudio Imbrenda
` (3 subsequent siblings)
13 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Memory allocated with the ZERO_FLAG will now be zeroed before being
returned to the caller.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index d850b6a..8c79202 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -372,6 +372,8 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
if (area & BIT(i))
res = page_memalign_order(areas + i, ord, al);
spin_unlock(&lock);
+ if (res && (flags & FLAG_ZERO))
+ memset(res, 0, BIT(ord) * PAGE_SIZE);
return res;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* [kvm-unit-tests PATCH v1 11/12] lib/alloc_page: Properly handle requests for fresh blocks
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (9 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 10/12] lib/alloc_page: Wire up ZERO_FLAG Claudio Imbrenda
@ 2020-12-16 20:11 ` Claudio Imbrenda
2020-12-16 20:12 ` [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default Claudio Imbrenda
` (2 subsequent siblings)
13 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:11 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
Upon initialization, all memory in an area is marked as fresh. Once
memory is used and freed, the freed memory is marked as free.
Free memory is always appended to the front of the freelist, meaning
that fresh memory stays on the tail.
When a block of fresh memory is split, the two blocks are put on the
tail of the appropriate freelist, so they can be found when needed.
When a fresh block is requested, a fresh block one order bigger is
taken, the first half is put back in the free pool (on the tail), and
the second half is returned. The reason behind this is that the first
page of every block always contains the pointers of the freelist.
Since the first page of a fresh block is actually not fresh, it cannot
be returned when a fresh allocation is requested.
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.c | 51 +++++++++++++++++++++++++++++++++++++-----------
1 file changed, 40 insertions(+), 11 deletions(-)
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 8c79202..4d5722f 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -120,10 +120,17 @@ static void split(struct mem_area *a, void *addr)
if ((order == a->max_order) && (is_list_empty(a->freelists + order)))
a->max_order--;
order--;
- /* add the first half block to the appropriate free list */
- list_add(a->freelists + order, addr);
- /* add the second half block to the appropriate free list */
- list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+
+ /* add the two half blocks to the appropriate free list */
+ if (IS_FRESH(metadata)) {
+ /* add to the tail if the blocks are fresh */
+ list_add_tail(a->freelists + order, addr);
+ list_add_tail(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+ } else {
+ /* add to the front if the blocks are dirty */
+ list_add(a->freelists + order, addr);
+ list_add(a->freelists + order, pfn_to_virt(pfn + BIT(order)));
+ }
}
/*
@@ -132,21 +139,33 @@ static void split(struct mem_area *a, void *addr)
*
* Both parameters must be not larger than the largest allowed order
*/
-static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
+static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz, bool fresh)
{
struct linked_list *p;
+ pfn_t idx;
u8 order;
assert((al < NLISTS) && (sz < NLISTS));
/* we need the bigger of the two as starting point */
order = sz > al ? sz : al;
+ /*
+ * we need to go one order up if we want a completely fresh block,
+ * since the first page contains the freelist pointers, and
+ * therefore it is always dirty
+ */
+ order += fresh;
/* search all free lists for some memory */
for ( ; order <= a->max_order; order++) {
- p = a->freelists[order].next;
- if (!is_list_empty(p))
- break;
+ p = fresh ? a->freelists[order].prev : a->freelists[order].next;
+ if (is_list_empty(p))
+ continue;
+ idx = virt_to_pfn(p) - a->base;
+ if (fresh && !IS_FRESH(a->page_states[idx]))
+ continue;
+ break;
}
+
/* out of memory */
if (order > a->max_order)
return NULL;
@@ -160,7 +179,16 @@ static void *page_memalign_order(struct mem_area *a, u8 al, u8 sz)
split(a, p);
list_remove(p);
- memset(a->page_states + (virt_to_pfn(p) - a->base), STATUS_ALLOCATED | order, BIT(order));
+ /* We now have a block twice the size, but the first page is dirty. */
+ if (fresh) {
+ order--;
+ /* Put back the first (partially dirty) half of the block */
+ memset(a->page_states + idx, STATUS_FRESH | order, BIT(order));
+ list_add_tail(a->freelists + order, p);
+ idx += BIT(order);
+ p = pfn_to_virt(a->base + idx);
+ }
+ memset(a->page_states + idx, STATUS_ALLOCATED | order, BIT(order));
return p;
}
@@ -364,13 +392,14 @@ void free_pages_special(phys_addr_t addr, size_t n)
static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
{
void *res = NULL;
- int i, area;
+ int i, area, fresh;
+ fresh = !!(flags & FLAG_FRESH);
spin_lock(&lock);
area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
for (i = 0; !res && (i < MAX_AREAS); i++)
if (area & BIT(i))
- res = page_memalign_order(areas + i, ord, al);
+ res = page_memalign_order(areas + i, ord, al, fresh);
spin_unlock(&lock);
if (res && (flags & FLAG_ZERO))
memset(res, 0, BIT(ord) * PAGE_SIZE);
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (10 preceding siblings ...)
2020-12-16 20:11 ` [kvm-unit-tests PATCH v1 11/12] lib/alloc_page: Properly handle requests for fresh blocks Claudio Imbrenda
@ 2020-12-16 20:12 ` Claudio Imbrenda
2020-12-24 18:17 ` Krish Sadhukhan
2020-12-17 19:41 ` [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Nadav Amit
2020-12-24 18:19 ` Krish Sadhukhan
13 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-16 20:12 UTC (permalink / raw)
To: kvm; +Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
The new function page_alloc_set_default_flags can be used to set the
default flags for allocations. The passed value will be ORed with the
flags argument passed to the allocator at each allocation.
The default value for the default flags is FLAG_ZERO, which means that
by default all allocated memory is now zeroed, restoring the default
behaviour that had been accidentally removed by a previous commit.
If needed, a testcase can call page_alloc_set_default_flags(0) in order
to get non-zeroed pages from the allocator. For example, if the
testcase will need fresh memory, the zero flag should be removed from
the default.
Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
Reported-by: Nadav Amit <nadav.amit@gmail.com>
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
lib/alloc_page.h | 3 +++
lib/alloc_page.c | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/lib/alloc_page.h b/lib/alloc_page.h
index 1039814..8b53a58 100644
--- a/lib/alloc_page.h
+++ b/lib/alloc_page.h
@@ -22,6 +22,9 @@
/* Returns true if the page allocator has been initialized */
bool page_alloc_initialized(void);
+/* Sets the default flags for the page allocator, the default is FLAG_ZERO */
+void page_alloc_set_default_flags(unsigned int flags);
+
/*
* Initializes a memory area.
* n is the number of the area to initialize
diff --git a/lib/alloc_page.c b/lib/alloc_page.c
index 4d5722f..08e0d05 100644
--- a/lib/alloc_page.c
+++ b/lib/alloc_page.c
@@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
static unsigned int areas_mask;
/* Protects areas and areas mask */
static struct spinlock lock;
+/* Default behaviour: zero allocated pages */
+static unsigned int default_flags = FLAG_ZERO;
bool page_alloc_initialized(void)
{
return areas_mask != 0;
}
+void page_alloc_set_default_flags(unsigned int flags)
+{
+ default_flags = flags;
+}
+
/*
* Each memory area contains an array of metadata entries at the very
* beginning. The usable memory follows immediately afterwards.
@@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
void *res = NULL;
int i, area, fresh;
+ flags |= default_flags;
fresh = !!(flags & FLAG_FRESH);
spin_lock(&lock);
area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
--
2.26.2
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default
2020-12-16 20:12 ` [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default Claudio Imbrenda
@ 2020-12-24 18:17 ` Krish Sadhukhan
2021-01-04 13:32 ` Claudio Imbrenda
0 siblings, 1 reply; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:17 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:12 PM, Claudio Imbrenda wrote:
> The new function page_alloc_set_default_flags can be used to set the
> default flags for allocations. The passed value will be ORed with the
> flags argument passed to the allocator at each allocation.
>
> The default value for the default flags is FLAG_ZERO, which means that
> by default all allocated memory is now zeroed, restoring the default
> behaviour that had been accidentally removed by a previous commit.
>
> If needed, a testcase can call page_alloc_set_default_flags(0) in order
> to get non-zeroed pages from the allocator. For example, if the
> testcase will need fresh memory, the zero flag should be removed from
> the default.
>
> Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page allocator")
> Reported-by: Nadav Amit<nadav.amit@gmail.com>
>
> Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> ---
> lib/alloc_page.h | 3 +++
> lib/alloc_page.c | 8 ++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> index 1039814..8b53a58 100644
> --- a/lib/alloc_page.h
> +++ b/lib/alloc_page.h
> @@ -22,6 +22,9 @@
> /* Returns true if the page allocator has been initialized */
> bool page_alloc_initialized(void);
>
> +/* Sets the default flags for the page allocator, the default is FLAG_ZERO */
> +void page_alloc_set_default_flags(unsigned int flags);
> +
> /*
> * Initializes a memory area.
> * n is the number of the area to initialize
> diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> index 4d5722f..08e0d05 100644
> --- a/lib/alloc_page.c
> +++ b/lib/alloc_page.c
> @@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
> static unsigned int areas_mask;
> /* Protects areas and areas mask */
> static struct spinlock lock;
> +/* Default behaviour: zero allocated pages */
> +static unsigned int default_flags = FLAG_ZERO;
>
> bool page_alloc_initialized(void)
> {
> return areas_mask != 0;
> }
>
> +void page_alloc_set_default_flags(unsigned int flags)
> +{
> + default_flags = flags;
Who calls this functions ?
Just wondering if default flag should be a static set of flag values
which the caller can override based on needs rather than the caller
setting the default flag.
> +}
> +
> /*
> * Each memory area contains an array of metadata entries at the very
> * beginning. The usable memory follows immediately afterwards.
> @@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord, u8 al, u32 flags)
> void *res = NULL;
> int i, area, fresh;
>
> + flags |= default_flags;
> fresh = !!(flags & FLAG_FRESH);
> spin_lock(&lock);
> area = (flags & AREA_MASK) ? flags & areas_mask : areas_mask;
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default
2020-12-24 18:17 ` Krish Sadhukhan
@ 2021-01-04 13:32 ` Claudio Imbrenda
0 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-04 13:32 UTC (permalink / raw)
To: Krish Sadhukhan
Cc: kvm, frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On Thu, 24 Dec 2020 10:17:30 -0800
Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> On 12/16/20 12:12 PM, Claudio Imbrenda wrote:
> > The new function page_alloc_set_default_flags can be used to set the
> > default flags for allocations. The passed value will be ORed with
> > the flags argument passed to the allocator at each allocation.
> >
> > The default value for the default flags is FLAG_ZERO, which means
> > that by default all allocated memory is now zeroed, restoring the
> > default behaviour that had been accidentally removed by a previous
> > commit.
> >
> > If needed, a testcase can call page_alloc_set_default_flags(0) in
> > order to get non-zeroed pages from the allocator. For example, if
> > the testcase will need fresh memory, the zero flag should be
> > removed from the default.
> >
> > Fixes: 8131e91a4b61 ("lib/alloc_page: complete rewrite of the page
> > allocator") Reported-by: Nadav Amit<nadav.amit@gmail.com>
> >
> > Signed-off-by: Claudio Imbrenda<imbrenda@linux.ibm.com>
> > ---
> > lib/alloc_page.h | 3 +++
> > lib/alloc_page.c | 8 ++++++++
> > 2 files changed, 11 insertions(+)
> >
> > diff --git a/lib/alloc_page.h b/lib/alloc_page.h
> > index 1039814..8b53a58 100644
> > --- a/lib/alloc_page.h
> > +++ b/lib/alloc_page.h
> > @@ -22,6 +22,9 @@
> > /* Returns true if the page allocator has been initialized */
> > bool page_alloc_initialized(void);
> >
> > +/* Sets the default flags for the page allocator, the default is
> > FLAG_ZERO */ +void page_alloc_set_default_flags(unsigned int flags);
> > +
> > /*
> > * Initializes a memory area.
> > * n is the number of the area to initialize
> > diff --git a/lib/alloc_page.c b/lib/alloc_page.c
> > index 4d5722f..08e0d05 100644
> > --- a/lib/alloc_page.c
> > +++ b/lib/alloc_page.c
> > @@ -54,12 +54,19 @@ static struct mem_area areas[MAX_AREAS];
> > static unsigned int areas_mask;
> > /* Protects areas and areas mask */
> > static struct spinlock lock;
> > +/* Default behaviour: zero allocated pages */
> > +static unsigned int default_flags = FLAG_ZERO;
> >
> > bool page_alloc_initialized(void)
> > {
> > return areas_mask != 0;
> > }
> >
> > +void page_alloc_set_default_flags(unsigned int flags)
> > +{
> > + default_flags = flags;
>
>
> Who calls this functions ?
nobody yet, since I have just introduced them.
The idea is that a testcase should call this early on to set the default
> Just wondering if default flag should be a static set of flag values
> which the caller can override based on needs rather than the caller
> setting the default flag.
hmmmmm
I would only need to reverse the semantics of FLAG_ZERO, but then I can
get rid of this patch
I think I'll do it
> > +}
> > +
> > /*
> > * Each memory area contains an array of metadata entries at the
> > very
> > * beginning. The usable memory follows immediately afterwards.
> > @@ -394,6 +401,7 @@ static void *page_memalign_order_flags(u8 ord,
> > u8 al, u32 flags) void *res = NULL;
> > int i, area, fresh;
> >
> > + flags |= default_flags;
> > fresh = !!(flags & FLAG_FRESH);
> > spin_lock(&lock);
> > area = (flags & AREA_MASK) ? flags & areas_mask :
> > areas_mask;
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (11 preceding siblings ...)
2020-12-16 20:12 ` [kvm-unit-tests PATCH v1 12/12] lib/alloc_page: default flags and zero pages by default Claudio Imbrenda
@ 2020-12-17 19:41 ` Nadav Amit
2020-12-18 14:19 ` Claudio Imbrenda
2020-12-24 18:19 ` Krish Sadhukhan
13 siblings, 1 reply; 35+ messages in thread
From: Nadav Amit @ 2020-12-17 19:41 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: KVM, Janosch Frank, David Hildenbrand, Thomas Huth,
Paolo Bonzini, cohuck, lvivier
> On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> My previous patchseries was rushed and not polished enough. Furthermore it
> introduced some regressions.
>
> This patchseries fixes hopefully all the issues reported, and introduces
> some new features.
>
> It also simplifies the code and hopefully makes it more readable.
>
> Fixed:
> * allocated memory is now zeroed by default
Thanks for doing that. Before I test it, did you also fix the issue of x86’s
setup_vm() [1]?
[1] https://www.spinics.net/lists/kvm/msg230470.html
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator
2020-12-17 19:41 ` [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Nadav Amit
@ 2020-12-18 14:19 ` Claudio Imbrenda
2020-12-28 6:31 ` Nadav Amit
0 siblings, 1 reply; 35+ messages in thread
From: Claudio Imbrenda @ 2020-12-18 14:19 UTC (permalink / raw)
To: Nadav Amit
Cc: KVM, Janosch Frank, David Hildenbrand, Thomas Huth,
Paolo Bonzini, cohuck, lvivier
On Thu, 17 Dec 2020 11:41:05 -0800
Nadav Amit <nadav.amit@gmail.com> wrote:
> > On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda
> > <imbrenda@linux.ibm.com> wrote:
> >
> > My previous patchseries was rushed and not polished enough.
> > Furthermore it introduced some regressions.
> >
> > This patchseries fixes hopefully all the issues reported, and
> > introduces some new features.
> >
> > It also simplifies the code and hopefully makes it more readable.
> >
> > Fixed:
> > * allocated memory is now zeroed by default
>
> Thanks for doing that. Before I test it, did you also fix the issue
> of x86’s setup_vm() [1]?
>
> [1] https://www.spinics.net/lists/kvm/msg230470.html
unfortunately no, because I could not reproduce it.
In theory setup_vm should just work, since it is called twice, but on
different address ranges.
The only issue I can think of is if the second call overlaps multiple
areas.
can you send me the memory map of that machine you're running the tests
on? (/proc/iomem)
Claudio
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator
2020-12-18 14:19 ` Claudio Imbrenda
@ 2020-12-28 6:31 ` Nadav Amit
2021-01-05 15:26 ` Claudio Imbrenda
0 siblings, 1 reply; 35+ messages in thread
From: Nadav Amit @ 2020-12-28 6:31 UTC (permalink / raw)
To: Claudio Imbrenda
Cc: KVM, Janosch Frank, David Hildenbrand, Thomas Huth,
Paolo Bonzini, cohuck, lvivier
> On Dec 18, 2020, at 6:19 AM, Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> On Thu, 17 Dec 2020 11:41:05 -0800
> Nadav Amit <nadav.amit@gmail.com> wrote:
>
>>> On Dec 16, 2020, at 12:11 PM, Claudio Imbrenda
>>> <imbrenda@linux.ibm.com> wrote:
>>>
>>> My previous patchseries was rushed and not polished enough.
>>> Furthermore it introduced some regressions.
>>>
>>> This patchseries fixes hopefully all the issues reported, and
>>> introduces some new features.
>>>
>>> It also simplifies the code and hopefully makes it more readable.
>>>
>>> Fixed:
>>> * allocated memory is now zeroed by default
>>
>> Thanks for doing that. Before I test it, did you also fix the issue
>> of x86’s setup_vm() [1]?
>>
>> [1] https://www.spinics.net/lists/kvm/msg230470.html
>
> unfortunately no, because I could not reproduce it.
>
> In theory setup_vm should just work, since it is called twice, but on
> different address ranges.
>
> The only issue I can think of is if the second call overlaps multiple
> areas.
>
> can you send me the memory map of that machine you're running the tests
> on? (/proc/iomem)
Sorry for the late response.
I see two calls to _page_alloc_init_area() with AREA_LOW_NUMBER, one with
(base_pfn=621, top_pfn=bfdd0) and one with (base_pfn=100000 top_pfn=240000).
As for /proc/iomem:
00000000-00000fff : Reserved
00001000-0009e7ff : System RAM
0009e800-0009ffff : Reserved
000a0000-000bffff : PCI Bus 0000:00
000c0000-000c7fff : Video ROM
000ca000-000cafff : Adapter ROM
000cb000-000ccfff : Adapter ROM
000d0000-000d3fff : PCI Bus 0000:00
000d4000-000d7fff : PCI Bus 0000:00
000d8000-000dbfff : PCI Bus 0000:00
000dc000-000fffff : Reserved
000f0000-000fffff : System ROM
00100000-bfecffff : System RAM
01000000-01c031d0 : Kernel code
01c031d1-0266f3bf : Kernel data
028ef000-02b97fff : Kernel bss
bfed0000-bfefefff : ACPI Tables
bfeff000-bfefffff : ACPI Non-volatile Storage
bff00000-bfffffff : System RAM
c0000000-febfffff : PCI Bus 0000:00
c0008000-c000bfff : 0000:00:10.0
e5b00000-e5bfffff : PCI Bus 0000:22
e5c00000-e5cfffff : PCI Bus 0000:1a
e5d00000-e5dfffff : PCI Bus 0000:12
e5e00000-e5efffff : PCI Bus 0000:0a
e5f00000-e5ffffff : PCI Bus 0000:21
e6000000-e60fffff : PCI Bus 0000:19
e6100000-e61fffff : PCI Bus 0000:11
e6200000-e62fffff : PCI Bus 0000:09
e6300000-e63fffff : PCI Bus 0000:20
e6400000-e64fffff : PCI Bus 0000:18
e6500000-e65fffff : PCI Bus 0000:10
e6600000-e66fffff : PCI Bus 0000:08
e6700000-e67fffff : PCI Bus 0000:1f
e6800000-e68fffff : PCI Bus 0000:17
e6900000-e69fffff : PCI Bus 0000:0f
e6a00000-e6afffff : PCI Bus 0000:07
e6b00000-e6bfffff : PCI Bus 0000:1e
e6c00000-e6cfffff : PCI Bus 0000:16
e6d00000-e6dfffff : PCI Bus 0000:0e
e6e00000-e6efffff : PCI Bus 0000:06
e6f00000-e6ffffff : PCI Bus 0000:1d
e7000000-e70fffff : PCI Bus 0000:15
e7100000-e71fffff : PCI Bus 0000:0d
e7200000-e72fffff : PCI Bus 0000:05
e7300000-e73fffff : PCI Bus 0000:1c
e7400000-e74fffff : PCI Bus 0000:14
e7500000-e75fffff : PCI Bus 0000:0c
e7600000-e76fffff : PCI Bus 0000:04
e7700000-e77fffff : PCI Bus 0000:1b
e7800000-e78fffff : PCI Bus 0000:13
e7900000-e79fffff : PCI Bus 0000:0b
e7a00000-e7afffff : PCI Bus 0000:03
e7b00000-e7ffffff : PCI Bus 0000:02
e8000000-efffffff : 0000:00:0f.0
e8000000-efffffff : vmwgfx probe
f0000000-f7ffffff : PCI MMCONFIG 0000 [bus 00-7f]
f0000000-f7ffffff : Reserved
f0000000-f7ffffff : pnp 00:08
fb500000-fb5fffff : PCI Bus 0000:22
fb600000-fb6fffff : PCI Bus 0000:1a
fb700000-fb7fffff : PCI Bus 0000:12
fb800000-fb8fffff : PCI Bus 0000:0a
fb900000-fb9fffff : PCI Bus 0000:21
fba00000-fbafffff : PCI Bus 0000:19
fbb00000-fbbfffff : PCI Bus 0000:11
fbc00000-fbcfffff : PCI Bus 0000:09
fbd00000-fbdfffff : PCI Bus 0000:20
fbe00000-fbefffff : PCI Bus 0000:18
fbf00000-fbffffff : PCI Bus 0000:10
fc000000-fc0fffff : PCI Bus 0000:08
fc100000-fc1fffff : PCI Bus 0000:1f
fc200000-fc2fffff : PCI Bus 0000:17
fc300000-fc3fffff : PCI Bus 0000:0f
fc400000-fc4fffff : PCI Bus 0000:07
fc500000-fc5fffff : PCI Bus 0000:1e
fc600000-fc6fffff : PCI Bus 0000:16
fc700000-fc7fffff : PCI Bus 0000:0e
fc800000-fc8fffff : PCI Bus 0000:06
fc900000-fc9fffff : PCI Bus 0000:1d
fca00000-fcafffff : PCI Bus 0000:15
fcb00000-fcbfffff : PCI Bus 0000:0d
fcc00000-fccfffff : PCI Bus 0000:05
fcd00000-fcdfffff : PCI Bus 0000:1c
fce00000-fcefffff : PCI Bus 0000:14
fcf00000-fcffffff : PCI Bus 0000:0c
fd000000-fd0fffff : PCI Bus 0000:04
fd100000-fd1fffff : PCI Bus 0000:1b
fd200000-fd2fffff : PCI Bus 0000:13
fd300000-fd3fffff : PCI Bus 0000:0b
fd400000-fd4fffff : PCI Bus 0000:03
fd500000-fdffffff : PCI Bus 0000:02
fd500000-fd50ffff : 0000:02:01.0
fd510000-fd51ffff : 0000:02:05.0
fd5c0000-fd5dffff : 0000:02:01.0
fd5c0000-fd5dffff : e1000
fd5ee000-fd5eefff : 0000:02:05.0
fd5ee000-fd5eefff : ahci
fd5ef000-fd5effff : 0000:02:03.0
fd5ef000-fd5effff : ehci_hcd
fdff0000-fdffffff : 0000:02:01.0
fdff0000-fdffffff : e1000
fe000000-fe7fffff : 0000:00:0f.0
fe000000-fe7fffff : vmwgfx probe
fe800000-fe9fffff : pnp 00:08
feba0000-febbffff : 0000:00:10.0
feba0000-febbffff : mpt
febc0000-febdffff : 0000:00:10.0
febc0000-febdffff : mpt
febfe000-febfffff : 0000:00:07.7
fec00000-fec0ffff : Reserved
fec00000-fec003ff : IOAPIC 0
fec10000-fec10fff : dmar0
fed00000-fed003ff : HPET 0
fed00000-fed003ff : pnp 00:04
fee00000-fee00fff : Local APIC
fee00000-fee00fff : Reserved
fffe0000-ffffffff : Reserved
100000000-23fffffff : System RAM
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator
2020-12-28 6:31 ` Nadav Amit
@ 2021-01-05 15:26 ` Claudio Imbrenda
0 siblings, 0 replies; 35+ messages in thread
From: Claudio Imbrenda @ 2021-01-05 15:26 UTC (permalink / raw)
To: Nadav Amit
Cc: KVM, Janosch Frank, David Hildenbrand, Thomas Huth,
Paolo Bonzini, cohuck, lvivier
On Sun, 27 Dec 2020 22:31:56 -0800
Nadav Amit <nadav.amit@gmail.com> wrote:
[...]
> >> Thanks for doing that. Before I test it, did you also fix the issue
> >> of x86’s setup_vm() [1]?
> >>
> >> [1] https://www.spinics.net/lists/kvm/msg230470.html
> >
> > unfortunately no, because I could not reproduce it.
> >
> > In theory setup_vm should just work, since it is called twice, but
> > on different address ranges.
> >
> > The only issue I can think of is if the second call overlaps
> > multiple areas.
> >
> > can you send me the memory map of that machine you're running the
> > tests on? (/proc/iomem)
>
> Sorry for the late response.
>
> I see two calls to _page_alloc_init_area() with AREA_LOW_NUMBER, one
> with (base_pfn=621, top_pfn=bfdd0) and one with (base_pfn=100000
> top_pfn=240000).
ok, I could reproduce the issue now.
to put it simply, the old code is broken.
I could not reproduce the issue with the new code, so you should be
able to test it.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator
2020-12-16 20:11 [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Claudio Imbrenda
` (12 preceding siblings ...)
2020-12-17 19:41 ` [kvm-unit-tests PATCH v1 00/12] Fix and improve the page allocator Nadav Amit
@ 2020-12-24 18:19 ` Krish Sadhukhan
13 siblings, 0 replies; 35+ messages in thread
From: Krish Sadhukhan @ 2020-12-24 18:19 UTC (permalink / raw)
To: Claudio Imbrenda, kvm
Cc: frankja, david, thuth, pbonzini, cohuck, lvivier, nadav.amit
On 12/16/20 12:11 PM, Claudio Imbrenda wrote:
> My previous patchseries was rushed and not polished enough. Furthermore it
> introduced some regressions.
>
> This patchseries fixes hopefully all the issues reported, and introduces
> some new features.
>
> It also simplifies the code and hopefully makes it more readable.
>
> Fixed:
> * allocated memory is now zeroed by default
>
> New features:
> * per-allocation flags to specify not just the area (like before) but also
> other parameters
> - zero flag: the allocation will be zeroed
> - fresh flag: the returned memory has never been read or written to before
> * default flags: it's possible to specify which flags should be enabled
> automatically at each allocation; by default the zero flag is set.
>
>
> I would appreciate if people could test these patches, especially on
> strange, unusual or exotic hardware (I tested only on s390x)
>
>
> GitLab:
> https://urldefense.com/v3/__https://gitlab.com/imbrenda/kvm-unit-tests/-/tree/page_allocator_fixes__;!!GqivPVa7Brio!LehVoD4e6fUc92A7OE_Rxl2QwwkrW4aY0WHTmPkgKyxYviNfnTs3hEmYWHsMj3I9paC-$
> CI:
> https://urldefense.com/v3/__https://gitlab.com/imbrenda/kvm-unit-tests/-/pipelines/229689192__;!!GqivPVa7Brio!LehVoD4e6fUc92A7OE_Rxl2QwwkrW4aY0WHTmPkgKyxYviNfnTs3hEmYWHsMj5GdDaIf$
>
>
> Claudio Imbrenda (12):
> lib/x86: fix page.h to include the generic header
> lib/list.h: add list_add_tail
> lib/vmalloc: add some asserts and improvements
> lib/asm: Fix definitions of memory areas
> lib/alloc_page: fix and improve the page allocator
> lib/alloc.h: remove align_min from struct alloc_ops
> lib/alloc_page: Optimization to skip known empty freelists
> lib/alloc_page: rework metadata format
> lib/alloc: replace areas with more generic flags
> lib/alloc_page: Wire up ZERO_FLAG
> lib/alloc_page: Properly handle requests for fresh blocks
> lib/alloc_page: default flags and zero pages by default
>
> lib/asm-generic/memory_areas.h | 9 +-
> lib/arm/asm/memory_areas.h | 11 +-
> lib/arm64/asm/memory_areas.h | 11 +-
> lib/powerpc/asm/memory_areas.h | 11 +-
> lib/ppc64/asm/memory_areas.h | 11 +-
> lib/s390x/asm/memory_areas.h | 13 +-
> lib/x86/asm/memory_areas.h | 27 +--
> lib/x86/asm/page.h | 4 +-
> lib/alloc.h | 1 -
> lib/alloc_page.h | 66 ++++++--
> lib/list.h | 9 +
> lib/alloc_page.c | 291 ++++++++++++++++++++-------------
> lib/alloc_phys.c | 9 +-
> lib/s390x/smp.c | 2 +-
> lib/vmalloc.c | 21 +--
> 15 files changed, 286 insertions(+), 210 deletions(-)
>
For patch# 1, 2, 7, 8, 9, 10 and 11:
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
^ permalink raw reply [flat|nested] 35+ messages in thread