Page MenuHomeFreeBSD

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

Authored by alc on Dec 11 2016, 10:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 15, 1:55 PM
Unknown Object (File)
Fri, Mar 15, 1:51 PM
Unknown Object (File)
Jan 9 2024, 5:50 AM
Unknown Object (File)
Jan 9 2024, 5:49 AM
Unknown Object (File)
Jan 9 2024, 5:49 AM
Unknown Object (File)
Jan 9 2024, 5:49 AM
Unknown Object (File)
Jan 9 2024, 5:49 AM
Unknown Object (File)
Jan 9 2024, 4:52 AM
Subscribers
None

Details

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 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.
kib edited edge metadata.

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

alc retitled this revision from Eliminate every mention of PG_CACHED pages from vm_page.c's comments to Eliminate every mention of PG_CACHED pages from the comments under vm/.
alc updated this object.
alc edited the test plan for this revision. (Show Details)
alc edited edge metadata.

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.

markj edited edge metadata.
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 ↗(On Diff #22826)

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
vm/vm_reserv.c
951 ↗(On Diff #22826)

Yes, it was.

alc edited edge metadata.

Revert "satisfies" to "satisfy".

This revision now requires review to proceed.Dec 12 2016, 7:17 AM
kib edited edge metadata.
kib added inline comments.
vm/vm_object.h
84 ↗(On Diff #22835)

(f) annotation explanation can be removed from the list, it is on the next line after the patch context.

vm/vm_reserv.c
199 ↗(On Diff #22835)

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.

vm/vm_object.h
84 ↗(On Diff #22835)

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.

vm/vm_reserv.c
199 ↗(On Diff #22835)

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.

I'll make another note to deal with this separately, since it will also immediately apply to older branches.

This revision was automatically updated to reflect the committed changes.