linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] guard mm->rss with page_table_lock (241p11)
@ 2001-01-29 21:23 Rasmus Andersen
  2001-01-29 21:30 ` Rik van Riel
  0 siblings, 1 reply; 15+ messages in thread
From: Rasmus Andersen @ 2001-01-29 21:23 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel, linux-mm

Hi.

This patch tries to fix the potential rss accounting race where we
fiddle with mm->rss without holding the page_table_lock.

In addition to the places this patch touches there are some places
in fs/ where mm->rss is touched. But I am not sure of the finer
points of this code, so perhaps somebody else could have a look?
The files are binfmt_aout.c, binfmt_elf.c, and exec.c.

It applies against ac12 and 241p11.


Please comment. Or else I will continue to sumbit it :)



diff -aur linux-2.4.1-pre11-clean/mm/memory.c linux/mm/memory.c
--- linux-2.4.1-pre11-clean/mm/memory.c	Sun Jan 28 20:53:13 2001
+++ linux/mm/memory.c	Sun Jan 28 22:43:04 2001
@@ -377,7 +377,6 @@
 		address = (address + PGDIR_SIZE) & PGDIR_MASK;
 		dir++;
 	} while (address && (address < end));
-	spin_unlock(&mm->page_table_lock);
 	/*
 	 * Update rss for the mm_struct (not necessarily current->mm)
 	 * Notice that rss is an unsigned long.
@@ -386,6 +385,7 @@
 		mm->rss -= freed;
 	else
 		mm->rss = 0;
+	spin_unlock(&mm->page_table_lock);
 }
 
 
@@ -1038,7 +1038,9 @@
 		flush_icache_page(vma, page);
 	}
 
+	spin_lock(&mm->page_table_lock);
 	mm->rss++;
+	spin_unlock(&mm->page_table_lock);
 
 	pte = mk_pte(page, vma->vm_page_prot);
 
@@ -1072,7 +1074,9 @@
 			return -1;
 		clear_user_highpage(page, addr);
 		entry = pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
+		spin_lock(&mm->page_table_lock);
 		mm->rss++;
+		spin_unlock(&mm->page_table_lock);
 		flush_page_to_ram(page);
 	}
 	set_pte(page_table, entry);
@@ -1111,7 +1115,9 @@
 		return 0;
 	if (new_page == NOPAGE_OOM)
 		return -1;
+	spin_lock(&mm->page_table_lock);
 	++mm->rss;
+	spin_unlock(&mm->page_table_lock);
 	/*
 	 * This silly early PAGE_DIRTY setting removes a race
 	 * due to the bad i386 page protection. But it's valid
diff -aur linux-2.4.1-pre11-clean/mm/mmap.c linux/mm/mmap.c
--- linux-2.4.1-pre11-clean/mm/mmap.c	Sat Dec 30 18:35:19 2000
+++ linux/mm/mmap.c	Sun Jan 28 22:43:04 2001
@@ -879,8 +879,8 @@
 	spin_lock(&mm->page_table_lock);
 	mpnt = mm->mmap;
 	mm->mmap = mm->mmap_avl = mm->mmap_cache = NULL;
-	spin_unlock(&mm->page_table_lock);
 	mm->rss = 0;
+	spin_unlock(&mm->page_table_lock);
 	mm->total_vm = 0;
 	mm->locked_vm = 0;
 	while (mpnt) {
diff -aur linux-2.4.1-pre11-clean/mm/swapfile.c linux/mm/swapfile.c
--- linux-2.4.1-pre11-clean/mm/swapfile.c	Fri Dec 29 23:07:24 2000
+++ linux/mm/swapfile.c	Sun Jan 28 22:43:04 2001
@@ -231,7 +231,9 @@
 	set_pte(dir, pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
 	swap_free(entry);
 	get_page(page);
+	spin_lock(&vma->vm_mm->page_table_lock);
 	++vma->vm_mm->rss;
+	spin_unlock(&vma->vm_mm->page_table_lock);
 }
 
 static inline void unuse_pmd(struct vm_area_struct * vma, pmd_t *dir,
diff -aur linux-2.4.1-pre11-clean/mm/vmscan.c linux/mm/vmscan.c
--- linux-2.4.1-pre11-clean/mm/vmscan.c	Sun Jan 28 20:53:13 2001
+++ linux/mm/vmscan.c	Mon Jan 29 22:09:18 2001
@@ -72,7 +72,9 @@
 		swap_duplicate(entry);
 		set_pte(page_table, swp_entry_to_pte(entry));
 drop_pte:
+		spin_lock(&mm->page_table_lock);
 		mm->rss--;
+		spin_unlock(&mm->page_table_lock);
 		if (!page->age)
 			deactivate_page(page);
 		UnlockPage(page);

-- 
Regards,
        Rasmus(rasmus@jaquet.dk)

I've never had major knee surgery on any other part of my body.
-Winston Bennett, University of Kentucky basketball forward
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-29 21:23 [PATCH] guard mm->rss with page_table_lock (241p11) Rasmus Andersen
@ 2001-01-29 21:30 ` Rik van Riel
  2001-01-29 21:43   ` Rasmus Andersen
  0 siblings, 1 reply; 15+ messages in thread
From: Rik van Riel @ 2001-01-29 21:30 UTC (permalink / raw)
  To: Rasmus Andersen; +Cc: torvalds, linux-kernel, linux-mm

On Mon, 29 Jan 2001, Rasmus Andersen wrote:

> Please comment. Or else I will continue to sumbit it :)

The following will hang the kernel on SMP, since you're
already holding the spinlock here. Try compiling with
CONFIG_SMP and see what happens...

> diff -aur linux-2.4.1-pre11-clean/mm/vmscan.c linux/mm/vmscan.c
> --- linux-2.4.1-pre11-clean/mm/vmscan.c	Sun Jan 28 20:53:13 2001
> +++ linux/mm/vmscan.c	Mon Jan 29 22:09:18 2001
> @@ -72,7 +72,9 @@
>  		swap_duplicate(entry);
>  		set_pte(page_table, swp_entry_to_pte(entry));
>  drop_pte:
> +		spin_lock(&mm->page_table_lock);
>  		mm->rss--;
> +		spin_unlock(&mm->page_table_lock);
>  		if (!page->age)
>  			deactivate_page(page);
>  		UnlockPage(page);

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com.br/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-29 21:30 ` Rik van Riel
@ 2001-01-29 21:43   ` Rasmus Andersen
  2001-01-29 21:47     ` Rik van Riel
                       ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Rasmus Andersen @ 2001-01-29 21:43 UTC (permalink / raw)
  To: Rik van Riel; +Cc: torvalds, linux-kernel, linux-mm

