Messages in this thread | | | Subject | Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part | From | Yang Shi <> | Date | Mon, 6 Aug 2018 09:53:13 -0700 |
| |
On 8/6/18 6:26 AM, Michal Hocko wrote: > On Fri 03-08-18 13:47:19, Yang Shi wrote: >> >> On 8/3/18 1:53 AM, Michal Hocko wrote: >>> On Fri 27-07-18 02:10:13, Yang Shi wrote: >>>> Introduces three new helper functions: >>>> * munmap_addr_sanity() >>>> * munmap_lookup_vma() >>>> * munmap_mlock_vma() >>>> >>>> They will be used by do_munmap() and the new do_munmap with zapping >>>> large mapping early in the later patch. >>>> >>>> There is no functional change, just code refactor. >>>> >>>> Reviewed-by: Laurent Dufour <ldufour@linux.vnet.ibm.com> >>>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>>> --- >>>> mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++-------------------- >>>> 1 file changed, 82 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/mm/mmap.c b/mm/mmap.c >>>> index d1eb87e..2504094 100644 >>>> --- a/mm/mmap.c >>>> +++ b/mm/mmap.c >>>> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma, >>>> return __split_vma(mm, vma, addr, new_below); >>>> } >>>> -/* Munmap is split into 2 main parts -- this part which finds >>>> - * what needs doing, and the areas themselves, which do the >>>> - * work. This now handles partial unmappings. >>>> - * Jeremy Fitzhardinge <jeremy@goop.org> >>>> - */ >>>> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len, >>>> - struct list_head *uf) >>>> +static inline bool munmap_addr_sanity(unsigned long start, size_t len) >>> munmap_check_addr? Btw. why does this need to have munmap prefix at all? >>> This is a general address space check. >> Just because I extracted this from do_munmap, no special consideration. It >> is definitely ok to use another name. >> >>>> { >>>> - unsigned long end; >>>> - struct vm_area_struct *vma, *prev, *last; >>>> - >>>> if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start) >>>> - return -EINVAL; >>>> + return false; >>>> - len = PAGE_ALIGN(len); >>>> - if (len == 0) >>>> - return -EINVAL; >>>> + if (PAGE_ALIGN(len) == 0) >>>> + return false; >>>> + >>>> + return true; >>>> +} >>>> + >>>> +/* >>>> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas. >>>> + * @mm: mm_struct >>>> + * @vma: the first overlapping vma >>>> + * @prev: vma's prev >>>> + * @start: start address >>>> + * @end: end address >>> This really doesn't help me to understand how to use the function. >>> Why do we need both prev and vma etc... >> prev will be used by unmap_region later. > But what does it stand for? Why cannot you take prev from the returned > vma? In other words, if somebody reads this documentation how does he > know what the prev is supposed to be used for? > >>>> + * >>>> + * returns 1 if successful, 0 or errno otherwise >>> This is a really weird calling convention. So what does 0 tell? /me >>> checks the code. Ohh, it is nothing to do. Why cannot you simply return >>> the vma. NULL implies nothing to do, ERR_PTR on error. >> A couple of reasons why it is implemented as so: >> >> * do_munmap returns 0 for both success and no suitable vma >> >> * Since prev is needed by finding the start vma, and prev will be used >> by unmap_region later too, so I just thought it would look clean to have one >> function to return both start vma and prev. In this way, we can share as >> much as possible common code. >> >> * In this way, we just need return 0, 1 or error no just as same as what >> do_munmap does currently. Then we know what is failure case exactly to just >> bail out right away. >> >> Actually, I tried the same approach as you suggested, but it had two >> problems: >> >> * If it returns the start vma, we have to re-find its prev later, but >> the prev has been found during finding start vma. And, duplicate the code in >> do_munmap_zap_rlock. It sounds not that ideal. >> >> * If it returns prev, it might be null (start vma is the first vma). We >> can't tell if null is a failure or success case > Even if you need to return both vma and prev then it would be better to > simply return vma directly than having this -errno, 0 or 1 return > semantic.
OK, I will try to refactor the code.
Thanks, Yang
| |