Page MenuHomeFreeBSD
Differential D43388

devel/jetbrains-sqlite: Update to 233.11799.241
ClosedPublic
Actions

Authored by jbo on Jan 10 2024, 2:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 23, 5:41 PM
Unknown Object (File)
Jan 31 2025, 7:28 PM
Unknown Object (File)
Jan 20 2025, 6:09 AM
Unknown Object (File)
Dec 25 2024, 8:02 PM
Unknown Object (File)
Nov 14 2024, 5:03 PM
Unknown Object (File)
Oct 24 2024, 12:24 AM
Unknown Object (File)
Oct 16 2024, 6:45 AM
Unknown Object (File)
Oct 16 2024, 6:44 AM
View All 50 Files
Subscribers
None

Details

Reviewers
zirias
tcberner
Commits
R11:6be6f4974278: devel/jetbrains-sqlite: Update to 233.11799.241
Summary

Synchronized version with devel/jetbrains-* 2023.3.

PR: 275924

Diff Detail

Repository
R11 FreeBSD ports repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 55342
Build 52231: arc lint + arc unit

Event Timeline

jbo requested review of this revision.Jan 10 2024, 2:43 AM
jbo created this revision.
Harbormaster completed remote builds in B55342: Diff 132533.Jan 10 2024, 2:43 AM
tcberner accepted this revision.Jan 10 2024, 6:09 PM

lgtm

This revision is now accepted and ready to land.Jan 10 2024, 6:09 PM
Closed by commit R11:6be6f4974278: devel/jetbrains-sqlite: Update to 233.11799.241 (authored by dmitry.wagin_ya.ru, committed by jbo). · Explain WhyJan 14 2024, 5:15 PM
This revision was automatically updated to reflect the committed changes.
jbo added a commit: R11:6be6f4974278: devel/jetbrains-sqlite: Update to 233.11799.241.

Revision Contents
Changeset List

PathSize
devel/
jetbrains-sqlite/
11 lines
6 lines
files/
4 lines

Diff 132533

View Options

devel/jetbrains-sqlite/Makefile

Show First 20 Lines • Show All 220 Lines • ▼ Show 20 Lines#define mnt_endzero mnt_gjprovider
struct mtx mnt_listmtx; struct mtx mnt_listmtx;
struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */ struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */
int mnt_activevnodelistsize;/* (l) # of active vnodes */ int mnt_activevnodelistsize;/* (l) # of active vnodes */
struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */ struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */
int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */ int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */
struct lock mnt_explock; /* vfs_export walkers lock */ struct lock mnt_explock; /* vfs_export walkers lock */
TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */ TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */
TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/ TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/
int mnt_vfs_ops; /* (i) pending vfs ops */
int *mnt_thread_in_ops_pcpu;
}; };
/* /*
* Definitions for MNT_VNODE_FOREACH_ALL. * Definitions for MNT_VNODE_FOREACH_ALL.
*/ */
struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp);
struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp);
void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp); void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp);
Show All 23 Lines
#define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \ #define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \
__mnt_vnode_markerfree_active(&(mvp), (mp)) __mnt_vnode_markerfree_active(&(mvp), (mp))
#define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx) #define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx)
#define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx) #define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx)
#define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx) #define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx)
#define MNT_MTX(mp) (&(mp)->mnt_mtx) #define MNT_MTX(mp) (&(mp)->mnt_mtx)
#define MNT_REF_FASTPATH(mp) do { \
kibUnsubmitted
Not Done
Inline Actions

I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions right now to replace it.

kib: I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions…
mjgAuthorUnsubmitted
Done
Inline Actions

_UNLOCKED or (later) _PCPU?

mjg: _UNLOCKED or (later) _PCPU?
kibUnsubmitted
Not Done
Inline Actions

unlocked sounds fine

