| Message ID | 20241211232754.1583023-7-kaleshsingh@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | mm: Introduce arch_mmap_hint() | expand |
* Kalesh Singh <kaleshsingh@google.com> [241211 18:28]: > Introduce csky arch_mmap_hint() and define HAVE_ARCH_MMAP_HINT. > This is a preparatory patch, no functional change is introduced. This also looks like it has changed the validation order and potentially introduced functional changes? All these stem from the same cloned code (sparc32 iirc), but were not updated when the cloned code was updated. This is why I am against arch_* code. We should find a better way to unify the code so that there is nothing different. You seem to have gotten some of the shared code together, but some still exists. In the addresses, there are upper and lower limits, and sometimes "colours". Could we not just define the upper/lower limits in each arch and if colour is used? Maybe this is complicated with 32/64 handled both in the 64 bit code. Is there any plan to unite this code further? We have had errors for many years in cloned but not updated code. I really wish there was more information in the cover letter on what is going on here. I'd like to try and reduce the arch_ code to, basically nothing. I was also disappointed that I wasn't Cc'ed because I've spent a lot of time in this code and this area. I am probably the last one to crawl through and change any of this. > > Signed-off-by: Kalesh Singh <kaleshsingh@google.com> > --- > > Changes in v2: > - MAP_FIXED case is also handled in arch_mmap_hint() since this is just a > special case of the hint addr being "enforced", per Yang Shi. > - Consolidate error handling in arch_mmap_hint(). > > arch/csky/abiv1/inc/abi/pgtable-bits.h | 1 + > arch/csky/abiv1/mmap.c | 68 ++++++++++++++------------ > 2 files changed, 38 insertions(+), 31 deletions(-) > > diff --git a/arch/csky/abiv1/inc/abi/pgtable-bits.h b/arch/csky/abiv1/inc/abi/pgtable-bits.h > index ae7a2f76dd42..c346a9fcb522 100644 > --- a/arch/csky/abiv1/inc/abi/pgtable-bits.h > +++ b/arch/csky/abiv1/inc/abi/pgtable-bits.h > @@ -51,5 +51,6 @@ > ((offset) << 10)}) > > #define HAVE_ARCH_UNMAPPED_AREA > +#define HAVE_ARCH_MMAP_HINT > > #endif /* __ASM_CSKY_PGTABLE_BITS_H */ > diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c > index 1047865e82a9..0c5c51a081e4 100644 > --- a/arch/csky/abiv1/mmap.c > +++ b/arch/csky/abiv1/mmap.c > @@ -13,6 +13,39 @@ > ((((addr)+SHMLBA-1)&~(SHMLBA-1)) + \ > (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1))) > > +unsigned long arch_mmap_hint(struct file *filp, unsigned long addr, > + unsigned long len, unsigned long pgoff, > + unsigned long flags) > +{ > + bool do_align; > + > + if (len > TASK_SIZE) > + return -ENOMEM; > + > + /* > + * We only need to do colour alignment if either the I or D > + * caches alias. > + */ > + do_align = filp || (flags & MAP_SHARED); > + > + /* > + * We enforce the MAP_FIXED case. > + */ > + if (flags & MAP_FIXED) { > + if (flags & MAP_SHARED && > + (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)) > + return -EINVAL; > + return addr; > + } > + > + if (do_align) > + addr = COLOUR_ALIGN(addr, pgoff); > + else > + addr = PAGE_ALIGN(addr); > + > + return generic_mmap_hint(filp, addr, len, pgoff, flags); > +} > + > /* > * We need to ensure that shared mappings are correctly aligned to > * avoid aliasing issues with VIPT caches. We need to ensure that > @@ -27,8 +60,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, > unsigned long flags, vm_flags_t vm_flags) > { > struct mm_struct *mm = current->mm; > - struct vm_area_struct *vma; > - int do_align = 0; > + bool do_align; > struct vm_unmapped_area_info info = { > .length = len, > .low_limit = mm->mmap_base, > @@ -36,37 +68,11 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, > .align_offset = pgoff << PAGE_SHIFT > }; > > - /* > - * We only need to do colour alignment if either the I or D > - * caches alias. > - */ This seems like useful information to keep around? > - do_align = filp || (flags & MAP_SHARED); > - > - /* > - * We enforce the MAP_FIXED case. > - */ > - if (flags & MAP_FIXED) { > - if (flags & MAP_SHARED && > - (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)) > - return -EINVAL; > + addr = arch_mmap_hint(filp, addr, len, pgoff, flags); > + if (addr) > return addr; > - } > - > - if (len > TASK_SIZE) > - return -ENOMEM; > - > - if (addr) { > - if (do_align) > - addr = COLOUR_ALIGN(addr, pgoff); > - else > - addr = PAGE_ALIGN(addr); > - > - vma = find_vma(mm, addr); > - if (TASK_SIZE - len >= addr && > - (!vma || addr + len <= vm_start_gap(vma))) > - return addr; > - } > > + do_align = filp || (flags & MAP_SHARED); > info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0; > return vm_unmapped_area(&info); > } > -- > 2.47.0.338.g60cca15819-goog > >
On Thu, 12 Dec 2024 16:40:10 -0500 "Liam R. Howlett" <Liam.Howlett@oracle.com> wrote: > * Kalesh Singh <kaleshsingh@google.com> [241211 18:28]: > > Introduce csky arch_mmap_hint() and define HAVE_ARCH_MMAP_HINT. > > This is a preparatory patch, no functional change is introduced. > > This also looks like it has changed the validation order and potentially > introduced functional changes? > > All these stem from the same cloned code (sparc32 iirc), but were not > updated when the cloned code was updated. This is why I am against > arch_* code. We should find a better way to unify the code so that > there is nothing different. You seem to have gotten some of the shared > code together, but some still exists. > > In the addresses, there are upper and lower limits, and sometimes > "colours". Could we not just define the upper/lower limits in each arch > and if colour is used? Maybe this is complicated with 32/64 handled > both in the 64 bit code. > > Is there any plan to unite this code further? > > We have had errors for many years in cloned but not updated code. I > really wish there was more information in the cover letter on what is > going on here. I'd like to try and reduce the arch_ code to, basically > nothing. > > I was also disappointed that I wasn't Cc'ed because I've spent a lot of > time in this code and this area. I am probably the last one to crawl > through and change any of this. Thanks, I removed this version of this series from mm-unstable.
diff --git a/arch/csky/abiv1/inc/abi/pgtable-bits.h b/arch/csky/abiv1/inc/abi/pgtable-bits.h index ae7a2f76dd42..c346a9fcb522 100644 --- a/arch/csky/abiv1/inc/abi/pgtable-bits.h +++ b/arch/csky/abiv1/inc/abi/pgtable-bits.h @@ -51,5 +51,6 @@ ((offset) << 10)}) #define HAVE_ARCH_UNMAPPED_AREA +#define HAVE_ARCH_MMAP_HINT #endif /* __ASM_CSKY_PGTABLE_BITS_H */ diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c index 1047865e82a9..0c5c51a081e4 100644 --- a/arch/csky/abiv1/mmap.c +++ b/arch/csky/abiv1/mmap.c @@ -13,6 +13,39 @@ ((((addr)+SHMLBA-1)&~(SHMLBA-1)) + \ (((pgoff)<<PAGE_SHIFT) & (SHMLBA-1))) +unsigned long arch_mmap_hint(struct file *filp, unsigned long addr, + unsigned long len, unsigned long pgoff, + unsigned long flags) +{ + bool do_align; + + if (len > TASK_SIZE) + return -ENOMEM; + + /* + * We only need to do colour alignment if either the I or D + * caches alias. + */ + do_align = filp || (flags & MAP_SHARED); + + /* + * We enforce the MAP_FIXED case. + */ + if (flags & MAP_FIXED) { + if (flags & MAP_SHARED && + (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)) + return -EINVAL; + return addr; + } + + if (do_align) + addr = COLOUR_ALIGN(addr, pgoff); + else + addr = PAGE_ALIGN(addr); + + return generic_mmap_hint(filp, addr, len, pgoff, flags); +} + /* * We need to ensure that shared mappings are correctly aligned to * avoid aliasing issues with VIPT caches. We need to ensure that @@ -27,8 +60,7 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, unsigned long flags, vm_flags_t vm_flags) { struct mm_struct *mm = current->mm; - struct vm_area_struct *vma; - int do_align = 0; + bool do_align; struct vm_unmapped_area_info info = { .length = len, .low_limit = mm->mmap_base, @@ -36,37 +68,11 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr, .align_offset = pgoff << PAGE_SHIFT }; - /* - * We only need to do colour alignment if either the I or D - * caches alias. - */ - do_align = filp || (flags & MAP_SHARED); - - /* - * We enforce the MAP_FIXED case. - */ - if (flags & MAP_FIXED) { - if (flags & MAP_SHARED && - (addr - (pgoff << PAGE_SHIFT)) & (SHMLBA - 1)) - return -EINVAL; + addr = arch_mmap_hint(filp, addr, len, pgoff, flags); + if (addr) return addr; - } - - if (len > TASK_SIZE) - return -ENOMEM; - - if (addr) { - if (do_align) - addr = COLOUR_ALIGN(addr, pgoff); - else - addr = PAGE_ALIGN(addr); - - vma = find_vma(mm, addr); - if (TASK_SIZE - len >= addr && - (!vma || addr + len <= vm_start_gap(vma))) - return addr; - } + do_align = filp || (flags & MAP_SHARED); info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0; return vm_unmapped_area(&info); }
Introduce csky arch_mmap_hint() and define HAVE_ARCH_MMAP_HINT. This is a preparatory patch, no functional change is introduced. Signed-off-by: Kalesh Singh <kaleshsingh@google.com> --- Changes in v2: - MAP_FIXED case is also handled in arch_mmap_hint() since this is just a special case of the hint addr being "enforced", per Yang Shi. - Consolidate error handling in arch_mmap_hint(). arch/csky/abiv1/inc/abi/pgtable-bits.h | 1 + arch/csky/abiv1/mmap.c | 68 ++++++++++++++------------ 2 files changed, 38 insertions(+), 31 deletions(-)