Details

Reviewers

kib
novel
shurd
se

Group Reviewers

bhyve
iflib

Commits

rGe2650af157bc: Make CPU_SET macros compliant with other implementations

Summary

There are issues with programs that expect GLIBC style CPU_* functions after detecting some of the names defined in sys/cpuset.h.

In order to allow those programs (ports) to be correctly built, we have to provide compatible functionality, without loosing the functionality we use and need in the FreeBSD kernel and user-land (in some commands any in many libraries).

Test Plan

Apply patches and build and run world and kernel.
Test build and functionality of 2 ports: devel/libvirt (with the patch file patch-src_util_virprocess.c removed, since it is obsoleted by this patch set) and lang/gcc11.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

se created this revision.Dec 14 2021, 10:31 PM

Herald added a subscriber: imp. · View Herald TranscriptDec 14 2021, 10:31 PM

se requested review of this revision.Dec 14 2021, 10:31 PM

Update patch set to include full context.

kib added inline comments.Dec 14 2021, 11:45 PM

sys/sys/bitset.h
318	You do not need to define usermode-allocating __BITSET_ALLOC. Only BITSET_ALLOC.
sys/sys/cpuset.h
78	This is WIP, at best, right?
79	I suspect this is a namespace pollution, POSIX does not allow inclusion of cpuset.h expose stdlib symbols.
81	And this is the reason for stdlib.h inclusion, right? This is why a lot of stuff are macros and not inlines.

se added inline comments.Dec 15 2021, 9:32 AM

sys/sys/bitset.h
318	I had added these defines in this way for symmetry reasons, not due to actual uses in code I know of. In fact, I do not know of any single use of BITSEC_ALLOC() in usermode code, and I guess it should not be defined outside _KERNEL at all.
sys/sys/cpuset.h
78	Yes, as I wrote in the comment to this review, this is a WIP state, just what I had after test building lang/gcc11 with the static inline implementation of CPU_ALLOC().
79	No, this was just a quick hack to make lang/gcc11 build with static inline CPU_ALLOC(), obviously not acceptable for a final commit.
81	The problem is that GCC poisons references to malloc(), even if it is only used to define some macro like CPU_ALLOC() which is never used in GCC. This static inline implementation is acceptable in GCC, but leads to the obvious problems due to stdlib.h being required and not present when cpuset.h is included in GCC. This is just one example of a port that uses GLIBC specific CPU_* functions. Your changes to cpuset.h where meant to improve compatibility with such programs, but due to the magic of autoconfigure there are now ports detect some GLIBC compatible functions and want to use them all without further testing for their existence. I'm wondering whether we need to provide both FreeBSD and GLIBC style implementations of those functions, the latter e.g. selected by _GNU_SOURCE being defined. The changes that I committed to sys/bitset.h et.al. where a result of my question, whether we should try to provide GLIBC compatible BIT_/BITSET_ macros, and you were correct to point out that __BIT* names could be used to prevent namespace pollution and to allow GLIBC BIT* macros in usermode code. I'm afraid that this will not work well for the CPU_* macros, and one solution might be a similar approach to that used in sys/bitset.h, i.e. CPU_* defined as __CPU_* and aliased back for the kernel and FreeBSD code in base, and GLIBC compatible macros for ports that define _GNU_SOURCE. You do probably know that GLIBC uses CPU_AND() for FreeBSD's CPU_AND2(), for example, but GNU programs have to #define _GNU_SOURCE to make these GLIBC CPU_* macros visible.

No, I do not think it is a good idea to switch to glibc-compatible CPU* macros. As I noted elsewhere, there is a line where we should stop in base at trying to placate third-party sw that assumes that sched.h or sched_getaffinity existence implies glibc, and just leave the local patch to ports and upstream.

BITSET->__BITSET is a good change on its own, and this is the primary sanity check that we should apply to anything we propose for the base. Absence of conflicts with non-standard GNU libc extensions should be secondary criteria.

In D33451#757414, @kib wrote:

No, I do not think it is a good idea to switch to glibc-compatible CPU* macros. As I noted elsewhere, there is a line where we should stop in base at trying to placate third-party sw that assumes that sched.h or sched_getaffinity existence implies glibc, and just leave the local patch to ports and upstream.