kib: unlocked sounds fine
atomic_add_int(&(mp)->mnt_ref, 1); \
} while (0)
#define MNT_REL_FASTPATH(mp) do { \
int _c; \
_c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
} while (0)
#define MNT_REF(mp) do { \ #define MNT_REF(mp) do { \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
(mp)->mnt_ref++; \ atomic_add_int(&(mp)->mnt_ref, 1); \
kibUnsubmitted
Not Done
Inline Actions

This should be MNT_REF_FASTPATH() ?

kib: This should be MNT_REF_FASTPATH() ?
mjgAuthorUnsubmitted
Done
Inline Actions

See below.

mjg: See below.
} while (0) } while (0)
#define MNT_REL(mp) do { \ #define MNT_REL(mp) do { \
int _c; \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
KASSERT((mp)->mnt_ref > 0, ("negative mnt_ref")); \ _c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
(mp)->mnt_ref--; \ KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
if ((mp)->mnt_ref == 0) \ if (_c == 0) \
kibUnsubmitted
Not Done
Inline Actions

If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it there too.

Can these four macros changed to inline functions ?

kib: If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it…
mjgAuthorUnsubmitted
Done
Inline Actions

I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a meaningful value so this will have to go back to doing work on its own. In order to keep things consistent I would prefer avoid changing the _REF_ variant either. But I don't have a strong opinion here.

As for inline, this requires adding several headers to struct mount.h and despite them being gated with _KERNEL it causes a lot of breakage during buildworld, e.g.:

/usr/src/lib/libprocstat/common_kvm.h:34: error: conflicting types for 'dev2udev'
/usr/obj/usr/src/sparc64.sparc64/tmp/usr/include/sys/systm.h:499: error: previous declaration of 'dev2udev' was here

I think conversion to inlines should be a separate effort, if done at all.

mjg: I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a…
kibUnsubmitted
Not Done
Inline Actions

Ok.
You might move that to a new header.

kib: Ok. You might move that to a new header.
wakeup((mp)); \ wakeup((mp)); \
} while (0) } while (0)
#endif /* _KERNEL */ #endif /* _KERNEL */
/* /*
* User specifiable flags, stored in mnt_flag. * User specifiable flags, stored in mnt_flag.
*/ */
▲ Show 20 Lines • Show All 650 Lines • ▼ Show 20 Lines
vfs_checkexp_t vfs_stdcheckexp; vfs_checkexp_t vfs_stdcheckexp;
vfs_init_t vfs_stdinit; vfs_init_t vfs_stdinit;
vfs_uninit_t vfs_stduninit; vfs_uninit_t vfs_stduninit;
vfs_extattrctl_t vfs_stdextattrctl; vfs_extattrctl_t vfs_stdextattrctl;
vfs_sysctl_t vfs_stdsysctl; vfs_sysctl_t vfs_stdsysctl;
void syncer_suspend(void); void syncer_suspend(void);
void syncer_resume(void); void syncer_resume(void);
void vfs_op_barrier_wait(struct mount *);
void vfs_op_enter(struct mount *);
void vfs_op_exit_locked(struct mount *);
void vfs_op_exit(struct mount *);
jeffUnsubmitted
Not Done
Inline Actions

Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop to handle the whole business?

If you wanted to do it with multiple cpus you need only set the bit in each counter.

jeff: Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop…
mjgAuthorUnsubmitted
Done
Inline Actions

Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a discussion on whether given per-cpu routine works and/or is a good idea. Whatever ends up protecting the fast path is trivially replaceable.

Main point of *this* review is to (dis)agree on the general approach - that is wrapping all code which requires an exact value with fast path enable/disable routines, with a guarantee everyone executing after the disabling routine returns goes for the current code. Assuming this is fine, I'll post an equivalent for busy/unbusy and write suspension. With this in in place we can discuss how to handle per-cpu, unless you are very opposed to rmlocks showing up in the first place even if temporarily (in which case I can elaborate on what scheme I had in mind).

mjg: Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a…
mjgAuthorUnsubmitted
Done
Inline Actions

Perhaps I should add I do think making the counters atomic in initial commits is an important logical step, especially when it comes to write suspension -- it required a little patching in SU code. It really just needs *something* to keep the fast path. You can see the previous iteration here https://people.freebsd.org/~mjg/patches/atomic-mnt-counters.diff

