Page MenuHomeFreeBSD

vfs: stop reading ->v_vnlock in default locking routines
AbandonedPublic

Authored by mjg on Mon, Nov 25, 6:28 PM.

Details

Reviewers
kib
Summary

For typical users the pointer is set to the lock in the same vnode, but it shares the cacheline. As a result it adds to cache misses from other CPUs during concurrent access and makes vop_stdlock/vop_stdunlock show up in profiles.

Test Plan

will ask pho

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 27765

Event Timeline

mjg created this revision.Mon, Nov 25, 6:28 PM
kib added inline comments.Mon, Nov 25, 9:07 PM
sys/kern/vfs_default.c
551

above

sys/ufs/ufs/ufs_vnops.c
2743

Why do you need the ops to ufs vector ? Isn't ffs enough, it is stacked over ufs.

mjg added inline comments.Mon, Nov 25, 9:18 PM
sys/ufs/ufs/ufs_vnops.c
2743

I found this in ufs/ffs/ffs_snapshot.c:

vp->v_vnlock = &sn->sn_lock;
lockmgr(&vp->v_lock, LK_RELEASE, NULL);

I don't want to take chances on this one.

kib added inline comments.Mon, Nov 25, 9:45 PM
sys/ufs/ufs/ufs_vnops.c
2743

Yes, this is a main reason why v_vnlock was introduced at all.

But I do not undersrtand why do you need vops for UFS. We only create vnodes with FFS vop vectors. Do you have an example where we instantiate a vnode with UFS vnodeops ?

mjg added inline comments.Mon, Nov 25, 11:52 PM
sys/ufs/ufs/ufs_vnops.c
2743

Currently common ops like shared vnode locking perform avoidable memory accesses. On top of that lockmgr provides features which are not needed by neither of tmpfs, zfs, devfs and few others; while said support comes with a performance hit stemming from additional accesses. Thus I'm trying to decouple "minimal" lockmgr which still works for the first group, while providing separate entry points for the rest. As such I don't think it is worth the effort to find out if a specific vop vector in ufs code can get away with the simpler variant.

kib added inline comments.Tue, Nov 26, 1:46 PM
sys/ufs/ufs/ufs_vnops.c
2743

The patch adds useless lines which will confuse anybody trying to understand what is going on.

mjg abandoned this revision.Wed, Dec 4, 3:18 AM

I decided to go with a different approach. D22665