I do agree, but many ports detect fractional support of these GLIBC features and do now assume that full support was available, preferring those GLIBC functions over the FreeBSD native ones.

BITSET->__BITSET is a good change on its own, and this is the primary sanity check that we should apply to anything we propose for the base. Absence of conflicts with non-standard GNU libc extensions should be secondary criteria.

I agree, in principle. But AFAIU the situation, there are now ports that see <sched.h> and some functions defined there-in and have started to assume that the GLIBC functionality is actually fully provided.
I do not know what ports benefit from that version of sched.h, but there are a number that do fail with the current version.

I had been hoping that it would be possible to remove -D_WITH_CPU_SET_T from ports, but I guess that I'll have to revert the change that removed the

#ifdef _WITH_CPU_SET_T

from sched.h.

The differences between the FreeBSD CPU_* macros and those in GLIBC are small (e.g. CPU_AND vs. CPU_AND2) and I'm not convinced that it is beneficial to be different in this way.

I could imagine to provide both the FreeBSD and the GLIBC style macros in sys/cpuset.h, depending on a #define _WANT_FREEBSD_CPUSET, for example (or _KERNEL, same as sys/bitset.h).

But I'm not going to work in this direction, if it is not considered the correct way forward.

In D33451#757426, @se wrote:
In D33451#757414, @kib wrote:

No, I do not think it is a good idea to switch to glibc-compatible CPU* macros. As I noted elsewhere, there is a line where we should stop in base at trying to placate third-party sw that assumes that sched.h or sched_getaffinity existence implies glibc, and just leave the local patch to ports and upstream.

I do agree, but many ports detect fractional support of these GLIBC features and do now assume that full support was available, preferring those GLIBC functions over the FreeBSD native ones.

BITSET->__BITSET is a good change on its own, and this is the primary sanity check that we should apply to anything we propose for the base. Absence of conflicts with non-standard GNU libc extensions should be secondary criteria.

I agree, in principle. But AFAIU the situation, there are now ports that see <sched.h> and some functions defined there-in and have started to assume that the GLIBC functionality is actually fully provided.
I do not know what ports benefit from that version of sched.h, but there are a number that do fail with the current version.

I had been hoping that it would be possible to remove -D_WITH_CPU_SET_T from ports, but I guess that I'll have to revert the change that removed the
#ifdef _WITH_CPU_SET_T
from sched.h.

Yes, I added it after I looked and screamed at the amount of brokeness.

The differences between the FreeBSD CPU_* macros and those in GLIBC are small (e.g. CPU_AND vs. CPU_AND2) and I'm not convinced that it is beneficial to be different in this way.

I could imagine to provide both the FreeBSD and the GLIBC style macros in sys/cpuset.h, depending on a #define _WANT_FREEBSD_CPUSET, for example (or _KERNEL, same as sys/bitset.h).

But I'm not going to work in this direction, if it is not considered the correct way forward.

No, I am not trying to discourage anybody to work in this direction, just explained by opinion. Nonetheless, I do think that having two identically named macros with different API or semantic is not going to work, regardless of cludges like WANT_FREEBSD_XXX.

It is just my opinion that it is better to not supply anything than supply contradictory symbols. This is why I like BITSET->BITSET change, and think that CPU->CPU for incompatible variants would be good.

But again, I do not believe that it is possible to solve all issues in src/, at some point ports would need to provide temporary patch, and then upstream proper FreeBSD support. Or we should declare that we would never get something like membarrier/rseq, which I declaim to accept.

New approach to allow the CPU_SET in FreeBSD to be visible in user-land programs:

FreeBSD has definitions of the CPU_* macros that are mostly compatible with those used in 3rd party programs developed targeting GLIBC based userlands.
The major difference is that GLIBC provides 3-address versions of boolean operations, e.g. CPU_AND(dest, src1, src2), while FreeBSD has a macro of the name using a 2-address signature: CPU_AND(dest, src).

By changing the FreeBSD signature of these functions to that used by GLIBC, and with aliasing of operands allowed (specifically dest may be the same variable as one of the src operands), compatibility with GLIBC is achieved.
This comes at the cost of changing an established call signature in all FreeBSD kernel and userland code, though - which we urgently try to avoid in STABLE branches of the source tree.