But again, if the intermediate work is not seen as necessary, I would be more than happy to go straight to per-cpu variants.

mjg: Perhaps I should add I do think making the counters atomic in initial commits is an important…
/*
* We mark ourselves as entering the section and post a sequentially consistent
kibUnsubmitted
Not Done
Inline Actions

Please use the 'sequentially consistent fence' and not 'full barrier' term.

kib: Please use the 'sequentially consistent fence' and not 'full barrier' term.
* fence, meaning the store is completed before we get into the section and
* mnt_vfs_ops is only read afterwards.
*
* Any thread transitioning the ops counter 0->1 does things in the opposite
* order - first bumps the count, posts a sequentially consistent fence and to
* observes all CPUs not executing within the section.
*
* This provides an invariant that by the time the last CPU is observed not
* executing, everyone else entering will see the counter > 0 and exit.
*
* Note there is no barrier between vfs_ops and the rest of the code in the
* section. It is not necessary as the writer has to wait for everyone to drain
* before making any changes or only make safe changes.
*/
#define vfs_op_thread_enter(mp) ({ \
kibUnsubmitted
Not Done
Inline Actions

Can this be inline function ?

kib: Can this be inline function ?
struct mount *_mp = (mp); \
bool _retval = true; \
critical_enter(); \
*(int *)zpcpu_get(_mp->mnt_thread_in_ops_pcpu) = 1; \
atomic_thread_fence_seq_cst(); \
if (__predict_false(_mp->mnt_vfs_ops > 0)) { \
vfs_op_thread_exit(_mp); \
_retval = false; \
} \
_retval; \
})
#define vfs_op_thread_exit(mp) do { \
atomic_thread_fence_rel(); \
*(int *)zpcpu_get(mp->mnt_thread_in_ops_pcpu) = 0; \
critical_exit(); \
} while (0)
#else /* !_KERNEL */ #else /* !_KERNEL */
#include <sys/cdefs.h> #include <sys/cdefs.h>
struct stat; struct stat;
__BEGIN_DECLS __BEGIN_DECLS
Show All 24 Lines
View Options

devel/jetbrains-sqlite/distinfo

Show First 20 Lines • Show All 220 Lines • ▼ Show 20 Lines#define mnt_endzero mnt_gjprovider
struct mtx mnt_listmtx; struct mtx mnt_listmtx;
struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */ struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */
int mnt_activevnodelistsize;/* (l) # of active vnodes */ int mnt_activevnodelistsize;/* (l) # of active vnodes */
struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */ struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */
int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */ int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */
struct lock mnt_explock; /* vfs_export walkers lock */ struct lock mnt_explock; /* vfs_export walkers lock */
TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */ TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */
TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/ TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/
int mnt_vfs_ops; /* (i) pending vfs ops */
int *mnt_thread_in_ops_pcpu;
}; };
/* /*
* Definitions for MNT_VNODE_FOREACH_ALL. * Definitions for MNT_VNODE_FOREACH_ALL.
*/ */
struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp);
struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp);
void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp); void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp);
Show All 23 Lines
#define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \ #define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \
__mnt_vnode_markerfree_active(&(mvp), (mp)) __mnt_vnode_markerfree_active(&(mvp), (mp))
#define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx) #define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx)
#define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx) #define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx)
#define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx) #define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx)
#define MNT_MTX(mp) (&(mp)->mnt_mtx) #define MNT_MTX(mp) (&(mp)->mnt_mtx)
#define MNT_REF_FASTPATH(mp) do { \
kibUnsubmitted
Not Done
Inline Actions

I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions right now to replace it.

kib: I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions…
mjgAuthorUnsubmitted
Done
Inline Actions

_UNLOCKED or (later) _PCPU?

mjg: _UNLOCKED or (later) _PCPU?
kibUnsubmitted
Not Done
Inline Actions

