Skip to contentSkip to conversation

Bug #3054 madvise(MADV_WILLNEED,...) fix for exceeding rlimit rss

PATCH In review Matt Keenan 2 participants 3 messages Jun 19, 2005

Subsystems (2)
Memory management madvise
lore
Share & tools
lore

Matt Keenan

author
#0 lore raw

PATCH 1/1 Bug #3054 madvise(MADV_WILLNEED,...) fix for exceeding rlimit rss

Here is an updated patch for 2.6.12, can it be included in -mm for testing? I have been banging on the code for an hour or so now; no probs.

Matt

mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +0 -0
@@ -61,6 +61,7 @@ static long madvise_willneed(struct vm_a
                            unsigned long start, unsigned long end)

{
       struct file *file = vma->vm_file;

+ struct task_struct *tsk = current;

       if (!file)
               return -EBADF;
@@ -70,6 +71,28 @@ static long madvise_willneed(struct vm_a
               end = vma->vm_end;
       end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;

+ /*

+        * This code below checks to see if mapping the requested
+        * readahead would make the task's rss exceed the task's
+        * rlimit rss.
+        *
+        * This doesn't account for pages that may already be mapped
+        * due to readahead, but since this is merely a hint to the
+        * kernel no harm should be done, it won't unmap anything
+        * already mapped if it fails. N.B. This won't affect the
+        * kernel's internal automatic readahead which doesn't check
+        * (or honour) rlimit rss.
+        */

+
+ spin_lock(&tsk->mm->page_table_lock);
+ if (((max_sane_readahead(end-start) << PAGE_SHIFT) +
+ tsk->mm->_rss) > tsk->signal->rlim[RLIMIT_RSS].rlim_cur)
+ {
+ spin_unlock(&tsk->mm->page_table_lock);
+ return -EIO;
+ }
+ spin_unlock(&tsk->mm->page_table_lock);
+

       force_page_cache_readahead(file->f_mapping,
                       file, start, max_sane_readahead(end - start));
       return 0;

@@ -170,6 +193,8 @@ static long madvise_vma(struct vm_area_s
* -ENOMEM - addresses in the specified range are not currently
* mapped, or are outside the AS of the process.
* -EIO - an I/O error occurred while paging in data.
+ * - MADV_WILLNEED would map in pages that would make the task's
+ * rss exceed rlimit rss.
* -EBADF - map exists, but area maps something that isn't a file.
* -EAGAIN - a kernel resource was temporarily unavailable. */

#1 lore raw

mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 static long madvise_willneed(struct vm_a · line 64

Quoted 7 lines · * Matt Keenan ([email protected]) wrote:
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +1 -0
@@ -61,6 +61,7 @@ static long madvise_willneed(struct vm_a
                            unsigned long start, unsigned long end)
 {
       struct file *file = vma->vm_file;
+       struct task_struct *tsk = current;

Looks like you've got some tab/whitespace damage going on here.

Quoted 12 lines
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +6 -0
       if (!file)
               return -EBADF;

@@ -70,6 +71,28 @@ static long madvise_willneed(struct vm_a
               end = vma->vm_end;
       end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
+       /*
+        * This code below checks to see if mapping the requested
+        * readahead would make the task's rss exceed the task's
+        * rlimit rss.
+        *
+        * This doesn't account for pages that may already be mapped

mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 static long madvise_willneed(struct vm_a · line 89

Quoted 10 lines
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +10 -0
+        * due to readahead, but since this is merely a hint to the
+        * kernel no harm should be done, it won't unmap anything
+        * already mapped if it fails. N.B. This won't affect the
+        * kernel's internal automatic readahead which doesn't check
+        * (or honour) rlimit rss.
+        */
+
+       spin_lock(&tsk->mm->page_table_lock);
+       if (((max_sane_readahead(end-start) << PAGE_SHIFT) +
+           tsk->mm->_rss) > tsk->signal->rlim[RLIMIT_RSS].rlim_cur)

I doubt this one would overflow, but we recenly made changes in similar tests to use page count rather than byte count. I belive this should use get_mm_counter(). Isn't _rss counting pages rather than bytes, so I think the math is off here. Something like:

	if ((max_sane_readahead(end - start) + get_mm_counter(tsk->mm, rss)) >
	    tsk->signal->rlim[RLIMIT_RSS].rlim_cur >> PAGE_SHIFT)

thanks,
-chris

author
#2 lore raw
Quoted 17 lines · Chris Wright

mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 static long madvise_willneed(struct vm_a · line 66

Quoted 9 lines · * Matt Keenan ([email protected]) wrote:
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +1 -0
@@ -61,6 +61,7 @@ static long madvise_willneed(struct vm_a
                           unsigned long start, unsigned long end)
 {
      struct file *file = vma->vm_file;
+       struct task_struct *tsk = current;

Looks like you've got some tab/whitespace damage going on here.

Damn mailer! I might have to go back to mutt.

Quoted 30 lines
Quoted 14 lines
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +0 -0
      if (!file)

              return -EBADF;
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +8 -0
@@ -70,6 +71,28 @@ static long madvise_willneed(struct vm_a
              end = vma->vm_end;
      end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
+       /*
+        * This code below checks to see if mapping the requested
+        * readahead would make the task's rss exceed the task's
+        * rlimit rss.
+        *
+        * This doesn't account for pages that may already be mapped
+        * due to readahead, but since this is merely a hint to the
+        * kernel no harm should be done, it won't unmap anything

mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 static long madvise_willneed(struct vm_a · line 91

Quoted 10 lines
mm/madvise.c 2005-06-19 17:20:56.000000000 +0100 renamed from mm/madvise.c 2005-04-12 15:58:30.000000000 +0100 +8 -0
+        * already mapped if it fails. N.B. This won't affect the
+        * kernel's internal automatic readahead which doesn't check
+        * (or honour) rlimit rss.
+        */
+
+       spin_lock(&tsk->mm->page_table_lock);
+       if (((max_sane_readahead(end-start) << PAGE_SHIFT) +
+           tsk->mm->_rss) > tsk->signal->rlim[RLIMIT_RSS].rlim_cur)

I doubt this one would overflow, but we recenly made changes in similar tests to use page count rather than byte count. I belive this should use get_mm_counter(). Isn't _rss counting pages rather than bytes, so I think the math is off here. Something like:

Quoted 5 lines
	if ((max_sane_readahead(end - start) + get_mm_counter(tsk->mm, rss)) >
	    tsk->signal->rlim[RLIMIT_RSS].rlim_cur >> PAGE_SHIFT)

Ok, here is the patch again with the suggested fixes. I have attached the patch (yes yes i know!), that will probably screw up people's mail -> patch generators, but the whitespace issue should be fixed. Hopefully this is ok, I'm going to bang on it for an hour or so here to make sure.

Signed-off-by Matthew Keenan <[email protected]>
mm/madvise.c +25 -0
@@ -61,6 +61,7 @@ static long madvise_willneed(struct vm_a
 			     unsigned long start, unsigned long end)
 {
 	struct file *file = vma->vm_file;
+	struct task_struct *tsk = current;
 
 	if (!file)
 		return -EBADF;

@@ -70,6 +71,28 @@ static long madvise_willneed(struct vm_a
 		end = vma->vm_end;
 	end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
 
+	/*
+	 * This code below checks to see if mapping the requested
+	 * readahead would make the task's rss exceed the task's
+	 * rlimit rss.
+	 *
+	 * This doesn't account for pages that may already be mapped
+	 * due to readahead, but since this is merely a hint to the
+	 * kernel no harm should be done, it won't unmap anything
+	 * already mapped if it fails. N.B. This won't affect the
+	 * kernel's internal automatic readahead which doesn't check
+	 * (or honour) rlimit rss.
+	 */
+
+	spin_lock(&tsk->mm->page_table_lock);
+	if ((max_sane_readahead(end - start) + get_mm_counter(tsk->mm, rss)) >
+	    (tsk->signal->rlim[RLIMIT_RSS].rlim_cur >> PAGE_SHIFT))
+	{
+		spin_unlock(&tsk->mm->page_table_lock);
+		return -EIO;
+	}
+	spin_unlock(&tsk->mm->page_table_lock);
+
 	force_page_cache_readahead(file->f_mapping,
 			file, start, max_sane_readahead(end - start));
 	return 0;

@@ -170,6 +193,8 @@ static long madvise_vma(struct vm_area_s
  *  -ENOMEM - addresses in the specified range are not currently
  *		mapped, or are outside the AS of the process.
  *  -EIO    - an I/O error occurred while paging in data.
+ *          - MADV_WILLNEED would map in pages that would make the task's
+ *              rss exceed rlimit rss.
  *  -EBADF  - map exists, but area maps something that isn't a file.
  *  -EAGAIN - a kernel resource was temporarily unavailable.
  */