Page MenuHomeFreeBSD
Differential D56611

Convert bunch of macros from sys/mount.h into static inlines
Needs ReviewPublic
Actions

Authored by kib on Fri, Apr 24, 2:14 AM.
Tags
None
Referenced Files
F155123535: D56611.diff
Fri, May 1, 1:16 PM
Unknown Object (File)
Thu, Apr 30, 8:17 AM
Unknown Object (File)
Wed, Apr 29, 11:15 AM
Unknown Object (File)
Wed, Apr 29, 11:00 AM
Unknown Object (File)
Wed, Apr 29, 11:00 AM
Unknown Object (File)
Tue, Apr 28, 2:33 PM
Unknown Object (File)
Tue, Apr 28, 9:23 AM
Unknown Object (File)
Tue, Apr 28, 7:41 AM
View All 18 Files
Subscribers
imp
olce

Details

Reviewers
kevans
markj
Summary

This gives type safety without loosing compiler optimizations

vfs: convert vfs_op_thread_* macros to static inlines


sys/vnode.h: convert VN_KNOTE() to function


vfs: convert VFS_OPs from macros to static inlines


sys/vnode.h: remove stale comment

The source sweep is not going to happen.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Fri, Apr 24, 2:14 AM
Herald added subscribers: olce, imp. · View Herald TranscriptFri, Apr 24, 2:14 AM
kib requested review of this revision.Fri, Apr 24, 2:14 AM
markj added inline comments.Fri, Apr 24, 1:14 PM
sys/sys/mount.h
859
sys/sys/vnode.h
228

The list-is-empty check is no longer inlined, now it requires an extra function call in the common case where there are no knotes present.

I'd actually like to add a new VIRF flag which gets set when the knote list is non-empty, similar to VIRF_INOTIFY. Then, instead of checking v->pollinfo != NULL && !KNLIST_EMPTY(&...) and vp->v_irflag & VIRF_INOTIFY separately in various VOP _post implementations, they can look like this:

void
vop_open_post(void *ap, int rc)
{
    struct vop_open_args *a = ap;

    if (rc == 0 && __predict_false((a->a_vp->v_irflag & (VIRF_INOTIFY | VIRF_KNOTE)))) {
        <slow path>
    }
}
kib updated this revision to Diff 176424.Fri, Apr 24, 7:36 PM
kib marked an inline comment as done.

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

markj added a comment.Sat, Apr 25, 11:22 PM
In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

I'm not sure why it was done this way. Maybe it's an optimization, since many VOPs already hold the vnode lock when they scan the list?

sys/kern/vfs_subr.c
5329

Shouldn't we set this in vfs_kqfilter(), not here?

5821

Maybe vop_check_pollinfo()? And I would annotate this with __predict_false.

kib marked 3 inline comments as done.Sat, Apr 25, 11:48 PM
In D56611#1297270, @markj wrote:
In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

I'm not sure why it was done this way. Maybe it's an optimization, since many VOPs already hold the vnode lock when they scan the list?

kib updated this revision to Diff 176525.Sat, Apr 25, 11:50 PM

Set VIRF_KNOTE in vfs_kqfilter(), where it should be, on inserting knote into the knlist.
Rename vop_check_knote() to vop_check_pollinfo().

kib updated this revision to Diff 176526.Sat, Apr 25, 11:52 PM

Do not set VIRF_KNOTE in knlist unlock, vfs_kqfilter() should be enough.

markj added a comment.Sun, Apr 26, 1:53 AM
In D56611#1297281, @kib wrote:
In D56611#1297270, @markj wrote:
In D56611#1296692, @kib wrote:

Coded VIRF_KNOTE.

The issue I see is that for vn_pollrecord() must interlock the enqueuing of of a knote with setting the VIRF_KNOTE flag, which creates the order between the vnode interlock and pollinfo vpi_lock mutex. I do not think there was an order before, then it should be fine for now. But even if it is, it is somewhat limiting the future code modifications.

Hmm but the knote list is locked by the vnode lock, not by the pollinfo lock. See vfs_knllock().

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

My main reason for putting it in v_irflag was because the flag is checked "frequently." If we had a frequently read flag field which is synchronized by the vnode lock, then that would be fine too.

kib added a comment.Sun, Apr 26, 2:14 AM
In D56611#1297289, @markj wrote:
In D56611#1297281, @kib wrote:

Right. And with the correction of setting the flag in vfs_kqfilter, the flag is naturally protected by the vnode lock. This makes it more natural to have it in v_vflag than in v_irflag (but I have same opinion about VIRF_INOTIFY).