unlocked sounds fine

kib: unlocked sounds fine
atomic_add_int(&(mp)->mnt_ref, 1); \
} while (0)
#define MNT_REL_FASTPATH(mp) do { \
int _c; \
_c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
} while (0)
#define MNT_REF(mp) do { \ #define MNT_REF(mp) do { \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
(mp)->mnt_ref++; \ atomic_add_int(&(mp)->mnt_ref, 1); \
kibUnsubmitted
Not Done
Inline Actions

This should be MNT_REF_FASTPATH() ?

kib: This should be MNT_REF_FASTPATH() ?
mjgAuthorUnsubmitted
Done
Inline Actions

See below.

mjg: See below.
} while (0) } while (0)
#define MNT_REL(mp) do { \ #define MNT_REL(mp) do { \
int _c; \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
KASSERT((mp)->mnt_ref > 0, ("negative mnt_ref")); \ _c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
(mp)->mnt_ref--; \ KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
if ((mp)->mnt_ref == 0) \ if (_c == 0) \
kibUnsubmitted
Not Done
Inline Actions

If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it there too.

Can these four macros changed to inline functions ?

kib: If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it…
mjgAuthorUnsubmitted
Done
Inline Actions

I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a meaningful value so this will have to go back to doing work on its own. In order to keep things consistent I would prefer avoid changing the _REF_ variant either. But I don't have a strong opinion here.

As for inline, this requires adding several headers to struct mount.h and despite them being gated with _KERNEL it causes a lot of breakage during buildworld, e.g.:

/usr/src/lib/libprocstat/common_kvm.h:34: error: conflicting types for 'dev2udev'
/usr/obj/usr/src/sparc64.sparc64/tmp/usr/include/sys/systm.h:499: error: previous declaration of 'dev2udev' was here

I think conversion to inlines should be a separate effort, if done at all.

mjg: I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a…
kibUnsubmitted
Not Done
Inline Actions

Ok.
You might move that to a new header.

kib: Ok. You might move that to a new header.
wakeup((mp)); \ wakeup((mp)); \
} while (0) } while (0)
#endif /* _KERNEL */ #endif /* _KERNEL */
/* /*
* User specifiable flags, stored in mnt_flag. * User specifiable flags, stored in mnt_flag.
*/ */
▲ Show 20 Lines • Show All 650 Lines • ▼ Show 20 Lines
vfs_checkexp_t vfs_stdcheckexp; vfs_checkexp_t vfs_stdcheckexp;
vfs_init_t vfs_stdinit; vfs_init_t vfs_stdinit;
vfs_uninit_t vfs_stduninit; vfs_uninit_t vfs_stduninit;
vfs_extattrctl_t vfs_stdextattrctl; vfs_extattrctl_t vfs_stdextattrctl;
vfs_sysctl_t vfs_stdsysctl; vfs_sysctl_t vfs_stdsysctl;
void syncer_suspend(void); void syncer_suspend(void);
void syncer_resume(void); void syncer_resume(void);
void vfs_op_barrier_wait(struct mount *);
void vfs_op_enter(struct mount *);
void vfs_op_exit_locked(struct mount *);
void vfs_op_exit(struct mount *);
jeffUnsubmitted
Not Done
Inline Actions

Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop to handle the whole business?

If you wanted to do it with multiple cpus you need only set the bit in each counter.

jeff: Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop…
mjgAuthorUnsubmitted
Done
Inline Actions

Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a discussion on whether given per-cpu routine works and/or is a good idea. Whatever ends up protecting the fast path is trivially replaceable.

Main point of *this* review is to (dis)agree on the general approach - that is wrapping all code which requires an exact value with fast path enable/disable routines, with a guarantee everyone executing after the disabling routine returns goes for the current code. Assuming this is fine, I'll post an equivalent for busy/unbusy and write suspension. With this in in place we can discuss how to handle per-cpu, unless you are very opposed to rmlocks showing up in the first place even if temporarily (in which case I can elaborate on what scheme I had in mind).

