Page MenuHomeFreeBSD

amd64 pmap: batch chunk removal in pmap_remove_pages
ClosedPublic

Authored by mjg on Sep 28 2019, 6:40 PM.

Details

Summary

pv list lock is the main bottleneck during poudriere -j 104 and pmap_remove_pages is the most impactful consumer. It frees chunks with the lock held even though it plays no role in correctness. Moreover chunks are often freed in groups, sample counts during buildkernel (0-sized frees removed):

value  ------------- Distribution ------------- count
      0 |                                         0
      1 |                                         8
      2 |@@@@@@@                                  19329
      4 |@@@@@@@@@@@@@@@@@@@@@@                   58517
      8 |                                         1085
     16 |                                         71
     32 |@@@@@@@@@@                               24919
     64 |                                         899
    128 |                                         7
    256 |                                         2
    512 |                                         0
  1. batch freeing
  2. move it past unlocking pv list

I did not bother benchmarking this change in isolation, see another review for details.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjg created this revision.Sep 28 2019, 6:40 PM
markj added inline comments.Sep 28 2019, 6:52 PM
sys/amd64/amd64/pmap.c
1111 ↗(On Diff #62690)

It should be called pv_chunklist.

Missing space after the comma.

6923 ↗(On Diff #62690)

Call it free_chunks?

7095 ↗(On Diff #62690)

Why does the pmap lock need to be held over free_pv_chunk_batch?

markj added a reviewer: alc.Sep 28 2019, 6:52 PM
markj added inline comments.
sys/amd64/amd64/pmap.c
7095 ↗(On Diff #62690)

(It's because the chunks are still in the global LRU.)

mjg updated this revision to Diff 62694.Sep 28 2019, 7:28 PM
  • chunklist -> pv_chunklist
  • free_chunk -> free_chunks
markj accepted this revision.Sep 28 2019, 7:33 PM
This revision is now accepted and ready to land.Sep 28 2019, 7:33 PM
alc accepted this revision.Sep 28 2019, 8:38 PM
kib accepted this revision.Sep 29 2019, 9:31 AM
kib added inline comments.
sys/amd64/amd64/pmap.c
4295 ↗(On Diff #62694)

_<anything> is the implementation reseverd namespace (for file-scoped definitions). In recent times, we try to not abuse reserved namespace in kernel.

mjg added inline comments.Sep 29 2019, 6:24 PM
sys/amd64/amd64/pmap.c
7095 ↗(On Diff #62690)

I can split this into dequeue in the current func and actual free after unlocking pmap.

4295 ↗(On Diff #62694)

What should I rename this to? free_pv_chunk_dequeued?

kib added inline comments.Sep 29 2019, 7:55 PM
sys/amd64/amd64/pmap.c
4295 ↗(On Diff #62694)

I am fine with this name.

This revision was automatically updated to reflect the committed changes.
alc added inline comments.Sep 29 2019, 9:21 PM
sys/amd64/amd64/pmap.c
7095 ↗(On Diff #62690)

I can split this into dequeue in the current func and actual free after unlocking pmap.

We're tearing down the address space. (Note the assertion at the start of this function that no other threads are active within this pmap.) So, moving the actual free until after unlocking the pmap isn't going to have any consequential effect.

markj added inline comments.Sep 29 2019, 10:06 PM
sys/amd64/amd64/pmap.c
7095 ↗(On Diff #62690)

We're tearing down the address space. (Note the assertion at the start of this function that no other threads are active within this pmap.) So, moving the actual free until after unlocking the pmap isn't going to have any consequential effect.

I agree in principle, but note that pmap_remove_pages() may still block the page daemon when it calls pmap_ts_referenced() on pages mapped into the pmap. This has systemic effects since the page daemon may be trying to reclaim pages. It can even cause problems during the background active queue scan, since the page daemon holds a page lock while calling pmap_ts_referenced(). This actually appears in practice when a large process forks frequently, since pmap_copy() will hold the pmap lock for a long time. It is even worse on 11.x, where the page daemon is also holding the active queue lock during scans and thus blocks concurrent page faults.

In this case though I suspect that the gain from freeing the pv chunk pages outside of the pmap lock is probably not significant enough to be worth doing.