Avoid unnecessary object locking in vm_page_grab_pages_unlocked().
ClosedPublic
Actions

Authored by markj on Sep 2 2020, 7:35 PM.

Details

Reviewers

alc
kib
jeff
dougm
cem

Commits

rS365275: Avoid unnecessary object locking in vm_page_grab_pages_unlocked().

Summary

Conrad pointed out that we are needlessly acquiring the object lock to
call vm_page_grab_pages() even when all of the requested pages were
looked up locklessly. Fix that, and add assertions to help catch this
kind of mistake.

Reported by: cem

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj requested review of this revision.Sep 2 2020, 7:35 PM

markj created this revision.

Harbormaster completed remote builds in B33342: Diff 76560.Sep 2 2020, 7:35 PM

markj added reviewers: alc, kib, jeff, dougm, cem.Sep 2 2020, 7:36 PM

hilarious, thank you.

jeff accepted this revision.Sep 2 2020, 7:38 PM

This revision is now accepted and ready to land.Sep 2 2020, 7:38 PM

cem accepted this revision.Sep 2 2020, 7:40 PM

alc accepted this revision.Sep 2 2020, 7:47 PM

dougm added inline comments.Sep 2 2020, 7:49 PM

sys/vm/vm_page.c
4732 ↗	(On Diff #76560)	If count>0, what is this test for?

markj added inline comments.Sep 2 2020, 7:52 PM

sys/vm/vm_page.c
4732 ↗	(On Diff #76560)	Thanks. It was added in the initial commit of vm_page_grab_pages(). All callers of the function now specify count > 0, so we can remove it.

Handle Doug's note.

This revision now requires review to proceed.Sep 2 2020, 7:53 PM

Harbormaster completed remote builds in B33343: Diff 76561.Sep 2 2020, 7:53 PM

dougm accepted this revision.Sep 2 2020, 7:56 PM

This revision is now accepted and ready to land.Sep 2 2020, 7:56 PM

Closed by commit rS365275: Avoid unnecessary object locking in vm_page_grab_pages_unlocked(). (authored by markj). · Explain WhySep 2 2020, 7:59 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS365275: Avoid unnecessary object locking in vm_page_grab_pages_unlocked()..

Herald added a subscriber: imp. · View Herald TranscriptSep 2 2020, 7:59 PM

kib added inline comments.Sep 2 2020, 8:19 PM

head/sys/vm/vm_page.c
4785	.. but why making the count == 0 case special ? If a caller needs that, why create more burden to use VM KPI ?
4809	This is fine ...

markj added inline comments.Sep 2 2020, 8:50 PM

head/sys/vm/vm_page.c
4785	None of the 6 callers of vm_page_grab_pages() in stable/12 will ever specify count == 0. I can't really see a scenario where handling count == 0 would be useful, so I'm inclined to believe that it is better to disallow count == 0 so we can easily catch bugs like this one.

Revision Contents
Changeset List

Path

Size

head/

sys/

vm/

vm_page.c

9 lines

Diff 76562

View Options

Avoid unnecessary object locking in vm_page_grab_pages_unlocked().ClosedPublicActions