Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part
From: Yang Shi
Date: Mon Aug 06 2018 - 12:53:46 EST
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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
---
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@xxxxxxxx>
- */
-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