Page MenuHomeFreeBSD

(umaperf 1/7) Save an extra cache line access for large buckets as well as a few branches.
ClosedPublic

Authored by jeff on Dec 15 2019, 11:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 14, 9:23 PM
Unknown Object (File)
Mar 22 2024, 10:31 PM
Unknown Object (File)
Mar 22 2024, 10:31 PM
Unknown Object (File)
Mar 22 2024, 10:31 PM
Unknown Object (File)
Mar 22 2024, 10:31 PM
Unknown Object (File)
Mar 8 2024, 5:47 PM
Unknown Object (File)
Jan 6 2024, 4:20 PM
Unknown Object (File)
Jan 6 2024, 4:20 PM
Subscribers

Details

Summary

This is part of a series of patches intended to enable first-touch numa policies for UMA by default. It also reduces the cost of uma_zalloc/zfree by approximately 30% each in my tests.

The concept here is that we embed the entries and cnt for each bucket loaded into a per-cpu cache directly in the per-cpu cache structure. This means we can check how much space is available without touching a bucket. For large buckets the bucket header won't be in the same cache line as the bucket item in many cases so this saves a cache miss.

I like the simplification in branches, loop logic, and summary routines that results. The overhead of managing the separate bucket type is minimal and provides an opportunity for extra asserts.

Diff Detail

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

Event Timeline

jeff added reviewers: mav, markj, rlibby, glebius, gallatin.
jeff set the repository for this revision to rS FreeBSD src repository - subversion.

Looks good, just some nitpicks.

sys/vm/uma_core.c
557–559 ↗(On Diff #65693)

To me it would read better to ifdef INVARIANTS this, since it's not just that we are only checking this under INVARIANTS but that we are only maintaining it under INVARIANTS.

Also, blank line for style.

...ah, I see this is c&p.

565–569 ↗(On Diff #65693)

I might suggest renaming procedures with s/load/unload/ and s/store/load/, i.e. you load and unload the bucket into the PCPU cache. (You even use the "load" terminology in your summary.)

2950 ↗(On Diff #65693)

Line length.

4345–4352 ↗(On Diff #65693)

Stale comment.

This revision is now accepted and ready to land.Dec 16 2019, 7:01 AM
sys/vm/uma_core.c
538 ↗(On Diff #65693)

zone unused in cache_bucket_push/pop.

sys/vm/uma_core.c
565–569 ↗(On Diff #65693)

I do think that is a bit more clear.

markj added inline comments.
sys/vm/uma_core.c
565–569 ↗(On Diff #65693)

+1. That naming makes a lot more sense if you think in terms of the "magazine" terminology of the original paper.

572 ↗(On Diff #65693)

Can we CRITICAL_ASSERT() here?

632 ↗(On Diff #65693)

This is inconsistent with cache_bucket_load_cross(), which is not conditionally compiled.

642 ↗(On Diff #65693)

ucb_spare doesn't seem to be defined anywhere. Did the routine use memcpy() at some point? I think that might be cleaner.

sys/vm/uma_core.c
642 ↗(On Diff #65693)

I see you put these spare bytes to use. This comment just belongs in the next patch, then.

Review feedback. Renaming, style, etc.

This revision now requires review to proceed.Dec 21 2019, 8:45 PM
This revision is now accepted and ready to land.Dec 22 2019, 9:35 PM