Eliminate every mention of PG_CACHED pages from the comments under vm/
ClosedPublic
Actions

Authored by alc on Dec 11 2016, 10:08 PM.

Details

Reviewers

kib
markj

Commits

rS309898: Eliminate every mention of PG_CACHED pages from the comments in the machine-

Summary

Test Plan

I took some liberties revising the nearby text of some comments in vm_page.{c,h}, e.g., vm_page_advise(). Please make sure that you're ok with the revised text.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

alc updated this revision to Diff 22819.Dec 11 2016, 10:08 PM

alc retitled this revision from to Eliminate every mention of PG_CACHED pages from vm_page.c's comments.

alc updated this object.

alc edited the test plan for this revision. (Show Details)

alc added reviewers: kib, markj.

There seems to be not many other references to cache that mean the eliminated PG_CACHE facility in sys/vm, I only see something in vm_object.h and vm_reserv.c.

This revision is now accepted and ready to land.Dec 11 2016, 10:28 PM

markj accepted this revision.Dec 11 2016, 10:33 PM

markj edited edge metadata.

In D8752#181330, @kib wrote:

There seems to be not many other references to cache that mean the eliminated PG_CACHE facility in sys/vm, I only see something in vm_object.h and vm_reserv.c.

You inspired me to find them all. :-)

I've also corrected the misuse of hyphens after words ending in -ly in vm_reserv.c, because there were several of these misuses that were adjacent to a mention of PG_CACHED pages.

Finally, I've updated the comment describing the four types of page queues.

This revision now requires review to proceed.Dec 12 2016, 1:01 AM

I couldn't decide whether the comments at the head of default_pager_dealloc() and default_pager_putpages() were referring to PG_CACHED pages. Also, I'm not sure that part of the comment describing the latter is factually correct.

In D8752#181427, @alc wrote:

I couldn't decide whether the comments at the head of default_pager_dealloc() and default_pager_putpages() were referring to PG_CACHED pages. Also, I'm not sure that part of the comment describing the latter is factually correct.

Reading some of the code in sys/vm@r42957, it looks like "cache list" refers to the PQ_CACHE lists removed in r172317.

I think the comment above default_pager_putpages() has always been bogus: before that revision, the OBJT_DEFAULT->OBJT_SWAP conversion was done in default_pager_putpages() or in default_pager_convert_to_swapq(). The latter was invoked in a few different circumstances from vm_map code; after that revision, it's only done at the beginning of swap_pager_putpages(). However, I'm not clear on which cache->free transition that comment might have been referring to.

vm/vm_reserv.c
951	Wasn't "satisfy" more correct? The request specifies constraints on (a run of) pages, not on the reservation itself.

This revision is now accepted and ready to land.Dec 12 2016, 2:02 AM

alc added inline comments.Dec 12 2016, 7:14 AM

vm/vm_reserv.c
951	Yes, it was.

Revert "satisfies" to "satisfy".

This revision now requires review to proceed.Dec 12 2016, 7:17 AM

kib accepted this revision.Dec 12 2016, 12:02 PM

kib edited edge metadata.

kib added inline comments.

vm/vm_object.h
84	(f) annotation explanation can be removed from the list, it is on the next line after the patch context.
vm/vm_reserv.c
199	Finish the defining sentence with the dot ?

This revision is now accepted and ready to land.Dec 12 2016, 12:02 PM

In D8752#181444, @markj wrote:

In D8752#181427, @alc wrote:

I couldn't decide whether the comments at the head of default_pager_dealloc() and default_pager_putpages() were referring to PG_CACHED pages. Also, I'm not sure that part of the comment describing the latter is factually correct.

Reading some of the code in sys/vm@r42957, it looks like "cache list" refers to the PQ_CACHE lists removed in r172317.

I think the comment above default_pager_putpages() has always been bogus: before that revision, the OBJT_DEFAULT->OBJT_SWAP conversion was done in default_pager_putpages() or in default_pager_convert_to_swapq(). The latter was invoked in a few different circumstances from vm_map code; after that revision, it's only done at the beginning of swap_pager_putpages(). However, I'm not clear on which cache->free transition that comment might have been referring to.

If the comment is confusing even to you, I do not expect the comment giving any goods to the random reader who is not acquainted with the VM code. I think it is more useful to remove that parts then to try to find some bits of meaning.

alc added inline comments.Dec 12 2016, 4:33 PM

vm/vm_object.h
84	Access to the object's reservation list is synchronized by the free page queues lock. I'll add an item to my TODO list to add the lock annotations to all of the fields. Since that could be MFCed immediately, I'll keep it separate from this change.

alc added inline comments.Dec 12 2016, 4:56 PM

vm/vm_reserv.c
199	I'm uncomfortable with adding a period here because it's not really a sentence without a verb.

In D8752#181503, @kib wrote:

In D8752#181444, @markj wrote:

In D8752#181427, @alc wrote:

I couldn't decide whether the comments at the head of default_pager_dealloc() and default_pager_putpages() were referring to PG_CACHED pages. Also, I'm not sure that part of the comment describing the latter is factually correct.

Reading some of the code in sys/vm@r42957, it looks like "cache list" refers to the PQ_CACHE lists removed in r172317.

I think the comment above default_pager_putpages() has always been bogus: before that revision, the OBJT_DEFAULT->OBJT_SWAP conversion was done in default_pager_putpages() or in default_pager_convert_to_swapq(). The latter was invoked in a few different circumstances from vm_map code; after that revision, it's only done at the beginning of swap_pager_putpages(). However, I'm not clear on which cache->free transition that comment might have been referring to.

If the comment is confusing even to you, I do not expect the comment giving any goods to the random reader who is not acquainted with the VM code. I think it is more useful to remove that parts then to try to find some bits of meaning.