mjg: Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a…
mjgAuthorUnsubmitted
Done
Inline Actions

Perhaps I should add I do think making the counters atomic in initial commits is an important logical step, especially when it comes to write suspension -- it required a little patching in SU code. It really just needs *something* to keep the fast path. You can see the previous iteration here https://people.freebsd.org/~mjg/patches/atomic-mnt-counters.diff

But again, if the intermediate work is not seen as necessary, I would be more than happy to go straight to per-cpu variants.

mjg: Perhaps I should add I do think making the counters atomic in initial commits is an important…
/*
* We mark ourselves as entering the section and post a sequentially consistent
kibUnsubmitted
Not Done
Inline Actions

Please use the 'sequentially consistent fence' and not 'full barrier' term.

kib: Please use the 'sequentially consistent fence' and not 'full barrier' term.
* fence, meaning the store is completed before we get into the section and
* mnt_vfs_ops is only read afterwards.
*
* Any thread transitioning the ops counter 0->1 does things in the opposite
* order - first bumps the count, posts a sequentially consistent fence and to
* observes all CPUs not executing within the section.
*
* This provides an invariant that by the time the last CPU is observed not
* executing, everyone else entering will see the counter > 0 and exit.
*
* Note there is no barrier between vfs_ops and the rest of the code in the
* section. It is not necessary as the writer has to wait for everyone to drain
* before making any changes or only make safe changes.
*/
#define vfs_op_thread_enter(mp) ({ \
kibUnsubmitted
Not Done
Inline Actions

Can this be inline function ?

kib: Can this be inline function ?
struct mount *_mp = (mp); \
bool _retval = true; \
critical_enter(); \
*(int *)zpcpu_get(_mp->mnt_thread_in_ops_pcpu) = 1; \
atomic_thread_fence_seq_cst(); \
if (__predict_false(_mp->mnt_vfs_ops > 0)) { \
vfs_op_thread_exit(_mp); \
_retval = false; \
} \
_retval; \
})
#define vfs_op_thread_exit(mp) do { \
atomic_thread_fence_rel(); \
*(int *)zpcpu_get(mp->mnt_thread_in_ops_pcpu) = 0; \
critical_exit(); \
} while (0)
#else /* !_KERNEL */ #else /* !_KERNEL */
#include <sys/cdefs.h> #include <sys/cdefs.h>
struct stat; struct stat;
__BEGIN_DECLS __BEGIN_DECLS
Show All 24 Lines
View Options

devel/jetbrains-sqlite/files/patch-platform_sqlite_make.sh

Show First 20 Lines • Show All 220 Lines • ▼ Show 20 Lines#define mnt_endzero mnt_gjprovider
struct mtx mnt_listmtx; struct mtx mnt_listmtx;
struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */ struct vnodelst mnt_activevnodelist; /* (l) list of active vnodes */
int mnt_activevnodelistsize;/* (l) # of active vnodes */ int mnt_activevnodelistsize;/* (l) # of active vnodes */
struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */ struct vnodelst mnt_tmpfreevnodelist; /* (l) list of free vnodes */
int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */ int mnt_tmpfreevnodelistsize;/* (l) # of free vnodes */
struct lock mnt_explock; /* vfs_export walkers lock */ struct lock mnt_explock; /* vfs_export walkers lock */
TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */ TAILQ_ENTRY(mount) mnt_upper_link; /* (m) we in the all uppers */
TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/ TAILQ_HEAD(, mount) mnt_uppers; /* (m) upper mounts over us*/
int mnt_vfs_ops; /* (i) pending vfs ops */
int *mnt_thread_in_ops_pcpu;
}; };
/* /*
* Definitions for MNT_VNODE_FOREACH_ALL. * Definitions for MNT_VNODE_FOREACH_ALL.
*/ */
struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_next_all(struct vnode **mvp, struct mount *mp);
struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp); struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp);
void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp); void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp);
Show All 23 Lines
#define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \ #define MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp) \
__mnt_vnode_markerfree_active(&(mvp), (mp)) __mnt_vnode_markerfree_active(&(mvp), (mp))
#define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx) #define MNT_ILOCK(mp) mtx_lock(&(mp)->mnt_mtx)
#define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx) #define MNT_ITRYLOCK(mp) mtx_trylock(&(mp)->mnt_mtx)
#define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx) #define MNT_IUNLOCK(mp) mtx_unlock(&(mp)->mnt_mtx)
#define MNT_MTX(mp) (&(mp)->mnt_mtx) #define MNT_MTX(mp) (&(mp)->mnt_mtx)
#define MNT_REF_FASTPATH(mp) do { \
kibUnsubmitted
Not Done
Inline Actions

