| Message ID | 20200918163724.2511-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] zsmalloc: switch from alloc_vm_area to get_vm_area | expand |
On 9/18/20 12:37 PM, Christoph Hellwig wrote: > > +static int gnttab_apply(pte_t *pte, unsigned long addr, void *data) > +{ > + pte_t ***p = data; > + > + **p = pte; > + (*p)++; > + return 0; > +} > + > static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned nr_frames) > { > area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL); > if (area->ptes == NULL) > return -ENOMEM; > - > - area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes); > - if (area->area == NULL) { > - kfree(area->ptes); > - return -ENOMEM; > - } > - > + area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP); > + if (!area->area) > + goto out_free_ptes; > + if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, > + PAGE_SIZE * nr_frames, gnttab_apply, &area->ptes)) This will end up incrementing area->ptes pointer. So perhaps something like pte_t **ptes = area->ptes; if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, PAGE_SIZE * nr_frames, gnttab_apply, &ptes)) { ... } -boris
On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrovsky@oracle.com wrote: > This will end up incrementing area->ptes pointer. So perhaps something like > > > pte_t **ptes = area->ptes; > > if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, > PAGE_SIZE * nr_frames, gnttab_apply, &ptes)) { > > ... Yeah. What do you think of this version? I think it is a little cleaner and matches what xenbus does. At this point it probably should be split into a Xen and a alloc_vm_area removal patch, though. --- From 74d6b797e049f72b5e9f63f14da6321c4209a792 Mon Sep 17 00:00:00 2001 From: Christoph Hellwig <hch@lst.de> Date: Wed, 16 Sep 2020 16:09:42 +0200 Subject: x86/xen: open code alloc_vm_area in arch_gnttab_valloc Open code alloc_vm_area in the last remaining caller. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/x86/xen/grant-table.c | 27 +++++++++++++++------ include/linux/vmalloc.h | 5 +--- mm/nommu.c | 7 ------ mm/vmalloc.c | 48 -------------------------------------- 4 files changed, 21 insertions(+), 66 deletions(-) diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c index 4988e19598c8a5..1e681bf62561a0 100644 --- a/arch/x86/xen/grant-table.c +++ b/arch/x86/xen/grant-table.c @@ -25,6 +25,7 @@ static struct gnttab_vm_area { struct vm_struct *area; pte_t **ptes; + int idx; } gnttab_shared_vm_area, gnttab_status_vm_area; int arch_gnttab_map_shared(unsigned long *frames, unsigned long nr_gframes, @@ -90,19 +91,31 @@ void arch_gnttab_unmap(void *shared, unsigned long nr_gframes) } } +static int gnttab_apply(pte_t *pte, unsigned long addr, void *data) +{ + struct gnttab_vm_area *area = data; + + area->ptes[area->idx++] = pte; + return 0; +} + static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned nr_frames) { area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL); if (area->ptes == NULL) return -ENOMEM; - - area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes); - if (area->area == NULL) { - kfree(area->ptes); - return -ENOMEM; - } - + area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP); + if (!area->area) + goto out_free_ptes; + if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, + PAGE_SIZE * nr_frames, gnttab_apply, area)) + goto out_free_vm_area; return 0; +out_free_vm_area: + free_vm_area(area->area); +out_free_ptes: + kfree(area->ptes); + return -ENOMEM; } static void arch_gnttab_vfree(struct gnttab_vm_area *area) diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h index 8ecd92a947ee0c..a1a4e2f8163504 100644 --- a/include/linux/vmalloc.h +++ b/include/linux/vmalloc.h @@ -168,6 +168,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size, unsigned long flags, unsigned long start, unsigned long end, const void *caller); +void free_vm_area(struct vm_struct *area); extern struct vm_struct *remove_vm_area(const void *addr); extern struct vm_struct *find_vm_area(const void *addr); @@ -203,10 +204,6 @@ static inline void set_vm_flush_reset_perms(void *addr) } #endif -/* Allocate/destroy a 'vmalloc' VM area. */ -extern struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes); -extern void free_vm_area(struct vm_struct *area); - /* for /dev/kmem */ extern long vread(char *buf, char *addr, unsigned long count); extern long vwrite(char *buf, char *addr, unsigned long count); diff --git a/mm/nommu.c b/mm/nommu.c index 75a327149af127..9272f30e4c4726 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -354,13 +354,6 @@ void vm_unmap_aliases(void) } EXPORT_SYMBOL_GPL(vm_unmap_aliases); -struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes) -{ - BUG(); - return NULL; -} -EXPORT_SYMBOL_GPL(alloc_vm_area); - void free_vm_area(struct vm_struct *area) { BUG(); diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 59f2afcf26c312..9f29147deca580 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -3077,54 +3077,6 @@ int remap_vmalloc_range(struct vm_area_struct *vma, void *addr, } EXPORT_SYMBOL(remap_vmalloc_range); -static int f(pte_t *pte, unsigned long addr, void *data) -{ - pte_t ***p = data; - - if (p) { - *(*p) = pte; - (*p)++; - } - return 0; -} - -/** - * alloc_vm_area - allocate a range of kernel address space - * @size: size of the area - * @ptes: returns the PTEs for the address space - * - * Returns: NULL on failure, vm_struct on success - * - * This function reserves a range of kernel address space, and - * allocates pagetables to map that range. No actual mappings - * are created. - * - * If @ptes is non-NULL, pointers to the PTEs (in init_mm) - * allocated for the VM area are returned. - */ -struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes) -{ - struct vm_struct *area; - - area = get_vm_area_caller(size, VM_IOREMAP, - __builtin_return_address(0)); - if (area == NULL) - return NULL; - - /* - * This ensures that page tables are constructed for this region - * of kernel virtual address space and mapped into init_mm. - */ - if (apply_to_page_range(&init_mm, (unsigned long)area->addr, - size, f, ptes ? &ptes : NULL)) { - free_vm_area(area); - return NULL; - } - - return area; -} -EXPORT_SYMBOL_GPL(alloc_vm_area); - void free_vm_area(struct vm_struct *area) { struct vm_struct *ret;
On 9/22/20 10:58 AM, Christoph Hellwig wrote: > On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrovsky@oracle.com wrote: >> This will end up incrementing area->ptes pointer. So perhaps something like >> >> >> pte_t **ptes = area->ptes; >> >> if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, >> PAGE_SIZE * nr_frames, gnttab_apply, &ptes)) { >> >> ... > Yeah. What do you think of this version? Oh yes, this is way better. This now can actually be read without trying to mentally unwind triple pointers. (You probably want to initialize idx to zero before calling apply_to_page_range(), I am not sure it's guaranteed to be zero). > I think it is a little > cleaner and matches what xenbus does. At this point it probably should > be split into a Xen and a alloc_vm_area removal patch, though. Right. -boris
On Tue, Sep 22, 2020 at 11:24:20AM -0400, boris.ostrovsky@oracle.com wrote: > > On 9/22/20 10:58 AM, Christoph Hellwig wrote: > > On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrovsky@oracle.com wrote: > >> This will end up incrementing area->ptes pointer. So perhaps something like > >> > >> > >> pte_t **ptes = area->ptes; > >> > >> if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, > >> PAGE_SIZE * nr_frames, gnttab_apply, &ptes)) { > >> > >> ... > > Yeah. What do you think of this version? > > > Oh yes, this is way better. This now can actually be read without trying to mentally unwind triple pointers. (You probably want to initialize idx to zero before calling apply_to_page_range(), I am not sure it's guaranteed to be zero). Both instances are static variables, thus in .bss and initialized. So unless you insist I don't think we need a manual one.
On 9/22/20 11:27 AM, Christoph Hellwig wrote: > On Tue, Sep 22, 2020 at 11:24:20AM -0400, boris.ostrovsky@oracle.com wrote: >> On 9/22/20 10:58 AM, Christoph Hellwig wrote: >>> On Mon, Sep 21, 2020 at 04:44:10PM -0400, boris.ostrovsky@oracle.com wrote: >>>> This will end up incrementing area->ptes pointer. So perhaps something like >>>> >>>> >>>> pte_t **ptes = area->ptes; >>>> >>>> if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, >>>> PAGE_SIZE * nr_frames, gnttab_apply, &ptes)) { >>>> >>>> ... >>> Yeah. What do you think of this version? >> >> Oh yes, this is way better. This now can actually be read without trying to mentally unwind triple pointers. (You probably want to initialize idx to zero before calling apply_to_page_range(), I am not sure it's guaranteed to be zero). > Both instances are static variables, thus in .bss and initialized. > So unless you insist I don't think we need a manual one. Yes, you are right. (I thought perhaps this code could be called more than once but no, it can't). -boris
diff --git a/arch/x86/xen/grant-table.c b/arch/x86/xen/grant-table.c index 4988e19598c8a5..ccb377c07c651f 100644 --- a/arch/x86/xen/grant-table.c +++ b/arch/x86/xen/grant-table.c @@ -90,19 +90,32 @@ void arch_gnttab_unmap(void *shared, unsigned long nr_gframes) } } +static int gnttab_apply(pte_t *pte, unsigned long addr, void *data) +{ + pte_t ***p = data; + + **p = pte; + (*p)++; + return 0; +} + static int arch_gnttab_valloc(struct gnttab_vm_area *area, unsigned nr_frames) { area->ptes = kmalloc_array(nr_frames, sizeof(*area->ptes), GFP_KERNEL); if (area->ptes == NULL) return -ENOMEM; - - area->area = alloc_vm_area(PAGE_SIZE * nr_frames, area->ptes); - if (area->area == NULL) { - kfree(area->ptes); - return -ENOMEM; - } - + area->area = get_vm_area(PAGE_SIZE * nr_frames, VM_IOREMAP); + if (!area->area) + goto out_free_ptes; + if (apply_to_page_range(&init_mm, (unsigned long)area->area->addr, + PAGE_SIZE * nr_frames, gnttab_apply, &area->ptes)) + goto out_free_vm_area; return 0; +out_free_vm_area: + free_vm_area(area->area); +out_free_ptes: + kfree(area->ptes); + return -ENOMEM; } static void arch_gnttab_vfree(struct gnttab_vm_area *area) diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h index 8ecd92a947ee0c..a1a4e2f8163504 100644 --- a/include/linux/vmalloc.h +++ b/include/linux/vmalloc.h @@ -168,6 +168,7 @@ extern struct vm_struct *__get_vm_area_caller(unsigned long size, unsigned long flags, unsigned long start, unsigned long end, const void *caller); +void free_vm_area(struct vm_struct *area); extern struct vm_struct *remove_vm_area(const void *addr); extern struct vm_struct *find_vm_area(const void *addr); @@ -203,10 +204,6 @@ static inline void set_vm_flush_reset_perms(void *addr) } #endif -/* Allocate/destroy a 'vmalloc' VM area. */ -extern struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes); -extern void free_vm_area(struct vm_struct *area); - /* for /dev/kmem */ extern long vread(char *buf, char *addr, unsigned long count); extern long vwrite(char *buf, char *addr, unsigned long count); diff --git a/mm/nommu.c b/mm/nommu.c index 75a327149af127..9272f30e4c4726 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -354,13 +354,6 @@ void vm_unmap_aliases(void) } EXPORT_SYMBOL_GPL(vm_unmap_aliases); -struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes) -{ - BUG(); - return NULL; -} -EXPORT_SYMBOL_GPL(alloc_vm_area); - void free_vm_area(struct vm_struct *area) { BUG(); diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 59f2afcf26c312..9f29147deca580 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -3077,54 +3077,6 @@ int remap_vmalloc_range(struct vm_area_struct *vma, void *addr, } EXPORT_SYMBOL(remap_vmalloc_range); -static int f(pte_t *pte, unsigned long addr, void *data) -{ - pte_t ***p = data; - - if (p) { - *(*p) = pte; - (*p)++; - } - return 0; -} - -/** - * alloc_vm_area - allocate a range of kernel address space - * @size: size of the area - * @ptes: returns the PTEs for the address space - * - * Returns: NULL on failure, vm_struct on success - * - * This function reserves a range of kernel address space, and - * allocates pagetables to map that range. No actual mappings - * are created. - * - * If @ptes is non-NULL, pointers to the PTEs (in init_mm) - * allocated for the VM area are returned. - */ -struct vm_struct *alloc_vm_area(size_t size, pte_t **ptes) -{ - struct vm_struct *area; - - area = get_vm_area_caller(size, VM_IOREMAP, - __builtin_return_address(0)); - if (area == NULL) - return NULL; - - /* - * This ensures that page tables are constructed for this region - * of kernel virtual address space and mapped into init_mm. - */ - if (apply_to_page_range(&init_mm, (unsigned long)area->addr, - size, f, ptes ? &ptes : NULL)) { - free_vm_area(area); - return NULL; - } - - return area; -} -EXPORT_SYMBOL_GPL(alloc_vm_area); - void free_vm_area(struct vm_struct *area) { struct vm_struct *ret;
Open code alloc_vm_area in the last remaining caller. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/x86/xen/grant-table.c | 27 +++++++++++++++------ include/linux/vmalloc.h | 5 +--- mm/nommu.c | 7 ------ mm/vmalloc.c | 48 -------------------------------------- 4 files changed, 21 insertions(+), 66 deletions(-)