This patch-set fixes all definitions and uses of the affected macros in FreeBSD. There might be externally developed drivers, that use these functions, though.
Their compilation will fail due to the changed number of macro parameters, and they can check the FreeBSD version to use the appropriate version.

One example of a port that builds on a system with these patches applied is devel/libvirt (with the recently added patch file patch-src_util_virprocess.c either present or removed).

Another issue with this patch set is that it defines CPU_ALLOC() as a macro based on malloc(). This breaks the build of lang/gcc*, which "poisons" the malloc() function (and prevents references to it in macro definitions, even if those macros are never used).
A solution might be to implement CPU_ALLOC() as a static inline function (it seems to be a function in GLIBC, too).

This patch set is meant as a basis for discussion: is future compatibility with user land programs written with GLIBC in mind of sufficient benefit to let us bear the cost of changing the established signature of CPU_AND, CPU_OR, and CPU_XOR?

I do not seek approval to commit this patch set as is, but hope for a discussion that I hope will lead to a patch set that will be committed.

Herald added a reviewer: shurd. · View Herald TranscriptDec 26 2021, 1:43 PM

Herald added a reviewer: bhyve. · View Herald Transcript

Herald added a reviewer: iflib. · View Herald Transcript

Herald added subscribers: glebius, melifaro, bcran and 3 others. · View Herald Transcript

se marked 3 inline comments as done.Dec 26 2021, 1:56 PM

se added inline comments.

contrib/ofed/libmlx5/mlx5.c
366	This is an example of a change applied to contributed code that needs to support both forms of CPU_OR() - at least if the change is to be applied to the up-stream sources.
sys/sys/bitset.h
318	CPU_ALLOC in sys/cpuset.h is currently based on BITSEC_ALLOC and is a user-land only function. The non-kernel variant of BITSET_ALLOC could be dropped if CPU_ALLOC is implemented in a different way (e.g. as a static inline function to allow lang/gcc* to be built).

Summary is, this patch makes CPU_XXX API identical to the one provided to glibc, am I right?

I think this is fine, I do not see why not. Adoption for the base requires quite modest amount of changes, and I hope there is no non-trivial issues from this move.

The only detail that I do not like in the overall picture is that you claim that we cannot merge this to stable. The formal argument is probably close to right, but on the other hand, is it useful to keep that API as is on the branch. As you noted, it means that we have two APIs around for much longer, and have to do relatively complex decisions in out of tree code to detect one API or another. Wouldn't it be simpler and better for third-party programs if we switch to the Linux-compatible API on stable?

I doubt that there are many consumers of FreeBSD API for CPU_ out of base, if any.

Change the definition of CPU_ALLOC() and CPU_FREE() to depend on malloc() respectively free().
This works around the issue that GCC does not allow macros to be defined using malloc().
It seems that ports using CPU_ALLOC() do already include <malloc.h> or <stdlib.h> to get the declarations of malloc() and free() from <malloc_np.h>.

With sys/cpuset.h and sys/bitset.h from this patch set both lang/gcc11 and devel/libvirt can be built (the latter with the patch previously required on -CURRENT removed).

A "make buildworld buildkernel" is currently running, but it succeeded with the previous version of this patch-set (and the only difference is in malloc vs. malloc and free vs. free, which should not have any impact on the build).

se added inline comments.Dec 26 2021, 11:23 PM

sys/sys/bitset.h
353	The definition of BITSET_FREE in the latest patch set did not contain the allocation type parameter "mt". This will be fixed in the final version.

This version is functionally equivalent to the previous one, but introduces a new private function __cpuset_alloc() to allocate a cpuset_t variable of a given size.
Naming of the function and placement in the symbol table are TBD.

This patch has been tested with buildworld/kernel on amd64 and with 2 ports that have special requirements regarding the functionality provided by sched.h.

cpuset_alloc.c seems to be missing

sys/sys/cpuset.h
81	This must be __cpuset_free(), to not depend on the user symbol. Also for symmetry if somebody needs to override.

Add __cpuset_free() and update cpuset.9 man-page to reflect argument changes.

The FreeBSD version has been bumped and an UPDATING entry has been added.

The diff still misses the new files.

Provide new files libc/gen/cpuset_{alloc,free}.c that were missing in the previous patch set.

