0/5: remove PageReserved

mm/memory.c +3 -1

@@ -1259,13 +1286,16 @@ static int do_wp_page(struct mm_struct *
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
-	if (!PageReserved(old_page))
+	if (old_page == ZERO_PAGE(address))
+		old_page = NULL;
+	else
 		page_cache_get(old_page);

Index: linux-2.6/mm/fremap.c
===================================================================

mm/fremap.c +5 -9

@@ -29,18 +29,21 @@ static inline void zap_pte(struct mm_str
 		return;
 	if (pte_present(pte)) {
 		unsigned long pfn = pte_pfn(pte);
+		struct page *page;
 
 		flush_cache_page(vma, addr, pfn);
 		pte = ptep_clear_flush(vma, addr, ptep);
-		if (pfn_valid(pfn)) {
-			struct page *page = pfn_to_page(pfn);
-			if (!PageReserved(page)) {
-				if (pte_dirty(pte))
-					set_page_dirty(page);
-				page_remove_rmap(page);
-				page_cache_release(page);
-				dec_mm_counter(mm, rss);
-			}
+		if (unlikely(!pfn_valid(pfn))) {
+			print_invalid_pfn("zap_pte", pte, vma->vm_flags, addr);
+			return;
+		}

mm/fremap.c +2 -0

@@ -65,6 +68,8 @@ int install_page(struct mm_struct *mm, s
 	pgd_t *pgd;
 	pte_t pte_val;
 
+	BUG_ON(vma->vm_flags & VM_RESERVED);
+
 	pgd = pgd_offset(mm, addr);
 	spin_lock(&mm->page_table_lock);
 

Index: linux-2.6/drivers/scsi/sg.c
===================================================================

drivers/scsi/sg.c +0 -0

@@ -1887,9 +1887,10 @@ st_unmap_user_pages(struct scatterlist *
 	int i;

Index: linux-2.6/drivers/scsi/st.c
===================================================================

Index: linux-2.6/sound/core/pcm_native.c
===================================================================

Index: linux-2.6/mm/mmap.c
===================================================================

mm/mmap.c +4 -0

@@ -1073,6 +1073,17 @@ munmap_back:
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
+		if ((vma->vm_flags & (VM_SHARED | VM_WRITE | VM_RESERVED))
+						== (VM_WRITE | VM_RESERVED)) {
+			printk(KERN_WARNING "program %s is using MAP_PRIVATE, "
+				"PROT_WRITE mmap of VM_RESERVED memory, which "

Index: linux-2.6/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c +1 -0

@@ -433,15 +433,12 @@ static int save_highmem_zone(struct zone
 			continue;
 		page = pfn_to_page(pfn);
 		/*
+		 * PageReserved(page) -
 		 * This condition results from rvmalloc() sans vmalloc_32()
 		 * and architectural memory reservations. This should be

include/linux/mm.h static inline void get_page(struct page · line 342

Quoted 7 lines · Nick Piggin

include/linux/mm.h +1 -1

@@ -337,7 +338,7 @@ static inline void get_page(struct page 
 static inline void put_page(struct page *page)
 {
-	if (!PageReserved(page) && put_page_testzero(page))
+	if (put_page_testzero(page))
 		__page_cache_release(page);
 }

No sweep before this? I'm not so sure.

Quoted 5 lines

mm/memory.c +0 -0

@@ -514,7 +534,8 @@ int copy_page_range(struct mm_struct *ds
 	return 0;
 }
 

mm/memory.c int copy_page_range(struct mm_struct *ds · line 541

Quoted 6 lines

mm/memory.c +2 -1

-static void zap_pte_range(struct mmu_gather *tlb, pmd_t *pmd,
+static void zap_pte_range(struct mmu_gather *tlb,
+				struct vm_area_struct *vma, pmd_t *pmd,
 				unsigned long addr, unsigned long end,
 				struct zap_details *details)
 {

As exciting as this is, !!(vma->vm_flags & VM_RESERVED) could trivially go into struct zap_details without excess args or diff.

Quoted 5 lines

mm/memory.c +0 -0

@@ -1225,6 +1251,8 @@ static int do_wp_page(struct mm_struct *
 	unsigned long pfn = pte_pfn(pte);
 	pte_t entry;
 

mm/memory.c static int do_wp_page(struct mm_struct * · line 1258

Quoted 5 lines

mm/memory.c +2 -0

+	BUG_ON(vma->vm_flags & VM_RESERVED);
+
 	if (unlikely(!pfn_valid(pfn))) {
 		/*
 		 * This should really halt the system so it can be debugged or

!pfn_valid(pfn) is banned when !(vma->vm_flags & VM_RESERVED); here we BUG_ON the precondition then handle !pfn_valid(pfn) in the old manner where some new infrastructure has been erected for reporting such errors.

Quoted 13 lines

mm/memory.c +4 -1

@@ -1259,13 +1286,16 @@ static int do_wp_page(struct mm_struct *
 	/*
 	 * Ok, we need to copy. Oh, well..
 	 */
-	if (!PageReserved(old_page))
+	if (old_page == ZERO_PAGE(address))
+		old_page = NULL;
+	else
 		page_cache_get(old_page);
+
 	spin_unlock(&mm->page_table_lock);
 

mm/memory.c static int do_wp_page(struct mm_struct * · line 1301

Quoted 7 lines

mm/memory.c +1 -1

 	if (unlikely(anon_vma_prepare(vma)))
 		goto no_new_page;
-	if (old_page == ZERO_PAGE(address)) {
+	if (old_page == NULL) {
 		new_page = alloc_zeroed_user_highpage(vma, address);
 		if (!new_page)
 			goto no_new_page;

There are some micro-optimizations mixed in with this and some subsequent do_wp_page() alterations.

mm/page_alloc.c void __init memmap_init_zone(unsigned lo · line 1662

Quoted 10 lines

mm/page_alloc.c +1 -1

@@ -1654,7 +1656,7 @@ void __init memmap_init_zone(unsigned lo
 
 	for (page = start; page < (start + size); page++) {
 		set_page_zone(page, NODEZONE(nid, zone));
-		set_page_count(page, 0);
+		set_page_count(page, 1);
 		reset_page_mapcount(page);
 		SetPageReserved(page);
 		INIT_LIST_HEAD(&page->lru);

The refcounting and PG_reserved activity in memmap_init_zone() is superfluous. bootmem.c does all the necessary accounting internally.

Quoted 31 lines

Index: linux-2.6/mm/fremap.c
===================================================================

mm/fremap.c +10 -9

@@ -29,18 +29,21 @@ static inline void zap_pte(struct mm_str
 		return;
 	if (pte_present(pte)) {
 		unsigned long pfn = pte_pfn(pte);
+		struct page *page;
 
 		flush_cache_page(vma, addr, pfn);
 		pte = ptep_clear_flush(vma, addr, ptep);
-		if (pfn_valid(pfn)) {
-			struct page *page = pfn_to_page(pfn);
-			if (!PageReserved(page)) {
-				if (pte_dirty(pte))
-					set_page_dirty(page);
-				page_remove_rmap(page);
-				page_cache_release(page);
-				dec_mm_counter(mm, rss);
-			}
+		if (unlikely(!pfn_valid(pfn))) {
+			print_invalid_pfn("zap_pte", pte, vma->vm_flags, addr);
+			return;
+		}
+		page = pfn_to_page(pfn);
+		if (page != ZERO_PAGE(addr)) {
+			if (pte_dirty(pte))
+				set_page_dirty(page);
+			page_remove_rmap(page);

mm/fremap.c static inline void zap_pte(struct mm_str · line 49

Quoted 5 lines

mm/fremap.c +2 -0

+			dec_mm_counter(mm, rss);
+			page_cache_release(page);
 		}
 	} else {
 		if (!pte_file(pte))

There is no error returned here to be handled by the caller.

Quoted 14 lines

mm/fremap.c +2 -0

@@ -65,6 +68,8 @@ int install_page(struct mm_struct *mm, s
 	pgd_t *pgd;
 	pte_t pte_val;
 
+	BUG_ON(vma->vm_flags & VM_RESERVED);
+
 	pgd = pgd_offset(mm, addr);
 	spin_lock(&mm->page_table_lock);
 

@@ -122,6 +127,8 @@ int install_file_pte(struct mm_struct *m
 	pgd_t *pgd;
 	pte_t pte_val;
 

mm/fremap.c int install_page(struct mm_struct *mm, s · line 74

Quoted 4 lines

mm/fremap.c +2 -0

+	BUG_ON(vma->vm_flags & VM_RESERVED);
+
 	pgd = pgd_offset(mm, addr);
 	spin_lock(&mm->page_table_lock);

This has no effect but to artificially constrain the interface. There is no a priori reason to avoid the use of install_page() to deposit mappings to normal pages in VM_RESERVED vmas. It's only the reverse being "banned" here. Similar comments also apply to zap_pte().

Quoted 8 lines

Index: linux-2.6/drivers/scsi/sg.c
===================================================================

drivers/scsi/sg.c +0 -0

@@ -1887,9 +1887,10 @@ st_unmap_user_pages(struct scatterlist *
 	int i;
 

drivers/scsi/sg.c st_unmap_user_pages(struct scatterlist * · line 1896

Quoted 9 lines

drivers/scsi/sg.c +2 -1

 	for (i=0; i < nr_pages; i++) {
-		if (dirtied && !PageReserved(sgl[i].page))
+		if (dirtied)
 			SetPageDirty(sgl[i].page);
 		/* unlock_page(sgl[i].page); */
+		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
 		/* FIXME: cache flush missing for rw==READ
 		 * FIXME: call the correct reference counting function
 		 */

An answer should be devised for this. My numerous SCSI CD-ROM devices (I have 5 across several different machines of several different arches) are rather unlikely to be happy with /* FIXME: XXX ... as an answer.

Quoted 6 lines

Index: linux-2.6/drivers/scsi/st.c
===================================================================

drivers/scsi/st.c static int sgl_unmap_user_pages(struct s · line 4443

Quoted 10 lines

drivers/scsi/st.c +2 -1

@@ -4435,8 +4435,9 @@ static int sgl_unmap_user_pages(struct s
 	int i;
 
 	for (i=0; i < nr_pages; i++) {
-		if (dirtied && !PageReserved(sgl[i].page))
+		if (dirtied)
 			SetPageDirty(sgl[i].page);
+		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
 		/* FIXME: cache flush missing for rw==READ
 		 * FIXME: call the correct reference counting function
 		 */

Mutatis mutandis for my SCSI tape drive.

Quoted 11 lines · Nick Piggin

Index: linux-2.6/sound/core/pcm_native.c
===================================================================

sound/core/pcm_native.c +1 -2

@@ -2942,8 +2942,7 @@ static struct page * snd_pcm_mmap_status
 		return NOPAGE_OOM;
 	runtime = substream->runtime;
 	page = virt_to_page(runtime->status);
-	if (!PageReserved(page))
-		get_page(page);
+	get_page(page);

sound/core/pcm_native.c static struct page * snd_pcm_mmap_status · line 2948

Quoted 3 lines

sound/core/pcm_native.c +0 -0

 	if (type)
 		*type = VM_FAULT_MINOR;
 	return page;

snd_malloc_pages() marks all pages it allocates PG_reserved. This merits some commentary, and likely the removal of the superfluous PG_reserved usage.

Quoted 19 lines

Index: linux-2.6/mm/mmap.c
===================================================================

mm/mmap.c +10 -0

@@ -1073,6 +1073,17 @@ munmap_back:
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
+		if ((vma->vm_flags & (VM_SHARED | VM_WRITE | VM_RESERVED))
+						== (VM_WRITE | VM_RESERVED)) {
+			printk(KERN_WARNING "program %s is using MAP_PRIVATE, "
+				"PROT_WRITE mmap of VM_RESERVED memory, which "
+				"is deprecated. Please report this to "
+				"[email protected]\n",current->comm);
+			if (vma->vm_ops && vma->vm_ops->close)
+				vma->vm_ops->close(vma);
+			error = -EACCES;
+			goto unmap_and_free_vma;

mm/mmap.c munmap_back: · line 1089

Quoted 4 lines

mm/mmap.c +1 -0

+		}
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
 		if (error)

This is user-triggerable where the driver honors mmap() protection flags blindly.

mm/bootmem.c static unsigned long __init free_all_boo · line 292

Quoted 13 lines

Index: linux-2.6/mm/bootmem.c
===================================================================

mm/bootmem.c +1 -0

@@ -286,6 +286,7 @@ static unsigned long __init free_all_boo
 				if (j + 16 < BITS_PER_LONG)
 					prefetchw(page + j + 16);
 				__ClearPageReserved(page + j);
+				set_page_count(page + j, 0);
 			}
 			__free_pages(page, order);
 			i += BITS_PER_LONG;

ibid re: bootmem

Quoted 19 lines

Index: linux-2.6/kernel/power/swsusp.c
===================================================================

kernel/power/swsusp.c +1 -4

@@ -433,15 +433,12 @@ static int save_highmem_zone(struct zone
 			continue;
 		page = pfn_to_page(pfn);
 		/*
+		 * PageReserved(page) -
 		 * This condition results from rvmalloc() sans vmalloc_32()
 		 * and architectural memory reservations. This should be
 		 * corrected eventually when the cases giving rise to this
 		 * are better understood.
 		 */
-		if (PageReserved(page)) {
-			printk("highmem reserved page?!\n");
-			continue;
-		}

kernel/power/swsusp.c static int save_highmem_zone(struct zone · line 444

Quoted 3 lines

kernel/power/swsusp.c +0 -0

 		BUG_ON(PageNosave(page));
 		if (PageNosaveFree(page))
 			continue;

This behavioral change needs to be commented on. There are some additional difficulties when memory holes are unintentionally covered by mem_map[]; It is beneficial otherwise. It's likely to triplefault on such holes.

Quoted 4 lines

kernel/power/swsusp.c +0 -0

@@ -527,13 +524,8 @@ static int saveable(struct zone * zone, 
 		return 0;
 

kernel/power/swsusp.c static int saveable(struct zone * zone, · line 530

Quoted 10 lines

kernel/power/swsusp.c +0 -5

 	page = pfn_to_page(pfn);
-	BUG_ON(PageReserved(page) && PageNosave(page));
 	if (PageNosave(page))
 		return 0;
-	if (PageReserved(page) && pfn_is_nosave(pfn)) {
-		pr_debug("[nosave pfn 0x%lx]", pfn);
-		return 0;
-	}
 	if (PageNosaveFree(page))
 		return 0;

The pfn_is_nosave() check must stand barring justification of why unintentionally saving (and hence restoring) the page is tolerable.

Quoted 2 lines

As exciting as this is, !!(vma->vm_flags & VM_RESERVED) could trivially go into struct zap_details without excess args or diff.

Actually, it isn't trivial. I thought of trying that.

Quoted 2 lines

The refcounting and PG_reserved activity in memmap_init_zone() is superfluous. bootmem.c does all the necessary accounting internally.

Well not superfluous yet. It is kept around to support all the arch code that still uses it (not much, mainly reserved memory reporting).

Quoted 1 line

There is no error returned here to be handled by the caller.

That's OK, the pte has been cleared. Nothing else we can do.

Quoted 4 lines

This has no effect but to artificially constrain the interface. There is no a priori reason to avoid the use of install_page() to deposit mappings to normal pages in VM_RESERVED vmas. It's only the reverse being "banned" here. Similar comments also apply to zap_pte().

No, install_page is playing with the page (eg. page_add_file_rmap) which is explicity banned even before my PageReserved removal. It is unclear that this ever safely worked for normal pages, and will hit NULL page dereferences if trying to do it with iomem.

Quoted 3 lines

An answer should be devised for this. My numerous SCSI CD-ROM devices (I have 5 across several different machines of several different arches) are rather unlikely to be happy with /* FIXME: XXX ... as an answer.

The worst it will do is dirty a VM_RESERVED page. So it is going to work unless you're thinking about doing something crazy like mmap /dev/mem and send your CDROM some pages from there. But yeah, I have to find someone who knows what they're doing to look at this.

Quoted 3 lines

snd_malloc_pages() marks all pages it allocates PG_reserved. This merits some commentary, and likely the removal of the superfluous PG_reserved usage.

Sure, but not in this patch. The aim here is just to eliminate special casing of refcounting. Other PG_reserved usage can stay around for the moment (and is actually good for catching errors).

Quoted 2 lines

This is user-triggerable where the driver honors mmap() protection flags blindly.

If the user is allowed write access to VM_RESERVED memory, then I suspect there is a lot worse they can do than flood the log. But the check isn't going to stay around forever.

Quoted 3 lines

This behavioral change needs to be commented on. There are some additional difficulties when memory holes are unintentionally covered by mem_map[]; It is beneficial otherwise. It's likely to triplefault on such holes.

It seems the author of this code themselves didn't really understand what was going on here, so I'm buggered if I can be bothered :) Remember though, PageReserved can stay around for as long as we need, so this hunk can be trivially reverted if it is an immediate problem.

Quoted 2 lines

The pfn_is_nosave() check must stand barring justification of why unintentionally saving (and hence restoring) the page is tolerable.

I thought the PageNosave should catch that, and the next line is just a debug check. But I'm looking for someone who *actually* knows how swsusp works, if anyone would like to volunteer :)

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

As exciting as this is, !!(vma->vm_flags & VM_RESERVED) could trivially go into struct zap_details without excess args or diff.

Quoted 1 line · Nick Piggin

Actually, it isn't trivial. I thought of trying that.

You're probably thinking of the zap_page_range() vs. unmap_page_range() topmost-level entrypoints as being the only places to set the flag in details. Unconditionally clobbering the field in details in zap_pud_range() should resolve any issues associated with that.

This is obviously a style and/or diff-minimization issue.

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

The refcounting and PG_reserved activity in memmap_init_zone() is superfluous. bootmem.c does all the necessary accounting internally.

Quoted 2 lines · Nick Piggin

Well not superfluous yet. It is kept around to support all the arch code that still uses it (not much, mainly reserved memory reporting).

That activity is outside memmap_init_zone(). Specifically, the page structures are not used for anything whatsoever before bootmem puts them into the buddy allocators. It is not particularly interesting or difficult to defer even the initialization to the precise point in time bootmem cares to perform buddy bitmap insertion. This goes for all fields of struct page. What's notable about PG_reserved and refcounts here is that memmap_init_zone() goes about flipping bits one way where free_all_bootmem_core() undoes all its work.

Quoted 1 line · William Lee Irwin III

Quoted 1 line

There is no error returned here to be handled by the caller.

On Thu, Jun 23, 2005 at 08:32:05PM +1000, Nick Piggin wrote:

Quoted 1 line

That's OK, the pte has been cleared. Nothing else we can do.

This is called in the interior of a loop, which may be beneficial to terminate if this intended semantic is to be enforced. Furthermore, no error is propagated to the caller, which is not the desired effect in the stated error reporting scheme. So the code is inconsistent with explicitly stated intention.

Quoted 4 lines · William Lee Irwin III

Quoted 4 lines

This has no effect but to artificially constrain the interface. There is no a priori reason to avoid the use of install_page() to deposit mappings to normal pages in VM_RESERVED vmas. It's only the reverse being "banned" here. Similar comments also apply to zap_pte().

Quoted 4 lines · Nick Piggin

No, install_page is playing with the page (eg. page_add_file_rmap) which is explicity banned even before my PageReserved removal. It is unclear that this ever safely worked for normal pages, and will hit NULL page dereferences if trying to do it with iomem.

You are going on about the fact that install_page() can't be used on memory outside mem_map[] as it requires a page structure, and can't be used on reserved pages because page_add_file_rmap() will BUG. This case is not being discussed.

The issue at stake is inserting normal pages into a VM_RESERVED vma. These will arise as e.g. kernel-allocated buffers managed by normal reference counting. remap_pfn_range() can't do it; it refuses to operate on "valid memory". install_page() now won't do it; it refuses to touch a VM_RESERVED vma. So this creates a giant semantic hole, and potentially breaks working code (i.e. if you were going to do this you would need not only a replacement but also a sweep to adjust all the drivers doing it or prove their nonexistence).

Quoted 3 lines · William Lee Irwin III

Quoted 3 lines

An answer should be devised for this. My numerous SCSI CD-ROM devices (I have 5 across several different machines of several different arches) are rather unlikely to be happy with /* FIXME: XXX ... as an answer.

Quoted 4 lines · Nick Piggin

The worst it will do is dirty a VM_RESERVED page. So it is going to work unless you're thinking about doing something crazy like mmap /dev/mem and send your CDROM some pages from there. But yeah, I have to find someone who knows what they're doing to look at this.

Then you should replace FIXME: XXX with this explanation. By and large the presence of "FIXME: XXX" is a sign there is a gigantic hole in the code. It should definitely not be done with new code, but rather, exclusively confined to documenting discoveries of preexisting brokenness. After all, if a patch is introducing broken code, why would we merge it? Best to adjust the commentary and avoid that question altogether.

There are actually larger questions about this than the reserved page handling. If e.g. pagecache pages need to be dirtied the raw bitflag toggling is probably not how it should be done.

Quoted 3 lines · William Lee Irwin III

Quoted 3 lines

snd_malloc_pages() marks all pages it allocates PG_reserved. This merits some commentary, and likely the removal of the superfluous PG_reserved usage.

Quoted 3 lines · Nick Piggin

Sure, but not in this patch. The aim here is just to eliminate special casing of refcounting. Other PG_reserved usage can stay around for the moment (and is actually good for catching errors).

Unfortunately for this scheme, it's very much a case of putting the cart before the horse. PG_reserved is toggled at random in this driver after this change, to no useful effect (debugging or otherwise). And this really goes for the whole affair. Diddling the core first is just going to create bugs. Converting the users first is the way these things need to be done. When complete, nothing needs the core flags twiddling anymore and you just nuke the flag twiddling from the core.

Quoted 2 lines · William Lee Irwin III

Quoted 2 lines

This is user-triggerable where the driver honors mmap() protection flags blindly.

Quoted 3 lines · Nick Piggin

If the user is allowed write access to VM_RESERVED memory, then I suspect there is a lot worse they can do than flood the log. But the check isn't going to stay around forever.

This doesn't really do a whole lot of good for the unwitting user who invokes a privileged application relying on such kernel behavior. User-visible changes need to be taken on with somewhat more care (okay, vastly more, along with long-term backward compatibility).

Quoted 3 lines · William Lee Irwin III

Quoted 3 lines

This behavioral change needs to be commented on. There are some additional difficulties when memory holes are unintentionally covered by mem_map[]; It is beneficial otherwise. It's likely to triplefault on such holes.

Quoted 4 lines · Nick Piggin

It seems the author of this code themselves didn't really understand what was going on here, so I'm buggered if I can be bothered :) Remember though, PageReserved can stay around for as long as we need, so this hunk can be trivially reverted if it is an immediate problem.

This doesn't really fly. A fair number of drivers are poorly-understood and numerous gyrations have to be gone through to avoid breaking their possible assumptions until at long last clarifications are made (that is, if they're ever made). swsusp is not demotable to below their status on a whim.

Quoted 5 lines

You're probably thinking of the zap_page_range() vs. unmap_page_range() topmost-level entrypoints as being the only places to set the flag in details. Unconditionally clobbering the field in details in zap_pud_range() should resolve any issues associated with that. This is obviously a style and/or diff-minimization issue.

I'm thinking of exit_mmap and unmap_vmas, that don't pass in a details field at all, and all the tests for details being marked unlikely. I ended up thinking it was less ugly this way.

Quoted 8 lines

That activity is outside memmap_init_zone(). Specifically, the page structures are not used for anything whatsoever before bootmem puts them into the buddy allocators. It is not particularly interesting or difficult to defer even the initialization to the precise point in time bootmem cares to perform buddy bitmap insertion. This goes for all fields of struct page. What's notable about PG_reserved and refcounts here is that memmap_init_zone() goes about flipping bits one way where free_all_bootmem_core() undoes all its work.

This patch doesn't care how it works, that would be for a later patch.

Quoted 5 lines

This is called in the interior of a loop, which may be beneficial to terminate if this intended semantic is to be enforced. Furthermore, no error is propagated to the caller, which is not the desired effect in the stated error reporting scheme. So the code is inconsistent with explicitly stated intention.

No, the error reporting scheme says it doesn't handle any error, that is all. What we have here in terms of behaviour is exactly what used to happen, that is - do something saneish on error. Changing behaviour would be outside the scope of this patch, but be my guest.

Quoted 4 lines

You are going on about the fact that install_page() can't be used on memory outside mem_map[] as it requires a page structure, and can't be used on reserved pages because page_add_file_rmap() will BUG. This case is not being discussed.

And that it isn't allowed to touch struct page of physical pages in a VM_RESERVED region.

Quoted 8 lines

The issue at stake is inserting normal pages into a VM_RESERVED vma. These will arise as e.g. kernel-allocated buffers managed by normal reference counting. remap_pfn_range() can't do it; it refuses to operate on "valid memory". install_page() now won't do it; it refuses to touch a VM_RESERVED vma. So this creates a giant semantic hole, and potentially breaks working code (i.e. if you were going to do this you would need not only a replacement but also a sweep to adjust all the drivers doing it or prove their nonexistence).

I think you'll find that remap_pfn_range will be happy to operate on valid memory, and that any driver trying to use install_page on VM_RESERVED probably needs fixing anyway.

Quoted 7 lines

Unfortunately for this scheme, it's very much a case of putting the cart before the horse. PG_reserved is toggled at random in this driver after this change, to no useful effect (debugging or otherwise). And this really goes for the whole affair. Diddling the core first is just going to create bugs. Converting the users first is the way these things need to be done. When complete, nothing needs the core flags twiddling anymore and you just nuke the flag twiddling from the core.

I'm sorry, I don't see how 'diddling' the core will create bugs. This is a fine way to do it, and "converting" users first (whatever that means)

This is a fine way to do it, and "converting" users first (whatever that means) is not possible because VM_RESERVED handling in core code is not up to the task of replacing PageReserved without this patch.

Quoted 5 lines

This doesn't really fly. A fair number of drivers are poorly-understood and numerous gyrations have to be gone through to avoid breaking their possible assumptions until at long last clarifications are made (that is, if they're ever made). swsusp is not demotable to below their status on a whim.

Yep, that's why I'm going to ask some swsusp developers to have a look at it.

I wouldn't pretend to be able to fix every bug everywhere in the kernel myself.

Quoted 1 line · Nick Piggin

This patch doesn't care how it works, that would be for a later patch.

The general gist of all this is that the patch doesn't cover anywhere near enough ground, and so the above illustrates that the usage in/around bootmem is among the easiest of usages to remove. I have implemented what I'm talking about on several independent occasions.

Quoted 5 lines · William Lee Irwin III

Quoted 5 lines

This is called in the interior of a loop, which may be beneficial to terminate if this intended semantic is to be enforced. Furthermore, no error is propagated to the caller, which is not the desired effect in the stated error reporting scheme. So the code is inconsistent with explicitly stated intention.

Quoted 5 lines · Nick Piggin

No, the error reporting scheme says it doesn't handle any error, that is all. What we have here in terms of behaviour is exactly what used to happen, that is - do something saneish on error. Changing behaviour would be outside the scope of this patch, but be my guest.

Some places BUG(), some back out with an error return, others blithely proceed. This kind of inconsistency will broadly confuse callers of the API's.

Quoted 4 lines · William Lee Irwin III

Quoted 4 lines

You are going on about the fact that install_page() can't be used on memory outside mem_map[] as it requires a page structure, and can't be used on reserved pages because page_add_file_rmap() will BUG. This case is not being discussed.

Quoted 2 lines · Nick Piggin

And that it isn't allowed to touch struct page of physical pages in a VM_RESERVED region.

non sequitur

Quoted 8 lines · William Lee Irwin III

Quoted 8 lines

The issue at stake is inserting normal pages into a VM_RESERVED vma. These will arise as e.g. kernel-allocated buffers managed by normal reference counting. remap_pfn_range() can't do it; it refuses to operate on "valid memory". install_page() now won't do it; it refuses to touch a VM_RESERVED vma. So this creates a giant semantic hole, and potentially breaks working code (i.e. if you were going to do this you would need not only a replacement but also a sweep to adjust all the drivers doing it or prove their nonexistence).

On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote:

Quoted 3 lines

I think you'll find that remap_pfn_range will be happy to operate on valid memory, and that any driver trying to use install_page on VM_RESERVED probably needs fixing anyway.

install_page() of a !PageReserved() page in a VM_RESERVED vma is neither broken now nor does it merit going BUG().

/dev/mem was disallowed from mapping ordinary kernel memory for a reason (though I disagree with it), so the removal of the pfn_valid()/PageReserved() checks can't be blithely done like in your patch. Other arrangements must be made.

Quoted 7 lines · William Lee Irwin III

Quoted 7 lines

Unfortunately for this scheme, it's very much a case of putting the cart before the horse. PG_reserved is toggled at random in this driver after this change, to no useful effect (debugging or otherwise). And this really goes for the whole affair. Diddling the core first is just going to create bugs. Converting the users first is the way these things need to be done. When complete, nothing needs the core flags twiddling anymore and you just nuke the flag twiddling from the core.

On Fri, Jun 24, 2005 at 09:21:31AM +1000, Nick Piggin wrote:

Quoted 3 lines

I'm sorry, I don't see how 'diddling' the core will create bugs. This is a fine way to do it, and "converting" users first (whatever that means)

[cut text included in full in the following quote block]

This is going way too far. Someone please deal with this.

[continued]

Quoted 4 lines

This is a fine way to do it, and "converting" users first (whatever that means) is not possible because VM_RESERVED handling in core code is not up to the task of replacing PageReserved without this patch.

You aren't replacing any of the PG_reserved usage with VM_RESERVED in any of these patches. The primary motive of all this is AFAICT little more than getting the PG_reserved check out of put_page(), drivers and arches be damned.

Quoted 7 lines · William Lee Irwin III

Quoted 3 lines · Nick Piggin

I'm sorry, I don't see how 'diddling' the core will create bugs. This is a fine way to do it, and "converting" users first (whatever that means)

Quoted 4 lines

[cut text included in full in the following quote block]

This is going way too far. Someone please deal with this.

No, just tell me how it might magically create bugs in drivers that didn't exist in the first place?

Quoted 2 lines

I wouldn't pretend to be able to fix every bug everywhere in the kernel myself.

If you can't handle the sweep, you have no business writing the patch.

Quoted 6 lines · Nick Piggin

Index: linux-2.6/drivers/scsi/sg.c
===================================================================

drivers/scsi/sg.c +0 -0

@@ -1887,9 +1887,10 @@ st_unmap_user_pages(struct scatterlist *
 	int i;

drivers/scsi/sg.c st_unmap_user_pages(struct scatterlist * · line 1896

Quoted 10 lines

drivers/scsi/sg.c +2 -1

 
 	for (i=0; i < nr_pages; i++) {
-		if (dirtied && !PageReserved(sgl[i].page))
+		if (dirtied)
 			SetPageDirty(sgl[i].page);
 		/* unlock_page(sgl[i].page); */
+		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
 		/* FIXME: cache flush missing for rw==READ
 		 * FIXME: call the correct reference counting function
 		 */

An answer should be devised for this. My numerous SCSI CD-ROM devices (I have 5 across several different machines of several different arches) are rather unlikely to be happy with /* FIXME: XXX ... as an answer.

Quoted 7 lines · Nick Piggin

Index: linux-2.6/drivers/scsi/st.c
===================================================================

drivers/scsi/st.c +0 -0

@@ -4435,8 +4435,9 @@ static int sgl_unmap_user_pages(struct s
 	int i;
 

drivers/scsi/st.c static int sgl_unmap_user_pages(struct s · line 4443

Quoted 8 lines

drivers/scsi/st.c +2 -1

 	for (i=0; i < nr_pages; i++) {
-		if (dirtied && !PageReserved(sgl[i].page))
+		if (dirtied)
 			SetPageDirty(sgl[i].page);
+		/* FIXME: XXX don't dirty/unmap VM_RESERVED regions? */
 		/* FIXME: cache flush missing for rw==READ
 		 * FIXME: call the correct reference counting function
 		 */

Mutatis mutandis for my SCSI tape drive.

Quoted 3 lines

An answer should be devised for this. My numerous SCSI CD-ROM devices (I have 5 across several different machines of several different arches) are rather unlikely to be happy with /* FIXME: XXX ... as an answer.

Quoted 1 line

Mutatis mutandis for my SCSI tape drive.

This scsi code is already rather wrong. There isn't much point in just setting PG_dirty and leaving the page marked as clean in the radix tree. As it is we'll lose data if the user reads it into a MAP_SHARED memory buffer.
set_page_dirty_lock() should be used here. That can sleep. The above two functions are called under write_lock_irqsave() (at least) and might be called from irq context (dunno). So we cannot use

set_page_dirty_lock() and we don't have a ref on the page's inode. We could use set_page_dirty() and be racy against page reclaim. But to get all this correct (and it's very incorrect now) we'd need to punt the page dirtying up to process context, along the lines of bio_check_pages_dirty().
Or, if st_unmap_user_pages() and sgl_unmap_user_pages() are not called from irq context then we should arrange for them to be called without locks held and use set_page_dirty_lock().

Quoted 1 line · William Lee Irwin III

Mutatis mutandis for my SCSI tape drive.

Review

Quoted 4 lines · Nick Piggin

(quoted diff) +4 -0

+		index = (address - vma->vm_start) >> PAGE_SHIFT;
+		index += vma->vm_pgoff;
+		index >>= PAGE_CACHE_SHIFT - PAGE_SHIFT;
+		page->index = index;

linear_page_index()