I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions right now to replace it.

kib: I should admit that I dislike the _FASTPATH() suffix on principle, but I have no suggestions…
mjgAuthorUnsubmitted
Done
Inline Actions

_UNLOCKED or (later) _PCPU?

mjg: _UNLOCKED or (later) _PCPU?
kibUnsubmitted
Not Done
Inline Actions

unlocked sounds fine

kib: unlocked sounds fine
atomic_add_int(&(mp)->mnt_ref, 1); \
} while (0)
#define MNT_REL_FASTPATH(mp) do { \
int _c; \
_c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
} while (0)
#define MNT_REF(mp) do { \ #define MNT_REF(mp) do { \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
(mp)->mnt_ref++; \ atomic_add_int(&(mp)->mnt_ref, 1); \
kibUnsubmitted
Not Done
Inline Actions

This should be MNT_REF_FASTPATH() ?

kib: This should be MNT_REF_FASTPATH() ?
mjgAuthorUnsubmitted
Done
Inline Actions

See below.

mjg: See below.
} while (0) } while (0)
#define MNT_REL(mp) do { \ #define MNT_REL(mp) do { \
int _c; \
mtx_assert(MNT_MTX(mp), MA_OWNED); \ mtx_assert(MNT_MTX(mp), MA_OWNED); \
KASSERT((mp)->mnt_ref > 0, ("negative mnt_ref")); \ _c = atomic_fetchadd_int(&(mp)->mnt_ref, -1) - 1; \
(mp)->mnt_ref--; \ KASSERT(_c >= 0, ("negative mnt_ref %d", _c)); \
if ((mp)->mnt_ref == 0) \ if (_c == 0) \
kibUnsubmitted
Not Done
Inline Actions

If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it there too.

Can these four macros changed to inline functions ?

kib: If you change MNT_REL_FASTPATH into ext macro that returns a value, I think you can use it…
mjgAuthorUnsubmitted
Done
Inline Actions

I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a meaningful value so this will have to go back to doing work on its own. In order to keep things consistent I would prefer avoid changing the _REF_ variant either. But I don't have a strong opinion here.

As for inline, this requires adding several headers to struct mount.h and despite them being gated with _KERNEL it causes a lot of breakage during buildworld, e.g.:

/usr/src/lib/libprocstat/common_kvm.h:34: error: conflicting types for 'dev2udev'
/usr/obj/usr/src/sparc64.sparc64/tmp/usr/include/sys/systm.h:499: error: previous declaration of 'dev2udev' was here

I think conversion to inlines should be a separate effort, if done at all.

mjg: I don't think changing this is worthwhile. The per-cpu variant will not have a way to return a…
kibUnsubmitted
Not Done
Inline Actions

Ok.
You might move that to a new header.