kib added inline comments.Dec 28 2021, 3:16 PM

lib/libc/gen/cpuset_alloc.c
28	Why do you need this header?

se added inline comments.Dec 28 2021, 4:48 PM

lib/libc/gen/cpuset_alloc.c
28	This is a relic from a prior version that used __malloc(). It has been removed from my current sources in both cpuset_alloc() and cpuset_free().

gerald added a subscriber: gerald.Dec 29 2021, 5:43 PM

kib added inline comments.Dec 29 2021, 10:41 PM

lib/libc/gen/cpuset_alloc.c
28	So are you going to update the patch?

Remove superfluous include files from cpuset_alloc.c and cpuset_free.c.

The date in the UPDATING entry has been set to 20211230 but may need to be adjusted, if the commit is delayed.

kib added inline comments.Dec 29 2021, 11:08 PM

sys/sys/_cpuset.h
52	extern is not needed I miss the declaration of __cpuset_free() These declarations should be wrapped with !_KERNEL and with BEGIN_DECLS/END_DECLS.

Provide definition of __cpuset_free() in sys/_cpuset.h.

Add missing !_KERNEL condition andBEGIN_DECLS and END_DECLS wrapper to sys/_cpuset.h, and add back lost #include <sched.h> to cpuset_free.c.

A "make buildworld buildkernel" is underway, but should succeed since the changes were cosmetic with regard to the functionality used in the base system.

kib accepted this revision.Dec 29 2021, 11:36 PM

jhb commandeered this revision.Feb 3 2022, 6:45 PM

jhb abandoned this revision.

jhb added a commit: rGe2650af157bc: Make CPU_SET macros compliant with other implementations.

jhb added a reviewer: se.

WIP Changes to sys/cpuset.h
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 100733

UPDATING

contrib/ofed/libmlx5/mlx5.c

lib/libc/gen/Makefile.inc

lib/libc/gen/Symbol.map

lib/libc/gen/cpuset_alloc.c

lib/libc/gen/cpuset_free.c

lib/libc/gen/sched_getaffinity.c

lib/libc/gen/sched_getcpu_gen.c

lib/libc/gen/sched_setaffinity.c

lib/libc/x86/sys/sched_getcpu_x86.c

share/man/man9/cpuset.9

sys/amd64/amd64/pmap.c

sys/amd64/vmm/vmm.c

sys/dev/acpica/acpi.c

sys/i386/i386/pmap.c

sys/i386/i386/vm_machdep.c

sys/kern/kern_cpuset.c

sys/kern/kern_rmlock.c

sys/kern/sched_4bsd.c

sys/kern/subr_kdb.c

sys/kern/subr_smp.c

sys/net/iflib.c

sys/powerpc/ofw/ofw_pcibus.c

sys/sys/_cpuset.h

sys/sys/bitset.h

sys/sys/cpuset.h

sys/sys/param.h

sys/x86/x86/cpu_machdep.c

WIP Changes to sys/cpuset.hAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 100733

UPDATING

contrib/ofed/libmlx5/mlx5.c

lib/libc/gen/Makefile.inc

lib/libc/gen/Symbol.map

lib/libc/gen/cpuset_alloc.c

lib/libc/gen/cpuset_free.c

lib/libc/gen/sched_getaffinity.c

lib/libc/gen/sched_getcpu_gen.c

lib/libc/gen/sched_setaffinity.c

lib/libc/x86/sys/sched_getcpu_x86.c

share/man/man9/cpuset.9

sys/amd64/amd64/pmap.c

sys/amd64/vmm/vmm.c

sys/dev/acpica/acpi.c

sys/i386/i386/pmap.c

sys/i386/i386/vm_machdep.c

sys/kern/kern_cpuset.c

sys/kern/kern_rmlock.c

sys/kern/sched_4bsd.c

sys/kern/subr_kdb.c

sys/kern/subr_smp.c

sys/net/iflib.c

sys/powerpc/ofw/ofw_pcibus.c

sys/sys/_cpuset.h

sys/sys/bitset.h

sys/sys/cpuset.h

sys/sys/param.h

sys/x86/x86/cpu_machdep.c

WIP Changes to sys/cpuset.h
AbandonedPublic
Actions

Revision Contents
Changeset List