guard mm->rss with page_table_lock (241p11)

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

Quoted 1 line · Rasmus Andersen

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...

Quoted 4 lines · Rik van Riel

Quoted 1 line · Rasmus Andersen

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

Quoted 3 lines

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.

mm/memory.c line 1043

Quoted 5 lines

...

mm/memory.c +2 -0

+	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).

Quoted 8 lines

mm/memory.c line 1043

Quoted 4 lines

mm/memory.c +2 -0

+	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.

Quoted 6 lines

Quoted 6 lines

mm/memory.c line 1043

Quoted 4 lines

mm/memory.c +2 -0

+	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

Quoted 1 line

Quoted 1 line

rather than a spinlock? (Which would also give you fewer atomic memory

access

Quoted 5 lines

Quoted 1 line

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.

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).

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...

Quoted 7 lines · Rik van Riel

Quoted 1 line · Rasmus Andersen

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

mm/memory.c Sun Jan 28 22:43:04 2001 renamed from mm/memory.c Sun Jan 28 20:53:13 2001 +7 -1

@@ -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

mm/mmap.c Sun Jan 28 22:43:04 2001 renamed from mm/mmap.c Sat Dec 30 18:35:19 2000 +1 -1

@@ -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

mm/swapfile.c Sun Jan 28 22:43:04 2001 renamed from mm/swapfile.c Fri Dec 29 23:07:24 2000 +2 -2

@@ -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([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/

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).

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).