kib: Ok. You might move that to a new header.
wakeup((mp)); \ wakeup((mp)); \
} while (0) } while (0)
#endif /* _KERNEL */ #endif /* _KERNEL */
/* /*
* User specifiable flags, stored in mnt_flag. * User specifiable flags, stored in mnt_flag.
*/ */
▲ Show 20 Lines • Show All 650 Lines • ▼ Show 20 Lines
vfs_checkexp_t vfs_stdcheckexp; vfs_checkexp_t vfs_stdcheckexp;
vfs_init_t vfs_stdinit; vfs_init_t vfs_stdinit;
vfs_uninit_t vfs_stduninit; vfs_uninit_t vfs_stduninit;
vfs_extattrctl_t vfs_stdextattrctl; vfs_extattrctl_t vfs_stdextattrctl;
vfs_sysctl_t vfs_stdsysctl; vfs_sysctl_t vfs_stdsysctl;
void syncer_suspend(void); void syncer_suspend(void);
void syncer_resume(void); void syncer_resume(void);
void vfs_op_barrier_wait(struct mount *);
void vfs_op_enter(struct mount *);
void vfs_op_exit_locked(struct mount *);
void vfs_op_exit(struct mount *);
jeffUnsubmitted
Not Done
Inline Actions

Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop to handle the whole business?

If you wanted to do it with multiple cpus you need only set the bit in each counter.

jeff: Instead of rmlock could we not encode the flag bits in the counter field and use a cmpset loop…
mjgAuthorUnsubmitted
Done
Inline Actions

Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a discussion on whether given per-cpu routine works and/or is a good idea. Whatever ends up protecting the fast path is trivially replaceable.

Main point of *this* review is to (dis)agree on the general approach - that is wrapping all code which requires an exact value with fast path enable/disable routines, with a guarantee everyone executing after the disabling routine returns goes for the current code. Assuming this is fine, I'll post an equivalent for busy/unbusy and write suspension. With this in in place we can discuss how to handle per-cpu, unless you are very opposed to rmlocks showing up in the first place even if temporarily (in which case I can elaborate on what scheme I had in mind).

mjg: Note I said I don't want rmlocks later, they are here specifically because I wanted to avoid a…
mjgAuthorUnsubmitted
Done
Inline Actions

Perhaps I should add I do think making the counters atomic in initial commits is an important logical step, especially when it comes to write suspension -- it required a little patching in SU code. It really just needs *something* to keep the fast path. You can see the previous iteration here https://people.freebsd.org/~mjg/patches/atomic-mnt-counters.diff

But again, if the intermediate work is not seen as necessary, I would be more than happy to go straight to per-cpu variants.

mjg: Perhaps I should add I do think making the counters atomic in initial commits is an important…
/*
* We mark ourselves as entering the section and post a sequentially consistent
kibUnsubmitted
Not Done
Inline Actions

Please use the 'sequentially consistent fence' and not 'full barrier' term.

kib: Please use the 'sequentially consistent fence' and not 'full barrier' term.
* fence, meaning the store is completed before we get into the section and
* mnt_vfs_ops is only read afterwards.
*
* Any thread transitioning the ops counter 0->1 does things in the opposite
* order - first bumps the count, posts a sequentially consistent fence and to
* observes all CPUs not executing within the section.
*
* This provides an invariant that by the time the last CPU is observed not
* executing, everyone else entering will see the counter > 0 and exit.
*
* Note there is no barrier between vfs_ops and the rest of the code in the
* section. It is not necessary as the writer has to wait for everyone to drain
* before making any changes or only make safe changes.
*/
#define vfs_op_thread_enter(mp) ({ \
kibUnsubmitted
Not Done
Inline Actions

Can this be inline function ?

kib: Can this be inline function ?
struct mount *_mp = (mp); \
bool _retval = true; \
critical_enter(); \
*(int *)zpcpu_get(_mp->mnt_thread_in_ops_pcpu) = 1; \
atomic_thread_fence_seq_cst(); \
if (__predict_false(_mp->mnt_vfs_ops > 0)) { \
vfs_op_thread_exit(_mp); \
_retval = false; \
} \
_retval; \
})
#define vfs_op_thread_exit(mp) do { \
atomic_thread_fence_rel(); \
*(int *)zpcpu_get(mp->mnt_thread_in_ops_pcpu) = 0; \
critical_exit(); \
} while (0)
#else /* !_KERNEL */ #else /* !_KERNEL */
#include <sys/cdefs.h> #include <sys/cdefs.h>
struct stat; struct stat;
__BEGIN_DECLS __BEGIN_DECLS
Show All 24 Lines