Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
Christophe Leroy's on June 19, 2019 11:18 pm: > > > Le 19/06/2019 à 05:43, Nicholas Piggin a écrit : >> Christophe Leroy's on June 11, 2019 3:24 pm: >>> >>> >>> Le 10/06/2019 à 06:38, Nicholas Piggin a écrit : > > [snip] > diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h index 51e131245379..812bea5866d6 100644 --- a/include/linux/vmalloc.h +++ b/include/linux/vmalloc.h @@ -147,6 +147,9 @@ extern struct vm_struct *find_vm_area(const void *addr); extern int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages); #ifdef CONFIG_MMU +extern int vmap_range(unsigned long addr, + unsigned long end, phys_addr_t phys_addr, pgprot_t prot, + unsigned int max_page_shift); >>> >>> Drop extern keyword here. >> >> I don't know if I was going crazy but at one point I was getting >> duplicate symbol errors that were fixed by adding extern somewhere. > > probably not on a function name ... I know it sounds crazy :P >>> As checkpatch tells you, 'CHECK:AVOID_EXTERNS: extern prototypes should >>> be avoided in .h files' >> >> I prefer to follow existing style in surrounding code at the expense >> of some checkpatch warnings. If somebody later wants to "fix" it >> that's fine. > > I don't think that's fine to 'fix' later things that could be done right > from the begining. 'Cosmetic only' fixes never happen because they are a > nightmare for backports, and a shame for 'git blame'. > > In some patches, you add cleanups to make the code look nicer, and here > you have the opportunity to make the code nice from the begining and you > prefer repeating the errors done in the past ? You're surprising me. Well I never claimed to be consistent. I actually don't mind the extern keyword so it's probably just my personal preference that makes me notice something nearby. I have dropped those "cleanup" changes though, so there. Thanks, Nick
Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
Le 19/06/2019 à 05:43, Nicholas Piggin a écrit : Christophe Leroy's on June 11, 2019 3:24 pm: Le 10/06/2019 à 06:38, Nicholas Piggin a écrit : [snip] diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h index 51e131245379..812bea5866d6 100644 --- a/include/linux/vmalloc.h +++ b/include/linux/vmalloc.h @@ -147,6 +147,9 @@ extern struct vm_struct *find_vm_area(const void *addr); extern int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page **pages); #ifdef CONFIG_MMU +extern int vmap_range(unsigned long addr, + unsigned long end, phys_addr_t phys_addr, pgprot_t prot, + unsigned int max_page_shift); Drop extern keyword here. I don't know if I was going crazy but at one point I was getting duplicate symbol errors that were fixed by adding extern somewhere. probably not on a function name ... Maybe sleep depravation. However... As checkpatch tells you, 'CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files' I prefer to follow existing style in surrounding code at the expense of some checkpatch warnings. If somebody later wants to "fix" it that's fine. I don't think that's fine to 'fix' later things that could be done right from the begining. 'Cosmetic only' fixes never happen because they are a nightmare for backports, and a shame for 'git blame'. In some patches, you add cleanups to make the code look nicer, and here you have the opportunity to make the code nice from the begining and you prefer repeating the errors done in the past ? You're surprising me. Christophe Thanks, Nick
Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
Christophe Leroy's on June 11, 2019 3:24 pm: > > > Le 10/06/2019 à 06:38, Nicholas Piggin a écrit : >> ioremap_page_range is a generic function to create a kernel virtual >> mapping, move it to mm/vmalloc.c and rename it vmap_range. >> >> For clarity with this move, also: >> - Rename vunmap_page_range (vmap_range's inverse) to vunmap_range. >> - Rename vmap_page_range (which takes a page array) to vmap_pages. > > Maybe it would be easier to follow the change if the name change was > done in another patch than the move. I could do that. >> Signed-off-by: Nicholas Piggin >> --- >> >> Fixed up the arm64 compile errors, fixed a few bugs, and tidied >> things up a bit more. >> >> Have tested powerpc and x86 but not arm64, would appreciate a review >> and test of the arm64 patch if possible. >> >> include/linux/vmalloc.h | 3 + >> lib/ioremap.c | 173 +++--- >> mm/vmalloc.c| 228 >> 3 files changed, 229 insertions(+), 175 deletions(-) >> >> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h >> index 51e131245379..812bea5866d6 100644 >> --- a/include/linux/vmalloc.h >> +++ b/include/linux/vmalloc.h >> @@ -147,6 +147,9 @@ extern struct vm_struct *find_vm_area(const void *addr); >> extern int map_vm_area(struct vm_struct *area, pgprot_t prot, >> struct page **pages); >> #ifdef CONFIG_MMU >> +extern int vmap_range(unsigned long addr, >> + unsigned long end, phys_addr_t phys_addr, pgprot_t prot, >> + unsigned int max_page_shift); > > Drop extern keyword here. I don't know if I was going crazy but at one point I was getting duplicate symbol errors that were fixed by adding extern somewhere. Maybe sleep depravation. However... > As checkpatch tells you, 'CHECK:AVOID_EXTERNS: extern prototypes should > be avoided in .h files' I prefer to follow existing style in surrounding code at the expense of some checkpatch warnings. If somebody later wants to "fix" it that's fine. Thanks, Nick
Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
Le 10/06/2019 à 06:38, Nicholas Piggin a écrit :
ioremap_page_range is a generic function to create a kernel virtual
mapping, move it to mm/vmalloc.c and rename it vmap_range.
For clarity with this move, also:
- Rename vunmap_page_range (vmap_range's inverse) to vunmap_range.
- Rename vmap_page_range (which takes a page array) to vmap_pages.
Maybe it would be easier to follow the change if the name change was
done in another patch than the move.
Signed-off-by: Nicholas Piggin
---
Fixed up the arm64 compile errors, fixed a few bugs, and tidied
things up a bit more.
Have tested powerpc and x86 but not arm64, would appreciate a review
and test of the arm64 patch if possible.
include/linux/vmalloc.h | 3 +
lib/ioremap.c | 173 +++---
mm/vmalloc.c| 228
3 files changed, 229 insertions(+), 175 deletions(-)
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 51e131245379..812bea5866d6 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -147,6 +147,9 @@ extern struct vm_struct *find_vm_area(const void *addr);
extern int map_vm_area(struct vm_struct *area, pgprot_t prot,
struct page **pages);
#ifdef CONFIG_MMU
+extern int vmap_range(unsigned long addr,
+ unsigned long end, phys_addr_t phys_addr, pgprot_t prot,
+ unsigned int max_page_shift);
Drop extern keyword here.
As checkpatch tells you, 'CHECK:AVOID_EXTERNS: extern prototypes should
be avoided in .h files'
Christophe
extern int map_kernel_range_noflush(unsigned long start, unsigned long size,
pgprot_t prot, struct page **pages);
extern void unmap_kernel_range_noflush(unsigned long addr, unsigned long
size);
diff --git a/lib/ioremap.c b/lib/ioremap.c
index 063213685563..e13946da8ec3 100644
--- a/lib/ioremap.c
+++ b/lib/ioremap.c
@@ -58,165 +58,24 @@ static inline int ioremap_pud_enabled(void) { return 0; }
static inline int ioremap_pmd_enabled(void) { return 0; }
#endif/* CONFIG_HAVE_ARCH_HUGE_VMAP */
-static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pte_t *pte;
- u64 pfn;
-
- pfn = phys_addr >> PAGE_SHIFT;
- pte = pte_alloc_kernel(pmd, addr);
- if (!pte)
- return -ENOMEM;
- do {
- BUG_ON(!pte_none(*pte));
- set_pte_at(&init_mm, addr, pte, pfn_pte(pfn, prot));
- pfn++;
- } while (pte++, addr += PAGE_SIZE, addr != end);
- return 0;
-}
-
-static int ioremap_try_huge_pmd(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
-{
- if (!ioremap_pmd_enabled())
- return 0;
-
- if ((end - addr) != PMD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PMD_SIZE))
- return 0;
-
- if (pmd_present(*pmd) && !pmd_free_pte_page(pmd, addr))
- return 0;
-
- return pmd_set_huge(pmd, phys_addr, prot);
-}
-
-static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pmd_t *pmd;
- unsigned long next;
-
- pmd = pmd_alloc(&init_mm, pud, addr);
- if (!pmd)
- return -ENOMEM;
- do {
- next = pmd_addr_end(addr, end);
-
- if (ioremap_try_huge_pmd(pmd, addr, next, phys_addr, prot))
- continue;
-
- if (ioremap_pte_range(pmd, addr, next, phys_addr, prot))
- return -ENOMEM;
- } while (pmd++, phys_addr += (next - addr), addr = next, addr != end);
- return 0;
-}
-
-static int ioremap_try_huge_pud(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
-{
- if (!ioremap_pud_enabled())
- return 0;
-
- if ((end - addr) != PUD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PUD_SIZE))
- return 0;
-
- if (pud_present(*pud) && !pud_free_pmd_page(pud, addr))
- return 0;
-
- return pud_set_huge(pud, phys_addr, prot);
-}
-
-static inline int ioremap_pud_range(p4d_t *p4d, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pud_t *pud;
- unsigned long next;
-
- pud = pud_alloc(&init_mm, p4d, addr);
- if (!pud)
- return -ENOMEM;
- do {
- next = pud_addr_end(addr, end);
-
- if (ioremap_try_huge_pud(pud, addr, next, phys_addr, prot))
- continue;
-
- if (ioremap_pmd_range(pud, addr, next, phys_addr, prot))
-
Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
Anshuman Khandual's on June 10, 2019 3:42 pm: > > > On 06/10/2019 10:08 AM, Nicholas Piggin wrote: >> ioremap_page_range is a generic function to create a kernel virtual >> mapping, move it to mm/vmalloc.c and rename it vmap_range. > > Absolutely. It belongs in mm/vmalloc.c as its a kernel virtual range. > But what is the rationale of changing the name to vmap_range ? Well it doesn't just map IO. It's for arbitrary kernel virtual mapping (including ioremap). Last patch uses it to map regular cacheable memory. >> For clarity with this move, also: >> - Rename vunmap_page_range (vmap_range's inverse) to vunmap_range. > > Will be inverse for both vmap_range() and vmap_page[s]_range() ? Yes. > >> - Rename vmap_page_range (which takes a page array) to vmap_pages. > > s/vmap_pages/vmap_pages_range instead here ^^ Yes. > This deviates from the subject of this patch that it is related to > ioremap only. I believe what this patch intends is to create > > - vunmap_range() takes [VA range] > > This will be the common kernel virtual range tear down > function for ranges created either with vmap_range() or > vmap_pages_range(). Is that correct ? > - vmap_range() takes [VA range, PA range, prot] > - vmap_pages_range() takes [VA range, struct pages, prot] That's right although we already have all those functions, so I don't create anything, only move and re-name. I'm happy to change the subject if you have a preference. > Can we re-order the arguments (pages <--> prot) for vmap_pages_range() > just to make it sync with vmap_range() ? > > static int vmap_pages_range(unsigned long start, unsigned long end, > pgprot_t prot, struct page **pages) > Sure, makes sense. Thanks, Nick
Re: [PATCH 1/4] mm: Move ioremap page table mapping function to mm/
On 06/10/2019 10:08 AM, Nicholas Piggin wrote: > ioremap_page_range is a generic function to create a kernel virtual > mapping, move it to mm/vmalloc.c and rename it vmap_range. Absolutely. It belongs in mm/vmalloc.c as its a kernel virtual range. But what is the rationale of changing the name to vmap_range ? > > For clarity with this move, also: > - Rename vunmap_page_range (vmap_range's inverse) to vunmap_range. Will be inverse for both vmap_range() and vmap_page[s]_range() ? > - Rename vmap_page_range (which takes a page array) to vmap_pages. s/vmap_pages/vmap_pages_range instead here ^^ This deviates from the subject of this patch that it is related to ioremap only. I believe what this patch intends is to create - vunmap_range() takes [VA range] This will be the common kernel virtual range tear down function for ranges created either with vmap_range() or vmap_pages_range(). Is that correct ? - vmap_range() takes [VA range, PA range, prot] - vmap_pages_range() takes [VA range, struct pages, prot] Can we re-order the arguments (pages <--> prot) for vmap_pages_range() just to make it sync with vmap_range() ? static int vmap_pages_range(unsigned long start, unsigned long end, pgprot_t prot, struct page **pages)
[PATCH 1/4] mm: Move ioremap page table mapping function to mm/
ioremap_page_range is a generic function to create a kernel virtual
mapping, move it to mm/vmalloc.c and rename it vmap_range.
For clarity with this move, also:
- Rename vunmap_page_range (vmap_range's inverse) to vunmap_range.
- Rename vmap_page_range (which takes a page array) to vmap_pages.
Signed-off-by: Nicholas Piggin
---
Fixed up the arm64 compile errors, fixed a few bugs, and tidied
things up a bit more.
Have tested powerpc and x86 but not arm64, would appreciate a review
and test of the arm64 patch if possible.
include/linux/vmalloc.h | 3 +
lib/ioremap.c | 173 +++---
mm/vmalloc.c| 228
3 files changed, 229 insertions(+), 175 deletions(-)
diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
index 51e131245379..812bea5866d6 100644
--- a/include/linux/vmalloc.h
+++ b/include/linux/vmalloc.h
@@ -147,6 +147,9 @@ extern struct vm_struct *find_vm_area(const void *addr);
extern int map_vm_area(struct vm_struct *area, pgprot_t prot,
struct page **pages);
#ifdef CONFIG_MMU
+extern int vmap_range(unsigned long addr,
+ unsigned long end, phys_addr_t phys_addr, pgprot_t prot,
+ unsigned int max_page_shift);
extern int map_kernel_range_noflush(unsigned long start, unsigned long size,
pgprot_t prot, struct page **pages);
extern void unmap_kernel_range_noflush(unsigned long addr, unsigned long size);
diff --git a/lib/ioremap.c b/lib/ioremap.c
index 063213685563..e13946da8ec3 100644
--- a/lib/ioremap.c
+++ b/lib/ioremap.c
@@ -58,165 +58,24 @@ static inline int ioremap_pud_enabled(void) { return 0; }
static inline int ioremap_pmd_enabled(void) { return 0; }
#endif /* CONFIG_HAVE_ARCH_HUGE_VMAP */
-static int ioremap_pte_range(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pte_t *pte;
- u64 pfn;
-
- pfn = phys_addr >> PAGE_SHIFT;
- pte = pte_alloc_kernel(pmd, addr);
- if (!pte)
- return -ENOMEM;
- do {
- BUG_ON(!pte_none(*pte));
- set_pte_at(&init_mm, addr, pte, pfn_pte(pfn, prot));
- pfn++;
- } while (pte++, addr += PAGE_SIZE, addr != end);
- return 0;
-}
-
-static int ioremap_try_huge_pmd(pmd_t *pmd, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
-{
- if (!ioremap_pmd_enabled())
- return 0;
-
- if ((end - addr) != PMD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PMD_SIZE))
- return 0;
-
- if (pmd_present(*pmd) && !pmd_free_pte_page(pmd, addr))
- return 0;
-
- return pmd_set_huge(pmd, phys_addr, prot);
-}
-
-static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pmd_t *pmd;
- unsigned long next;
-
- pmd = pmd_alloc(&init_mm, pud, addr);
- if (!pmd)
- return -ENOMEM;
- do {
- next = pmd_addr_end(addr, end);
-
- if (ioremap_try_huge_pmd(pmd, addr, next, phys_addr, prot))
- continue;
-
- if (ioremap_pte_range(pmd, addr, next, phys_addr, prot))
- return -ENOMEM;
- } while (pmd++, phys_addr += (next - addr), addr = next, addr != end);
- return 0;
-}
-
-static int ioremap_try_huge_pud(pud_t *pud, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
-{
- if (!ioremap_pud_enabled())
- return 0;
-
- if ((end - addr) != PUD_SIZE)
- return 0;
-
- if (!IS_ALIGNED(phys_addr, PUD_SIZE))
- return 0;
-
- if (pud_present(*pud) && !pud_free_pmd_page(pud, addr))
- return 0;
-
- return pud_set_huge(pud, phys_addr, prot);
-}
-
-static inline int ioremap_pud_range(p4d_t *p4d, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
-{
- pud_t *pud;
- unsigned long next;
-
- pud = pud_alloc(&init_mm, p4d, addr);
- if (!pud)
- return -ENOMEM;
- do {
- next = pud_addr_end(addr, end);
-
- if (ioremap_try_huge_pud(pud, addr, next, phys_addr, prot))
- continue;
-
- if (ioremap_pmd_range(pud, addr, next, phys_addr, prot))
- return -ENOMEM;
- } while (pud++, phys_addr += (next - addr), addr = next, addr != end);
- return 0;
-}
-
-static int ioremap_try_huge_p4d(p4d_t *p4d, unsigned long addr,
- unsigned long end, phys_addr_t phys_addr,
- pgprot_t prot)
