Page MenuHomeFreeBSD

vm_phys: Try to clean up NUMA KPIs
ClosedPublic

Authored by markj on Nov 13 2020, 3:50 PM.
Tags
None
Referenced Files
F81520511: D27207.id79786.diff
Wed, Apr 17, 11:55 AM
Unknown Object (File)
Sun, Mar 31, 6:03 PM
Unknown Object (File)
Sun, Mar 31, 6:03 PM
Unknown Object (File)
Sun, Mar 31, 5:59 PM
Unknown Object (File)
Sun, Mar 31, 5:59 PM
Unknown Object (File)
Sun, Mar 31, 5:51 PM
Unknown Object (File)
Dec 23 2023, 3:26 AM
Unknown Object (File)
Dec 3 2023, 6:32 PM
Subscribers

Details

Summary

It can useful for code outside the VM system to look up the NUMA domain
of a page backing a virtual or physical address, specifically when
creating NUMA-aware data structures. We have
_vm_phys_domain() for this, but the leading underscore implies that it's
an internal function, and vm_phys.h has dependencies on a number of
other headers. This diff tries to make _vm_phys_domain() a bit easier
to use:


- Rename vm_phys_domain() to vm_phys_page_domain(). That function is
only used by the VM; I think it's ok to use a somewhat longer name.
- Compile vm_phys_page_domain() only if vm_page.h is included, since it
references vm_page fields but is not often used.
- Conditionally define struct pglist, like we do in vm_object.h.
- Rename _vm_phys_domain() to vm_phys_domain() and provide an inline
implementation.
- Include machine/vmparam.h in vm_phys.h since the layout of struct
vm_phys_seg depends on VM_NRESERVLEVEL.
- Remove the size of phys_avail[] from the declaration, to avoid a
dependency on vm/vmparam.h. Note that mem_affinity[] is similarly
declared without an explicit size.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj requested review of this revision.Nov 13 2020, 3:50 PM
This revision is now accepted and ready to land.Nov 13 2020, 5:17 PM

I ran into a similar problem with respect to vm_phys_segs[] in eliminating iteration from the vm_dumpset operations. I wonder if we shouldn't separate the struct vm_phys_seg and array declarations into their own separate header file that is included by vm_param.h. Then, vm_page.h could implement vm_page_domain().

sys/vm/vm_phys.h
133 ↗(On Diff #79504)

In general, I'm a fan of longer names, especially ones that include a verb. :-) But, in this case, I would advocate for the shorter vm_page_domain(). The only thing that the phys_ prefix does is hint at where the function is implemented. But, conceptually this is an operation on a vm_page.

In D27207#607441, @alc wrote:

I ran into a similar problem with respect to vm_phys_segs[] in eliminating iteration from the vm_dumpset operations. I wonder if we shouldn't separate the struct vm_phys_seg and array declarations into their own separate header file that is included by vm_param.h. Then, vm_page.h could implement vm_page_domain().

Perhaps add a _vm_phys.h with the struct vm_phys_seg definition and vm_phys_seg_array declaration, and include that directly in vm_page.h, along with machine/vmparam.h? It feels a bit strange to add addition includes to vm_param.h.

  • Add _vm_phys.h
  • Rename vm_phys_page_domain() to vm_page_domain()
This revision now requires review to proceed.Nov 17 2020, 9:14 PM

Are there any objections to the current patch?

sys/vm/vm_page.c
3601 ↗(On Diff #79685)

Could this (or should this) be micro-optimized to avoid the second call to vm_page_domain() within vm_pagequeue_domain()?

sys/vm/vm_page.c
3601 ↗(On Diff #79685)

Yes, in fact I have that patch queued up in my tree. :)

In GENERIC-NODEBUG it didn't appear to have an effect on code generation though.

sys/vm/vm_pagequeue.h
388–393 ↗(On Diff #79685)

If I'm allowed to be nit-picky for the moment, I would observe that this function has nothing to do with page queues. :-) (To be clear, I'm not asking for a further change to this particular patch.)

This revision is now accepted and ready to land.Nov 19 2020, 12:24 AM
sys/vm/vm_pagequeue.h
388–393 ↗(On Diff #79685)

The naming of this file is kind of strange. vm_pagequeue.h's main purpose is to define struct vm_domain, which happens to embed the domain's page queues. We could perhaps rename it to vm_domain.h. I'm not sure what this particular function would logically be called, though.

This revision was automatically updated to reflect the committed changes.