On Mon, Jan 29, 2001 at 07:30:01PM -0200, Rik van Riel wrote:
> On Mon, 29 Jan 2001, Rasmus Andersen wrote:
> 
> > Please comment. Or else I will continue to sumbit it :)
> 
> The following will hang the kernel on SMP, since you're
> already holding the spinlock here. Try compiling with
> CONFIG_SMP and see what happens...

You are right. Sloppy research by me :(

New patch below with the vmscan part removed.


diff -aur linux-2.4.1-pre11-clean/mm/memory.c linux/mm/memory.c
--- linux-2.4.1-pre11-clean/mm/memory.c	Sun Jan 28 20:53:13 2001
+++ linux/mm/memory.c	Sun Jan 28 22:43:04 2001
@@ -377,7 +377,6 @@
 		address = (address + PGDIR_SIZE) & PGDIR_MASK;
 		dir++;
 	} while (address && (address < end));
-	spin_unlock(&mm->page_table_lock);
 	/*
 	 * Update rss for the mm_struct (not necessarily current->mm)
 	 * Notice that rss is an unsigned long.
@@ -386,6 +385,7 @@
 		mm->rss -= freed;
 	else
 		mm->rss = 0;
+	spin_unlock(&mm->page_table_lock);
 }
 
 
@@ -1038,7 +1038,9 @@
 		flush_icache_page(vma, page);
 	}
 
+	spin_lock(&mm->page_table_lock);
 	mm->rss++;
+	spin_unlock(&mm->page_table_lock);
 
 	pte = mk_pte(page, vma->vm_page_prot);
 
@@ -1072,7 +1074,9 @@
 			return -1;
 		clear_user_highpage(page, addr);
 		entry = pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
+		spin_lock(&mm->page_table_lock);
 		mm->rss++;
+		spin_unlock(&mm->page_table_lock);
 		flush_page_to_ram(page);
 	}
 	set_pte(page_table, entry);
@@ -1111,7 +1115,9 @@
 		return 0;
 	if (new_page == NOPAGE_OOM)
 		return -1;
+	spin_lock(&mm->page_table_lock);
 	++mm->rss;
+	spin_unlock(&mm->page_table_lock);
 	/*
 	 * This silly early PAGE_DIRTY setting removes a race
 	 * due to the bad i386 page protection. But it's valid
diff -aur linux-2.4.1-pre11-clean/mm/mmap.c linux/mm/mmap.c
--- linux-2.4.1-pre11-clean/mm/mmap.c	Sat Dec 30 18:35:19 2000
+++ linux/mm/mmap.c	Sun Jan 28 22:43:04 2001
@@ -879,8 +879,8 @@
 	spin_lock(&mm->page_table_lock);
 	mpnt = mm->mmap;
 	mm->mmap = mm->mmap_avl = mm->mmap_cache = NULL;
-	spin_unlock(&mm->page_table_lock);
 	mm->rss = 0;
+	spin_unlock(&mm->page_table_lock);
 	mm->total_vm = 0;
 	mm->locked_vm = 0;
 	while (mpnt) {
diff -aur linux-2.4.1-pre11-clean/mm/swapfile.c linux/mm/swapfile.c
--- linux-2.4.1-pre11-clean/mm/swapfile.c	Fri Dec 29 23:07:24 2000
+++ linux/mm/swapfile.c	Sun Jan 28 22:43:04 2001
@@ -231,7 +231,9 @@
 	set_pte(dir, pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
 	swap_free(entry);
 	get_page(page);
+	spin_lock(&vma->vm_mm->page_table_lock);
 	++vma->vm_mm->rss;
+	spin_unlock(&vma->vm_mm->page_table_lock);
 }
 
 static inline void unuse_pmd(struct vm_area_struct * vma, pmd_t *dir,

-- 
        Rasmus(rasmus@jaquet.dk)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-29 21:43   ` Rasmus Andersen
@ 2001-01-29 21:47     ` Rik van Riel
  2001-01-30  8:18     ` David Howells
  2001-02-13  3:15     ` george anzinger
  2 siblings, 0 replies; 15+ messages in thread
From: Rik van Riel @ 2001-01-29 21:47 UTC (permalink / raw)
  To: Rasmus Andersen; +Cc: torvalds, linux-kernel, linux-mm

On Mon, 29 Jan 2001, Rasmus Andersen wrote:
> On Mon, Jan 29, 2001 at 07:30:01PM -0200, Rik van Riel wrote:
> > On Mon, 29 Jan 2001, Rasmus Andersen wrote:
> > 
> > > Please comment. Or else I will continue to sumbit it :)
> > 
> > The following will hang the kernel on SMP, since you're
> > already holding the spinlock here. Try compiling with
> > CONFIG_SMP and see what happens...
> 
> You are right. Sloppy research by me :(
> 
> New patch below with the vmscan part removed.

Have you bothered to also check the rest?

Don't be surprised if there are more places
like this. Please compile your kernel with
CONFIG_SMP and test if your changes work
before submitting them into the stable kernel.

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com.br/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-29 21:43   ` Rasmus Andersen
  2001-01-29 21:47     ` Rik van Riel
@ 2001-01-30  8:18     ` David Howells
  2001-01-30  8:31       ` Rasmus Andersen
  2001-01-30  8:39       ` David S. Miller
  2001-02-13  3:15     ` george anzinger
  2 siblings, 2 replies; 15+ messages in thread
From: David Howells @ 2001-01-30  8:18 UTC (permalink / raw)
  To: Rasmus Andersen; +Cc: Rik van Riel, linux-kernel, linux-mm

>...
> +	spin_lock(&mm->page_table_lock);
>  	mm->rss++;
> +	spin_unlock(&mm->page_table_lock);
>...

Would it not be better to use some sort of atomic add/subtract/clear operation
rather than a spinlock? (Which would also give you fewer atomic memory access
cycles).

David
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30  8:18     ` David Howells
@ 2001-01-30  8:31       ` Rasmus Andersen
  2001-01-30 14:32         ` Mark Hahn
  2001-01-30  8:39       ` David S. Miller
  1 sibling, 1 reply; 15+ messages in thread
From: Rasmus Andersen @ 2001-01-30  8:31 UTC (permalink / raw)
  To: David Howells; +Cc: Rik van Riel, linux-kernel, linux-mm

On Tue, Jan 30, 2001 at 08:18:56AM +0000, David Howells wrote:
> >...
> > +	spin_lock(&mm->page_table_lock);
> >  	mm->rss++;
> > +	spin_unlock(&mm->page_table_lock);
> >...
> 
> Would it not be better to use some sort of atomic add/subtract/clear operation
> rather than a spinlock? (Which would also give you fewer atomic memory access
> cycles).

This will unfortunately not do for all platforms. Please read
http://marc.theaimsgroup.com/?t=97630768100003&w=2&r=1 for the
last discussion of this.

Regards,
  Rasmus
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30  8:18     ` David Howells
  2001-01-30  8:31       ` Rasmus Andersen
@ 2001-01-30  8:39       ` David S. Miller
  2001-01-30 11:17         ` Chris Wedgwood
  1 sibling, 1 reply; 15+ messages in thread
From: David S. Miller @ 2001-01-30  8:39 UTC (permalink / raw)
  To: David Howells; +Cc: Rasmus Andersen, Rik van Riel, linux-kernel, linux-mm

David Howells writes:
 > Would it not be better to use some sort of atomic add/subtract/clear operation
 > rather than a spinlock? (Which would also give you fewer atomic memory access
 > cycles).

Please see older threads about this, it has been discussed to death
already (hint: sizeof(atomic_t), sizeof(unsigned long)).

Later,
David S. Miller
davem@redhat.com
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30  8:39       ` David S. Miller
@ 2001-01-30 11:17         ` Chris Wedgwood
  2001-01-30 11:23           ` Rik van Riel
  0 siblings, 1 reply; 15+ messages in thread
From: Chris Wedgwood @ 2001-01-30 11:17 UTC (permalink / raw)
  To: David S. Miller
  Cc: David Howells, Rasmus Andersen, Rik van Riel, linux-kernel, linux-mm

On Tue, Jan 30, 2001 at 12:39:24AM -0800, David S. Miller wrote:

    Please see older threads about this, it has been discussed to
    death already (hint: sizeof(atomic_t), sizeof(unsigned long)).

can we not define a macro so architectures that can do do atomically
inc/dec with unsigned long will? otherwise it uses the spinlock?


  --cw
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30 11:17         ` Chris Wedgwood
@ 2001-01-30 11:23           ` Rik van Riel
  2001-01-30 11:38             ` Rasmus Andersen
  0 siblings, 1 reply; 15+ messages in thread
From: Rik van Riel @ 2001-01-30 11:23 UTC (permalink / raw)
  To: Chris Wedgwood
  Cc: David S. Miller, David Howells, Rasmus Andersen, linux-kernel, linux-mm

On Wed, 31 Jan 2001, Chris Wedgwood wrote:
> On Tue, Jan 30, 2001 at 12:39:24AM -0800, David S. Miller wrote:
> 
>     Please see older threads about this, it has been discussed to
>     death already (hint: sizeof(atomic_t), sizeof(unsigned long)).
> 
> can we not define a macro so architectures that can do do atomically
> inc/dec with unsigned long will? otherwise it uses the spinlock?

Why bother ?

In most places where we update mm->rss, we are *already*
holding the spinlock anyway, this correction is just for
a few places.

The big patch Rasmus made seems to contain spin_lock(&foo)
in places where we already have the lock, leading to
instant SMP deadlock. I suspect Rasmus' patch should be
about half the size it is currently...

regards,

Rik
--
Virtual memory is like a game you can't win;
However, without VM there's truly nothing to lose...

		http://www.surriel.com/
http://www.conectiva.com/	http://distro.conectiva.com.br/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30 11:23           ` Rik van Riel
@ 2001-01-30 11:38             ` Rasmus Andersen
  0 siblings, 0 replies; 15+ messages in thread
From: Rasmus Andersen @ 2001-01-30 11:38 UTC (permalink / raw)
  To: Rik van Riel
  Cc: Chris Wedgwood, David S. Miller, David Howells, linux-kernel, linux-mm

On Tue, Jan 30, 2001 at 09:23:27AM -0200, Rik van Riel wrote:
> Why bother ?
> 
> In most places where we update mm->rss, we are *already*
> holding the spinlock anyway, this correction is just for
> a few places.
> 
> The big patch Rasmus made seems to contain spin_lock(&foo)
> in places where we already have the lock, leading to
> instant SMP deadlock. I suspect Rasmus' patch should be
> about half the size it is currently...

After donning my brown paper bag yesterday I looked at 
the call-paths again and removed one more lock pair
(the one in swapfile). The others seemed OK so I made 
a SMP-on-UP kernel and ran my usual stuff (X, mozilla, 
kernel compiles) alongside mmap001, mmap002 and misc001
with no ill effects.

I will beat on it some more today and tomorrow, but if
real SMP is needed for testing I need some help to do
that.


Regards,
   Rasmus 
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30  8:31       ` Rasmus Andersen
@ 2001-01-30 14:32         ` Mark Hahn
  2001-01-30 15:30           ` Eric W. Biederman
  0 siblings, 1 reply; 15+ messages in thread
From: Mark Hahn @ 2001-01-30 14:32 UTC (permalink / raw)
  To: Rasmus Andersen; +Cc: linux-mm

> > > +	spin_lock(&mm->page_table_lock);
> > >  	mm->rss++;
> > > +	spin_unlock(&mm->page_table_lock);
> > >...
> > 
> > Would it not be better to use some sort of atomic add/subtract/clear operation
> > rather than a spinlock? (Which would also give you fewer atomic memory access
> > cycles).
> 
> This will unfortunately not do for all platforms. Please read
> http://marc.theaimsgroup.com/?t=97630768100003&w=2&r=1 for the
> last discussion of this.

which can be summarized as "yet another way sparc support screws Linux,
and DMiller didn't want to fix his mess close to 2.4.0".  it's ridiculous
for an inconsequential arch like sparc32 to cause fairly noticable problems
for all other arches.

if noone beats me, I'll be submitting a patch to fix this silliness in 2.5.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-30 14:32         ` Mark Hahn
@ 2001-01-30 15:30           ` Eric W. Biederman
  0 siblings, 0 replies; 15+ messages in thread
From: Eric W. Biederman @ 2001-01-30 15:30 UTC (permalink / raw)
  To: linux-mm

Mark Hahn <hahn@coffee.psychology.mcmaster.ca> writes:

> > > > +	spin_lock(&mm->page_table_lock);
> > > >  	mm->rss++;
> > > > +	spin_unlock(&mm->page_table_lock);
> > > >...
> > > 
> > > Would it not be better to use some sort of atomic add/subtract/clear
> operation
> 
> > > rather than a spinlock? (Which would also give you fewer atomic memory
> access
> 
> > > cycles).
> > 
> > This will unfortunately not do for all platforms. Please read
> > http://marc.theaimsgroup.com/?t=97630768100003&w=2&r=1 for the
> > last discussion of this.
> 
> which can be summarized as "yet another way sparc support screws Linux,
> and DMiller didn't want to fix his mess close to 2.4.0".  it's ridiculous
> for an inconsequential arch like sparc32 to cause fairly noticable problems
> for all other arches.
> 
> if noone beats me, I'll be submitting a patch to fix this silliness in 2.5.

The thing is we need a spinlock to actually change the page tables to
change the rss.  We pretty much get the accounting of rss under the
same spinlock for free.

Eric
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-02-13  3:15     ` george anzinger
@ 2001-02-13  2:05       ` Marcelo Tosatti
  2001-02-13 10:08       ` Stephen C. Tweedie
  1 sibling, 0 replies; 15+ messages in thread
From: Marcelo Tosatti @ 2001-02-13  2:05 UTC (permalink / raw)
  To: george anzinger
  Cc: Rasmus Andersen, Rik van Riel, torvalds, linux-kernel, linux-mm


On Mon, 12 Feb 2001, george anzinger wrote:

> Excuse me if I am off base here, but wouldn't an atomic operation be
> better here.  There are atomic inc/dec and add/sub macros for this.  It
> just seems that that is all that is needed here (from inspection of the
> patch).

Most functions which touch mm->rss already hold mm->page_table_lock (also
this functions are called more often and they use more CPU).

Making those functions use an atomic instruction just to optimize the
functions which do not lock mm->page_table_lock is not a good tradeoff.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-01-29 21:43   ` Rasmus Andersen
  2001-01-29 21:47     ` Rik van Riel
  2001-01-30  8:18     ` David Howells
@ 2001-02-13  3:15     ` george anzinger
  2001-02-13  2:05       ` Marcelo Tosatti
  2001-02-13 10:08       ` Stephen C. Tweedie
  2 siblings, 2 replies; 15+ messages in thread
From: george anzinger @ 2001-02-13  3:15 UTC (permalink / raw)
  To: Rasmus Andersen; +Cc: Rik van Riel, torvalds, linux-kernel, linux-mm

Excuse me if I am off base here, but wouldn't an atomic operation be
better here.  There are atomic inc/dec and add/sub macros for this.  It
just seems that that is all that is needed here (from inspection of the
patch).

George


Rasmus Andersen wrote:
> 
> On Mon, Jan 29, 2001 at 07:30:01PM -0200, Rik van Riel wrote:
> > On Mon, 29 Jan 2001, Rasmus Andersen wrote:
> >
> > > Please comment. Or else I will continue to sumbit it :)
> >
> > The following will hang the kernel on SMP, since you're
> > already holding the spinlock here. Try compiling with
> > CONFIG_SMP and see what happens...
> 
> You are right. Sloppy research by me :(
> 
> New patch below with the vmscan part removed.
> 
> diff -aur linux-2.4.1-pre11-clean/mm/memory.c linux/mm/memory.c
> --- linux-2.4.1-pre11-clean/mm/memory.c Sun Jan 28 20:53:13 2001
> +++ linux/mm/memory.c   Sun Jan 28 22:43:04 2001
> @@ -377,7 +377,6 @@
>                 address = (address + PGDIR_SIZE) & PGDIR_MASK;
>                 dir++;
>         } while (address && (address < end));
> -       spin_unlock(&mm->page_table_lock);
>         /*
>          * Update rss for the mm_struct (not necessarily current->mm)
>          * Notice that rss is an unsigned long.
> @@ -386,6 +385,7 @@
>                 mm->rss -= freed;
>         else
>                 mm->rss = 0;
> +       spin_unlock(&mm->page_table_lock);
>  }
> 
> 
> @@ -1038,7 +1038,9 @@
>                 flush_icache_page(vma, page);
>         }
> 
> +       spin_lock(&mm->page_table_lock);
>         mm->rss++;
> +       spin_unlock(&mm->page_table_lock);
> 
>         pte = mk_pte(page, vma->vm_page_prot);
> 
> @@ -1072,7 +1074,9 @@
>                         return -1;
>                 clear_user_highpage(page, addr);
>                 entry = pte_mkwrite(pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
> +               spin_lock(&mm->page_table_lock);
>                 mm->rss++;
> +               spin_unlock(&mm->page_table_lock);
>                 flush_page_to_ram(page);
>         }
>         set_pte(page_table, entry);
> @@ -1111,7 +1115,9 @@
>                 return 0;
>         if (new_page == NOPAGE_OOM)
>                 return -1;
> +       spin_lock(&mm->page_table_lock);
>         ++mm->rss;
> +       spin_unlock(&mm->page_table_lock);
>         /*
>          * This silly early PAGE_DIRTY setting removes a race
>          * due to the bad i386 page protection. But it's valid
> diff -aur linux-2.4.1-pre11-clean/mm/mmap.c linux/mm/mmap.c
> --- linux-2.4.1-pre11-clean/mm/mmap.c   Sat Dec 30 18:35:19 2000
> +++ linux/mm/mmap.c     Sun Jan 28 22:43:04 2001
> @@ -879,8 +879,8 @@
>         spin_lock(&mm->page_table_lock);
>         mpnt = mm->mmap;
>         mm->mmap = mm->mmap_avl = mm->mmap_cache = NULL;
> -       spin_unlock(&mm->page_table_lock);
>         mm->rss = 0;
> +       spin_unlock(&mm->page_table_lock);
>         mm->total_vm = 0;
>         mm->locked_vm = 0;
>         while (mpnt) {
> diff -aur linux-2.4.1-pre11-clean/mm/swapfile.c linux/mm/swapfile.c
> --- linux-2.4.1-pre11-clean/mm/swapfile.c       Fri Dec 29 23:07:24 2000
> +++ linux/mm/swapfile.c Sun Jan 28 22:43:04 2001
> @@ -231,7 +231,9 @@
>         set_pte(dir, pte_mkdirty(mk_pte(page, vma->vm_page_prot)));
>         swap_free(entry);
>         get_page(page);
> +       spin_lock(&vma->vm_mm->page_table_lock);
>         ++vma->vm_mm->rss;
> +       spin_unlock(&vma->vm_mm->page_table_lock);
>  }
> 
>  static inline void unuse_pmd(struct vm_area_struct * vma, pmd_t *dir,
> 
> --
>         Rasmus(rasmus@jaquet.dk)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] guard mm->rss with page_table_lock (241p11)
  2001-02-13  3:15     ` george anzinger
  2001-02-13  2:05       ` Marcelo Tosatti
@ 2001-02-13 10:08       ` Stephen C. Tweedie
  1 sibling, 0 replies; 15+ messages in thread
From: Stephen C. Tweedie @ 2001-02-13 10:08 UTC (permalink / raw)
  To: george anzinger
  Cc: Rasmus Andersen, Rik van Riel, torvalds, linux-kernel, linux-mm

Hi,

On Mon, Feb 12, 2001 at 07:15:57PM -0800, george anzinger wrote:
> Excuse me if I am off base here, but wouldn't an atomic operation be
> better here.  There are atomic inc/dec and add/sub macros for this.  It
> just seems that that is all that is needed here (from inspection of the
> patch).

The counter-argument is that we already hold the page table lock in
the vast majority of places where the rss is modified, so overall it's
cheaper to avoid the extra atomic update.

Cheers,
 Stephen
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux.eu.org/Linux-MM/

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2001-02-13 10:08 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-01-29 21:23 [PATCH] guard mm->rss with page_table_lock (241p11) Rasmus Andersen
2001-01-29 21:30 ` Rik van Riel
2001-01-29 21:43   ` Rasmus Andersen
2001-01-29 21:47     ` Rik van Riel
2001-01-30  8:18     ` David Howells
2001-01-30  8:31       ` Rasmus Andersen
2001-01-30 14:32         ` Mark Hahn
2001-01-30 15:30           ` Eric W. Biederman
2001-01-30  8:39       ` David S. Miller
2001-01-30 11:17         ` Chris Wedgwood
2001-01-30 11:23           ` Rik van Riel
2001-01-30 11:38             ` Rasmus Andersen
2001-02-13  3:15     ` george anzinger
2001-02-13  2:05       ` Marcelo Tosatti
2001-02-13 10:08       ` Stephen C. Tweedie

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox