inline atomics and allow tied modules to inline locks

[mmacy]

Summary

prompted by a discussion with @kib

[imp]

So what's a tied module?

sys/sys/lock.h

130 ↗

(On Diff #44697)

what's KLD_TIED?

[mmacy]

sys/sys/lock.h

130 ↗

(On Diff #44697)

It's kib's preferred term for a define to allow in-tree modules to opt in to inlining locking calls.

[kib]

sys/sys/lock.h

130 ↗

(On Diff #44697)

This is a reference to the tied modules, which are required to be compiled against __FreeBSD_version equal to the version of the kernel. See DECLARE_MODULE_TIED() in sys/module.h.

I think that MODULE_VERSION() should assert that required kernel version for module is equal to the kernel version when it detects KLD_TIED. In other words, KLD_TIED should imply DECLARE_MODULE_TIED().

Or better idea, remove DECLARE_MODULE_TIED() and make DECLARE_MODULE() automatically do _TIED when KLD_TIED is defined.

[kib]

BTW, I think it would be useful to allow something like

MODULE_TIED=1

in the module's Makefile, instead of requiring

CFLAGS+=-DKLD_TIED

[mmacy]

In D16079#340862, @kib wrote:

BTW, I think it would be useful to allow something like

MODULE_TIED=1

in the module's Makefile, instead of requiring

CFLAGS+=-DKLD_TIED

What does that entail?

[kib]

In D16079#340964, @mmacy wrote:

In D16079#340862, @kib wrote:

BTW, I think it would be useful to allow something like

MODULE_TIED=1

in the module's Makefile, instead of requiring

CFLAGS+=-DKLD_TIED

What does that entail?

I do not quite understand the question. Are you asking how to implement this, or what the consequences of specifying MODULE_TIED=1 in the module' Makefile should be ?

If the statement is present in the Makefile, then

• KLD_TIED should be defined for C preprocessor, activating the inlining that you are adding in the review
• MODULE_VERSION() in the module code should be magically converted into MODULE_VERSION_TIED()

I.e. this is an easy and consistent way to access the feature you are adding.

[mmacy]

In D16079#340971, @kib wrote:

In D16079#340964, @mmacy wrote:

In D16079#340862, @kib wrote:

BTW, I think it would be useful to allow something like

MODULE_TIED=1

in the module's Makefile, instead of requiring

CFLAGS+=-DKLD_TIED

What does that entail?

I do not quite understand the question. Are you asking how to implement this, or what the consequences of specifying MODULE_TIED=1 in the module' Makefile should be ?

If the statement is present in the Makefile, then

• KLD_TIED should be defined for C preprocessor, activating the inlining that you are adding in the review
• MODULE_VERSION() in the module code should be magically converted into MODULE_VERSION_TIED()

I.e. this is an easy and consistent way to access the feature you are adding.

Ok. But how do I drive the MODULES_TIED=1 => -DKLD_TIED? I can't do it in module.h because there's no guarantee that it's included before the lock headers.

-M

[kib]

In D16079#340979, @mmacy wrote:

Ok. But how do I drive the MODULES_TIED=1 => -DKLD_TIED? I can't do it in module.h because there's no guarantee that it's included before the lock headers.

There is some confusion. You add the handling somewhere in conf/kmod.mk, not in module.h.

diff --git a/sys/conf/kmod.mk b/sys/conf/kmod.mk
index ef07516eb6b..e8aa3ee1520 100644
--- a/sys/conf/kmod.mk
+++ b/sys/conf/kmod.mk
@@ -111,6 +111,9 @@ WERROR?=	-Werror
 CFLAGS+=	${WERROR}
 CFLAGS+=	-D_KERNEL
 CFLAGS+=	-DKLD_MODULE
+.if defined(MODULE_TIED)
+CFLAGS+=	-DKLD_TIED
+.endif
 
 # Don't use any standard or source-relative include directories.
 NOSTDINC=	-nostdinc

And module.h would check for #ifdef KLD_TIED and then record the right version in MODILE_VERSION().

[mmacy]

• add MODULE_TIED
• enforce module tied if KLD_TIED is defined

[imp]

So I'm confused a little.

Seems weird to have the indirection for KLD_TIED in the Makefile, and not in the source. Why bother with the indirection... Why not just state it directly in the source file(s)?

unless this is intended to be a documented build interface that the user can tweak, in which case it should be spelled MK_KLD_TIED={yes,no} and have the appropriate src/conf/kern.opts.mk tweaks done.

[mmacy]

In D16079#341073, @imp wrote:

So I'm confused a little.

Seems weird to have the indirection for KLD_TIED in the Makefile, and not in the source. Why bother with the indirection... Why not just state it directly in the source file(s)?

Putting it in the source would require putting it at the top of every source file to guarantee inlining. I'm not sure why that would make more sense.

unless this is intended to be a documented build interface that the user can tweak, in which case it should be spelled MK_KLD_TIED={yes,no} and have the appropriate src/conf/kern.opts.mk tweaks done.

That generally implies that it's done for the whole tree. I'm only advocating applying it to modules that could really benefit, e.g. rack, fastpath, cxgbe, mlx5. I don't really have any feelings about the Makefile cosmetics. I'll defer to the others on that.

[bdrewery]

sys/sys/lock.h

130 ↗

(On Diff #44697)

This commit is wacky. DECLARE_MODULE_TIED does not define KLD_TIED. What even defines KLD_TIED?
And why this duplication?

#ifdef KLD_TIED
#define DECLARE_MODULE(name, data, sub, order)                          \
        DECLARE_MODULE_WITH_MAXVER(name, data, sub, order, __FreeBSD_version)
#else
#define DECLARE_MODULE(name, data, sub, order)                                                  \
        DECLARE_MODULE_WITH_MAXVER(name, data, sub, order, MODULE_KERNEL_MAXVER)
#endif

/*
 * The module declared with DECLARE_MODULE_TIED can only be loaded
 * into the kernel with exactly the same __FreeBSD_version.
 *
 * Use it for modules that use kernel interfaces that are not stable
 * even on STABLE/X branches.
 */
#define DECLARE_MODULE_TIED(name, data, sub, order)                     \
        DECLARE_MODULE_WITH_MAXVER(name, data, sub, order, __FreeBSD_version)

[bdrewery]

head/sys/conf/kmod.mk

114

This is not documented anywhere that I see, and it does feel very redundant with DECLARE_MODULE_TIED. I don't buy about the "must be defined earlier" argument. It's no reason to seemingly duplicate something. Should this really be MODULE_INLINED_LOCKS?

[kib]

sys/sys/lock.h

130 ↗

(On Diff #44697)

I do not understand your question there. KLD_TIED is defined by the build infrastructure. DECLARE_MODULE_TIED() cannot have an effect in files where it is not present, and it cannot have an effect before its line.

[imp]

head/sys/conf/kmod.mk

114

I think this build option needs to be more than just this quick hack too... In addition to the other concerns raised.

[mmacy]

head/sys/conf/kmod.mk

114

There are no "other concerns" that haven't either been addressed or refuted. Please explain to us how you would like it to be made more than a "quick hack".

[mmacy]

head/sys/conf/kmod.mk

114

@imp please tell me explicitly how to proceed. Superficially your views conflict with John's. I'm happy to fix but you need to provide concrete guidance.

[imp]

head/sys/conf/kmod.mk

114

I'll take care of fixing things after the holiday weekend since it's purely a build mechanic at this point.

[mmacy]

head/sys/conf/kmod.mk

114

Thanks.