mm->rss is modified without page_table_lock held

PATCH In review Rasmus Andersen 5 participants 12 messages Dec 8, 2000 – Dec 9, 2000

Subsystems (6)

Memory management MM core MGLRU Memory mapping +2 more subsystems

lore

Share & tools

lore

Rasmus Andersen

author

2000-12-08 21:00 UTC #0 lore raw

PATCH 1/1 mm->rss is modified without page_table_lock held

Hi.

The following patch moves the page_table_lock in mm/* to cover the modification of mm->rss in 240-test12-pre7. It was inspired by a similar patch from davej(?) which covered too much, AFAIR. The item is on Tytso's ToDo list.

Please comment.

mm/memory.c +7 -1

diff -Naur linux-240-t12-pre7-clean/mm/memory.c linux/mm/memory.c
@@ -371,7 +371,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.

@@ -380,6 +379,7 @@
 		mm->rss -= freed;
 	else
 		mm->rss = 0;
+	spin_unlock(&mm->page_table_lock);
 }

@@ -1076,7 +1076,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);

@@ -1110,7 +1112,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);

@@ -1149,7 +1153,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

mm/mmap.c +1 -1

diff -Naur linux-240-t12-pre7-clean/mm/mmap.c linux/mm/mmap.c
@@ -889,8 +889,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) {

mm/swapfile.c +2 -0

diff -Naur linux-240-t12-pre7-clean/mm/swapfile.c linux/mm/swapfile.c
@@ -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,

mm/vmscan.c +2 -0

diff -Naur linux-240-t12-pre7-clean/mm/vmscan.c linux/mm/vmscan.c
@@ -98,7 +98,9 @@
 		set_pte(page_table, swp_entry_to_pte(entry));
 drop_pte:
 		UnlockPage(page);
+		spin_lock(&mm->page_table_lock);
 		mm->rss--;
+		spin_unlock(&mm->page_table_lock);
 		flush_tlb_page(vma, address);
 		deactivate_page(page);
 		page_cache_release(page);

Signature · 10 lines

Regards,
        Rasmus([email protected])

You know how dumb the average guy is?  Well, by  definition, half
of them are even dumber than that.
            -- J.R. "Bob" Dobbs 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Mark Hahn

2000-12-08 21:07 UTC #1 lore raw

Quoted 2 lines

The following patch moves the page_table_lock in mm/* to cover the modification of mm->rss in 240-test12-pre7. It was inspired by a

can't we just change rss to count pages?
or are we worried about rss's over ~16 TB?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-08 21:44 UTC #2 lore raw

Date: Fri, 8 Dec 2000 15:36:35 -0500 (EST)
From: Mark Hahn <[email protected]>

can't we just change rss to count pages?

This is what it does now.

or are we worried about rss's over ~16 TB?

If we weren't, we could just use an atomic_t for this problem. We can't.

This patch is the only currently available solution to this problem which is in any way acceptable.

Later,
David S. Miller
[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Roberto Fichera

2000-12-09 11:01 UTC #3 lore raw

In reply to Rasmus Andersen

Quoted 6 lines · Rasmus Andersen

Hi.
The following patch moves the page_table_lock in mm/* to cover the modification of mm->rss in 240-test12-pre7. It was inspired by a similar patch from davej(?) which covered too much, AFAIR. The item is on Tytso's ToDo list.

[...snip...]

mm/memory.c line 1082

Quoted 8 lines

mm/memory.c +2 -0

@@ -1076,7 +1076,9 @@
                flush_icache_page(vma, page);
        }
 
+       spin_lock(&mm->page_table_lock);
        mm->rss++;
+       spin_unlock(&mm->page_table_lock);

[...snip...]

Why we couldn't use atomic_inc(&mm->rss) here and below, avoiding to wrap the inc with a spin_lock()/spin_unlock() ?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-09 13:19 UTC #4 lore raw

Date: Sat, 09 Dec 2000 11:25:09 +0100
From: Roberto Fichera <[email protected]>

Why we couldn't use atomic_inc(&mm->rss) here and below, avoiding to wrap the inc with a spin_lock()/spin_unlock() ?

atomic_t does not guarentee a large enough range necessary for mm->rss

Roberto Fichera

2000-12-09 15:24 UTC #5 lore raw

Quoted 16 lines · Rasmus Andersen

On Sat, Dec 09, 2000 at 11:25:09AM +0100, Roberto Fichera wrote:
[...]
Quoted 10 lines
mm/memory.c line 1081
Quoted 4 lines
mm/memory.c +2 -0
+       spin_lock(&mm->page_table_lock);
        mm->rss++;
+       spin_unlock(&mm->page_table_lock);
[...snip...]
Why we couldn't use atomic_inc(&mm->rss) here and below, avoiding to wrap the inc with a spin_lock()/spin_unlock() ?
AFAIR, because for some architectures we can't rely on mm->rss fitting in an atomic_t. See davem's (somewhat short) post in this thread. Otherwise search the archives for the original thread treating this problem.

Quoted 7 lines · David S. Miller

Date: Sat, 09 Dec 2000 11:25:09 +0100
From: Roberto Fichera <[email protected]>
Why we couldn't use atomic_inc(&mm->rss) here and below, avoiding to wrap the inc with a spin_lock()/spin_unlock() ?
atomic_t does not guarentee a large enough range necessary for mm->rss

If we haven't some atomic_t that can be negative we could define atomic_t as unsigned long for all arch, this is sufficient to fitting all the range for the mm->rss.

Roberto Fichera.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-09 15:29 UTC #6 lore raw

Date: Sat, 09 Dec 2000 15:48:05 +0100
From: Roberto Fichera <[email protected]>

Quoted 1 line

atomic_t does not guarentee a large enough range necessary for mm->rss

If we haven't some atomic_t that can be negative we could define atomic_t as unsigned long for all arch,
this is sufficient to fitting all the range for the mm->rss.

32-bit Sparc has unsigned long as 32-bit, and the top 8 bits of the atomic_t are used for a spinlock, thus a 27-bit atomic_t, there is not enough precision.

Roberto Fichera

2000-12-09 15:44 UTC #7 lore raw

Quoted 5 lines · David S. Miller

Date: Sat, 09 Dec 2000 15:48:05 +0100
From: Roberto Fichera <[email protected]>
Quoted 1 line
atomic_t does not guarentee a large enough range necessary for mm->rss

Quoted 7 lines

If we haven't some atomic_t that can be negative we could define atomic_t as unsigned long for all arch,
this is sufficient to fitting all the range for the mm->rss.
32-bit Sparc has unsigned long as 32-bit, and the top 8 bits of the atomic_t are used for a spinlock, thus a 27-bit atomic_t, there is not enough precision.

8 bits for a spinlock ? What kind of use we have here ? All arch except Sparc32 don't have this trick.

Roberto Fichera.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

David S. Miller

2000-12-09 15:46 UTC #8 lore raw

Date: Sat, 09 Dec 2000 16:07:03 +0100
From: Roberto Fichera <[email protected]>

8 bits for a spinlock ? What kind of use we have here ?

Sparc32 (like some other older architectures) do not have a word atomic update instruction, but it does have a byte spinlock. To conserve space and implement the atomic update properly, we use a spinlock in the top byte of the word.

All arch except Sparc32 don't have this trick.

This may not be true forever.

Also, this sematic was decided upon many eons ago, changing it a month before 2.4.0 just to deal with this mm->rss atomicity issue is not going to happen. The spinlock patch exists, and if nothing better comes up, we should just use it.

Roberto Fichera

2000-12-09 18:48 UTC #9 lore raw

Quoted 9 lines · David S. Miller

Date: Sat, 09 Dec 2000 16:07:03 +0100
From: Roberto Fichera <[email protected]>
8 bits for a spinlock ? What kind of use we have here ?
Sparc32 (like some other older architectures) do not have a word atomic update instruction, but it does have a byte spinlock. To conserve space and implement the atomic update properly, we use a spinlock in the top byte of the word.

There's any possibility ;-) to define it as

typedef struct { volatile char spinlock, volatile long counter } atomic_t;

Quoted 4 lines

Also, this sematic was decided upon many eons ago, changing it a month before 2.4.0 just to deal with this mm->rss atomicity issue is not going to happen. The spinlock patch exists, and if nothing better comes up, we should just use it.

Indeed! You are right! I was thinking to optimize it, using a spinlock/unlock we spent several time for a inc.

Roberto Fichera.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Albert D. Cahalan

2000-12-09 23:10 UTC #10 lore raw

In reply to David S. Miller

Quoted 3 lines

32-bit Sparc has unsigned long as 32-bit, and the top 8 bits of the atomic_t are used for a spinlock, thus a 27-bit atomic_t, there is not enough precision.

So the SPARC port is broken. It is just sick to have this "feature" screw things up for all the ports with a proper atomic_t.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Rasmus Andersen

author

2000-12-09 12:14 UTC #11 lore raw

In reply to Rasmus Andersen

On Sat, Dec 09, 2000 at 11:25:09AM +0100, Roberto Fichera wrote:
[...]

Quoted 10 lines

mm/memory.c line 1081
Quoted 4 lines
mm/memory.c +2 -0
+       spin_lock(&mm->page_table_lock);
        mm->rss++;
+       spin_unlock(&mm->page_table_lock);
[...snip...]
Why we couldn't use atomic_inc(&mm->rss) here and below, avoiding to wrap the inc with a spin_lock()/spin_unlock() ?

AFAIR, because for some architectures we can't rely on mm->rss fitting in an atomic_t. See davem's (somewhat short) post in this thread. Otherwise search the archives for the original thread treating this problem.

Signature · 8 lines

        Rasmus([email protected])

Television is called a medium because it is neither rare nor well-done. 
  -- Anonymous
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
Please read the FAQ at http://www.tux.org/lkml/