My main reason for putting it in v_irflag was because the flag is checked "frequently." If we had a frequently read flag field which is synchronized by the vnode lock, then that would be fine too.

We have 7 bytes unused in the v_rl rangelock member of struct vnode. I will propose something after this batch is landed.

markj added inline comments.Mon, Apr 27, 10:04 PM
sys/kern/vfs_subr.c
6726–6731

Why vholdl()?

When I run the regression test suite, the sys/kern/path_test tests trigger a panic:

VNASSERT failed: mtx_owned(VI_MTX(vp)) not true at /home/markj/sb/main/src/sys/kern/vfs_subr.c:5761 (assert_vi_locked)                                         
0xfffffe019af43dc0: type VREG state VSTATE_CONSTRUCTED op 0xffffffff830c6380                                                                                   
    usecount 1, writecount 0, refcount 3 seqc users 0                                                                                                          
    hold count flags ()                                                                                                                                        
    flags (VIRF_PGREAD|VMP_LAZYLIST)                                                                                                                           
    v_object 0xfffffe01991cec30 ref 0 pages 1 cleanbuf 0 dirtybuf 1                                                                                            
    lock type ufs: UNLOCKED                                                                                                                                    
        nlink=1, effnlink=1, size=1024, extsize=0                                                                                                              
        generation=604727da, uid=0, gid=0, flags=0x0                                                                                                                                                                                                                                                                          
        ino 886830, on dev gpt/rootfs                                                                                                                          
panic: vfs_kqfilter: vnode interlock is not locked but should be                                                                                               
cpuid = 11                                                                                                                                                     
time = 1777327121                                                                                                                                              
KDB: stack backtrace:                                                                                                                                          
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe00f17cdfd0                                                                                 
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe00f17ce130                                                                                                                                                                                                                                                                
vpanic() at vpanic+0x214/frame 0xfffffe00f17ce2d0                                                                                                                                                                                                                                                                             
panic() at panic+0xb5/frame 0xfffffe00f17ce390                                                                                                                 
vfs_kqfilter() at vfs_kqfilter+0x2a7/frame 0xfffffe00f17ce440                                                                                                  
VOP_KQFILTER_APV() at VOP_KQFILTER_APV+0x7d/frame 0xfffffe00f17ce470                                                                                           
vn_kqfilter_opath() at vn_kqfilter_opath+0xe1/frame 0xfffffe00f17ce530                                                                                         
kqueue_register() at kqueue_register+0xa24/frame 0xfffffe00f17ce690                                                                                            
kqueue_kevent() at kqueue_kevent+0x1b4/frame 0xfffffe00f17ce9c0                                                                                                
kern_kevent_fp() at kern_kevent_fp+0xce/frame 0xfffffe00f17cea10                                                                                                                                                                                                                                                              
kern_kevent() at kern_kevent+0x12f/frame 0xfffffe00f17ceaf0                                                                                                                                                                                                                                                                   
kern_kevent_generic() at kern_kevent_generic+0x135/frame 0xfffffe00f17cebd0                                                                                                                                                                                                                                                   
sys_kevent() at sys_kevent+0x232/frame 0xfffffe00f17ced10                                                                                                                                                                                                                                                                     
amd64_syscall() at amd64_syscall+0x3d5/frame 0xfffffe00f17cef30                                                                                                                                                                                                                                                               
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00f17cef30                                                                                     
--- syscall (560, FreeBSD ELF64, kevent), rip = 0xdd0a4b1420a, rsp = 0xdd0a201fc88, rbp = 0xdd0a2020110 ---
kib marked an inline comment as done.Mon, Apr 27, 11:56 PM
kib added inline comments.
sys/kern/vfs_subr.c
6726–6731

Sorry for it. The change was a left-over from using the interlock for setting the flag, before I changed to vnode lock.

kib updated this revision to Diff 176684.Tue, Apr 28, 12:00 AM
kib marked an inline comment as done.

vhold

Revision Contents
Changeset List

PathSize
sys/
kern/
4 lines
6 lines
61 lines
4 lines
sys/
293 lines
27 lines

Diff 176525

View Options

sys/kern/vfs_cache.c

Loading...
View Options

sys/kern/vfs_mount.c

Loading...
View Options

sys/kern/vfs_subr.c

Loading...
View Options

sys/kern/vfs_vnops.c

Loading...
View Options

sys/sys/mount.h

Loading...
View Options

sys/sys/vnode.h

Loading...