Page MenuHomeFreeBSD

vfs: eliminate v_iflag read from vn_lock
AbandonedPublic

Authored by mjg on Dec 4 2019, 2:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 2:04 AM
Unknown Object (File)
Sat, Apr 27, 2:03 AM
Unknown Object (File)
Sat, Apr 27, 2:01 AM
Unknown Object (File)
Sat, Apr 27, 2:01 AM
Unknown Object (File)
Sat, Apr 27, 12:33 AM
Unknown Object (File)
Jan 31 2024, 9:42 AM
Unknown Object (File)
Jan 31 2024, 5:23 AM
Unknown Object (File)
Jan 14 2024, 5:18 AM
Subscribers

Details

Reviewers
kib
jeff
imp
Summary

v_iflag shares the cacheline with v_usecount and makes the routine show up in profiles due to cache-line ping pong.

The var is read only to check for VI_DOOMED. Shorten v_type to make more room and provide v_doomed to indicate the same information from a read-mostly area.

With the patch the routine disappears from flamegraphs during -j 104 buildkernel.

Test Plan

will ask pho

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

This revision is now accepted and ready to land.Dec 4 2019, 4:47 AM

There is a precedence for VN_* macros operating on vnodes. I don't think we have other VNODE_ macros. Please rename for consistency and this is otherwise fine.

Given the complaint raised in D22689 I suggest the following.

Key observation is that the enum holding the type does not have to be 4 bytes wide. Thus I shorten it to 1 byte, freeing up 3 extra bytes for use. In that place I put v_doomed which can also be read without suffering cache misses.

This poses a problem in that this boolean duplicates information, but I think given the few asserts I added it's worth it (and a tree-wide replacement is not warranted). Another approach would move v_iflag to the same area as there is a 4 byte gap between v_type and v_op. However, I find it highly undesirable as it would put more constraints on what we can move around in the vnode without suffering cacheline ping pong during concurrent use.

This revision now requires review to proceed.Dec 5 2019, 4:37 PM
mjg edited the summary of this revision. (Show Details)
  • tighten up the assertion

Can we not add such hacks to the already quite complicated VFS, please ?
If you want v_doomed, then VI_DOOMED should go away.

But I would start with another your proposal and add an assert that we never see non-deadfs v_ops vector for doomed vnode. If stress2 and some reasonable wait time after the commit indicate that there is no drop of the vnode lock in vgonel(), then this should be the best route.

This change was supposed to be a stop gap before a more time consuming real solution is implemented. Given the feedback I decided to just go for it, see D22715

Adding the assertion at hand may be useful on its own right, but waiting for its results would only stall the work at hand.