lkml.org 
[lkml]   [2018]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part
From
Date


On 8/7/18 7:59 AM, Vlastimil Babka wrote:
> On 07/26/2018 08:10 PM, 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)
> Since it's returning bool, the proper naming scheme would be something
> like "munmap_addr_ok()". I don't know how I would replace the "munmap_"
> prefix myself though.

OK, thanks for the suggestion.

>
>> {
>> - 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
>> + *
>> + * returns 1 if successful, 0 or errno otherwise
>> + */
>> +static int munmap_lookup_vma(struct mm_struct *mm, struct vm_area_struct **vma,
>> + struct vm_area_struct **prev, unsigned long start,
>> + unsigned long end)
> Agree with Michal that you could simply return vma, NULL, or error.
> Caller can easily find out prev from that, it's not like we have to
> count each cpu cycle here. It will be a bit less tricky code as well,
> which is a plus.
>
> ...
>> +static inline void munmap_mlock_vma(struct vm_area_struct *vma,
>> + unsigned long end)
> This function does munlock, not mlock. You could call it e.g.
> munlock_vmas().

OK

>
>> +{
>> + struct vm_area_struct *tmp = vma;
>> +
>> + while (tmp && tmp->vm_start < end) {
>> + if (tmp->vm_flags & VM_LOCKED) {
>> + vma->vm_mm->locked_vm -= vma_pages(tmp);
> You keep 'vma' just for the vm_mm? Better extract mm pointer first and
> then you don't need the 'tmp'.

OK

Thanks,
Yang

>
>> + munlock_vma_pages_all(tmp);
>> + }
>> + tmp = tmp->vm_next;
>> + }
>> +}

\
 
 \ /
  Last update: 2018-08-07 20:06    [W:0.083 / U:9.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site