Re: [RFC v3 PATCH 3/5] mm: refactor do_munmap() to extract the common part
On 7/2/18 10:58 AM, Michal Hocko wrote:
On Mon 02-07-18 09:59:06, Yang Shi wrote:
On 7/2/18 6:42 AM, Michal Hocko wrote:
On Sat 30-06-18 06:39:43, Yang Shi wrote:
Introduces two new helper functions:
* munmap_addr_sanity()
* munmap_lookup_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.
There are whitespace changes which make the code much harder to review
than necessary.
+static inline bool munmap_addr_sanity(unsigned long start, size_t len)
{
- unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE -
start)
+ return false;
- if ((offset_in_page(start)) || start > TASK_SIZE || len >
TASK_SIZE-start)
- return -EINVAL;
e.g. here.
Oh, yes. I did some coding style cleanup too.
If you want to do some coding style cleanups make them a separate patch.
The resulting diff would be much easier to review.
Sure, thanks
Re: [RFC v3 PATCH 3/5] mm: refactor do_munmap() to extract the common part
On Mon 02-07-18 09:59:06, Yang Shi wrote:
>
>
> On 7/2/18 6:42 AM, Michal Hocko wrote:
> > On Sat 30-06-18 06:39:43, Yang Shi wrote:
> > > Introduces two new helper functions:
> > >* munmap_addr_sanity()
> > >* munmap_lookup_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.
> > There are whitespace changes which make the code much harder to review
> > than necessary.
> > > +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
> > > {
> > > - unsigned long end;
> > > - struct vm_area_struct *vma, *prev, *last;
> > > + if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE -
> > > start)
> > > + return false;
> > > - if ((offset_in_page(start)) || start > TASK_SIZE || len >
> > > TASK_SIZE-start)
> > > - return -EINVAL;
> > e.g. here.
>
> Oh, yes. I did some coding style cleanup too.
If you want to do some coding style cleanups make them a separate patch.
The resulting diff would be much easier to review.
--
Michal Hocko
SUSE Labs
Re: [RFC v3 PATCH 3/5] mm: refactor do_munmap() to extract the common part
On 7/2/18 6:42 AM, Michal Hocko wrote:
On Sat 30-06-18 06:39:43, Yang Shi wrote:
Introduces two new helper functions:
* munmap_addr_sanity()
* munmap_lookup_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.
There are whitespace changes which make the code much harder to review
than necessary.
+static inline bool munmap_addr_sanity(unsigned long start, size_t len)
{
- unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE -
start)
+ return false;
- if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start)
- return -EINVAL;
e.g. here.
Oh, yes. I did some coding style cleanup too.
Re: [RFC v3 PATCH 3/5] mm: refactor do_munmap() to extract the common part
On Sat 30-06-18 06:39:43, Yang Shi wrote:
> Introduces two new helper functions:
> * munmap_addr_sanity()
> * munmap_lookup_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.
There are whitespace changes which make the code much harder to review
than necessary.
> +static inline bool munmap_addr_sanity(unsigned long start, size_t len)
> {
> - unsigned long end;
> - struct vm_area_struct *vma, *prev, *last;
> + if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE -
> start)
> + return false;
>
> - if ((offset_in_page(start)) || start > TASK_SIZE || len >
> TASK_SIZE-start)
> - return -EINVAL;
e.g. here.
--
Michal Hocko
SUSE Labs
[RFC v3 PATCH 3/5] mm: refactor do_munmap() to extract the common part
Introduces two new helper functions:
* munmap_addr_sanity()
* munmap_lookup_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.
Signed-off-by: Yang Shi
---
mm/mmap.c | 107 ++
1 file changed, 72 insertions(+), 35 deletions(-)
diff --git a/mm/mmap.c b/mm/mmap.c
index d1eb87e..87dcf83 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2686,34 +2686,45 @@ 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
- */
-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)
{
- unsigned long end;
- struct vm_area_struct *vma, *prev, *last;
+ if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE -
start)
+ return false;
- if ((offset_in_page(start)) || start > TASK_SIZE || len >
TASK_SIZE-start)
- return -EINVAL;
+ if (PAGE_ALIGN(len) == 0)
+ return false;
- len = PAGE_ALIGN(len);
- if (len == 0)
- return -EINVAL;
+ 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)
+{
+ struct vm_area_struct *tmp, *last;
+ int ret;
/* Find the first overlapping VMA */
- vma = find_vma(mm, start);
- if (!vma)
+ tmp = find_vma(mm, start);
+ if (!tmp)
return 0;
- prev = vma->vm_prev;
- /* we have start < vma->vm_end */
+
+ *prev = tmp->vm_prev;
+
+ /* we have start < vma->vm_end */
/* if it doesn't overlap, we have nothing.. */
- end = start + len;
- if (vma->vm_start >= end)
+ if (tmp->vm_start >= end)
return 0;
/*
@@ -2723,31 +2734,57 @@ int do_munmap(struct mm_struct *mm, unsigned long
start, size_t len,
* unmapped vm_area_struct will remain in use: so lower split_vma
* places tmp vma above, and higher split_vma places tmp vma below.
*/
- if (start > vma->vm_start) {
- int error;
-
+ if (start > tmp->vm_start) {
/*
* Make sure that map_count on return from munmap() will
* not exceed its limit; but let map_count go just above
* its limit temporarily, to help free resources as expected.
*/
- if (end < vma->vm_end && mm->map_count >= sysctl_max_map_count)
+ if (end < tmp->vm_end &&
+ mm->map_count > sysctl_max_map_count)
return -ENOMEM;
- error = __split_vma(mm, vma, start, 0);
- if (error)
- return error;
- prev = vma;
+ ret = __split_vma(mm, tmp, start, 0);
+ if (ret)
+ return ret;
+ *prev = tmp;
}
/* Does it split the last one? */
last = find_vma(mm, end);
if (last && end > last->vm_start) {
- int error = __split_vma(mm, last, end, 1);
- if (error)
- return error;
+ ret = __split_vma(mm, last, end, 1);
+ if (ret)
+ return ret;
}
- vma = prev ? prev->vm_next : mm->mmap;
+
+ *vma = *prev ? (*prev)->vm_next : mm->mmap;
+
+ return 1;
+}
+
+/* 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
+ */
+int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,
+ struct list_head *uf)
+{
+ unsigned long end;
+ struct vm_area_struct *vma = NULL, *prev;
+ int ret = 0;
+
+ if (!munmap_addr_sanity(start, len))
+ return -EINVAL;
+
+ len = PAGE_ALIGN(len);
+
+ end = start + len;
+
+ ret = munmap_lookup_vma(mm, &vma, &prev, start, end);
+ if (ret != 1)
+ return ret;
if (unlikely(uf)) {
/*
@@ -2759,9 +2796,9 @@ int do_munmap(struct mm_struct *mm, unsigne
