kobj: add DEFINE_CLASSN() for arbitrary # parent, add public/private

[ehem_freebsd_m5p.com]

Summary

When originally implemented at 46ba7a35f2ea, the default for variables was static. As such DECLARE_CLASS() plus DEFINE_CLASS_#() were sufficient, since unless a DECLARE_CLASS() was encountered first the kobj would be static. Since that time the standards have changed and variables now default to extern.

A result of this is a proliferation of static DEFINE_CLASS and many instances of classes being declared without using the macros. This was further exacerbated by this not working for classes which had at least one parent. In light of this add PUBLIC and PRIVATE macro variants which allow overt declaration of static or extern.

By using VA_ARGS and VA_OPT there is no longer a need for a distinct macro for every parent count. This also allows for there to be single versions of public/private, instead of four copies of each.

Switch to deriving the base class list name from the class variable. The name is not required to be a valid C identifier, whereas the class variable must be so. There is no mention of a reason for this when it was implemented at 46ba7a35f2ea. This allows removing the workarounds added at 617994efc718 and 550d01a2117f.

[ehem_freebsd_m5p.com]

Natually that last minute change breaks things.

Clang accepts VA_OPT() with the options used for building FreeBSD. I'm rather unsure as to the stance on whether it is acceptable to use due to being a fairly recent feature.

I've noticed many uses of static DEFINE_CLASS_0(...);. Issue is this doesn't work with anything besides zero parents and I would expect non-zero parents to be fairly common. As such I'm suggsting an alternative macro set which handles the static internally.

I'm unsure of appropriate reviewers. This is related to D48408, though a distinct issue with the kernel object system.

[ehem_freebsd_m5p.com]

I'm unsure of who I should ask to review this, seems close enough to the core you two may be good candidates.

[ehem_freebsd_m5p.com]

Ugh, identify the simple goof made on the original version. Going back with that corrected.

[ehem_freebsd_m5p.com]

Looking at this some more, the real value is creating PUBLIC_ and PRIVATE_ versions of the DEFINE_CLASS macros. Handling arbitrary numbers of parents mostly serves to simplify things to only needing two macros. It is notable with the current macros it is impossible to create a private/static class with at least one parent.

It might be worthwhile for PUBLIC_DEFINE_CLASSN to create extern kobj_class_t name ## _baseclasses_singleparent; since it is very common to have a single parent. Having two or more is quite uncommon.

Likely DEFINE_CLASSN should be dropped. I was thinking compatibility, but that seems dubious value. Likely drop the N suffix as the public/private prefixes are sufficient to distinguish these from the existing macros. Do kind of need reviewer attention though...

[ehem_freebsd_m5p.com]

Switch to using _classvar as the prefix for the base class list. _name has been a *string*, while _classvar was already required to be a valid C variable name. Apparently doesn't effect too many places, but a lurking issue.

Note, I could also see DEFINE_{PUBLIC,PRIVATE}_CLASS as the macro name. The initial revision is {PUBLIC,PRIVATE}_DEFINE_CLASS, but the other word order might be better (or worse).

[jrtc27]

This doesn't work for N >= 3. Even if you have indirect recursion, macro foo will not further expand a foo within it, so you will find that DEFINE_CLASS_3 leaves behind a ___DEFINE_CLASSN_BASE_EXPANDER for the final class (or more than one class for N > 3). As it happens DEFINE_CLASS_3 is unused in the tree which is why you didn't notice. But that's all the more reason not to generalise if we don't even need N > 2.

[ehem_freebsd_m5p.com]

Give up on fooling the preprocessor, simply terminate near existing limit.

[ehem_freebsd_m5p.com]

Yeah, further testing confirms I didn't fool the preprocessor. Still, this has 3 rather valuable improvements over the existing macros:

1. Addition of PUBLIC/PRIVATE variants. There are many instances of static DEFINE_CLASS which seem ugly. This is due to using the newer standards where global/extern is the default. Problem is this only works with DEFINE_CLASS and DEFINE_CLASS_0. This is extremely problematic since base (therefore global) classes are likely to have fewer parents whereas leaf classes are likely to have more parents.
2. Removal of the _# suffix. Not a huge deal until you're adding something as a parent of 30+ existing classes.
3. Fix the naming of the base class list. Of the arguments to DEFINE_CLASS_*, name is a C-string, while classvar is inherently a C-identifier. This has likely been causing minor trouble for some time, just no one had realized the error.

Hopefully this adjustment can be approved?

[ehem_freebsd_m5p.com]

Correct the count of ampersand adds, I was close previously, but now do exact match. Upon examination seems the issue I noticed had been worked around at least twice. Curious it wasn't addressed then.

[ehem_freebsd_m5p.com]

@kevans is another person who might provide valuable insight into this (due to D39824 and D48079). The real issue is the declarations of kernel objects are very inconsistent. It is split between the uses of the existing macros (DEFINE_CLASS() and DEFINE_CLASS_[0-3]()) and inline uses of struct kobj_class. Feels roughly equal, so somewhere between 25:75 and 50:50.

One factor I note is there are a lot of static DEFINE_CLASS(); uses. This seems an ugly way to make the struct kobj_class static/private. Meanwhile the DEFINE_CLASS_[1-3]() macros are always global and cannot be used to generate static/private kernel objects. Rather problematically a number of drivers are likely to end up changing from zero parents to one parent in the near future.

I guess there is an underlying issue, which direction should kernel object handling in the FreeBSD kernel head? Increase uses of macros and try to remove inline structures? Deprecate the macros and change to overt structures? I was thinking the direction was towards macro use, but if I'm wrong about that knowing would be valuable.

[ehem_freebsd_m5p.com]

I guess I wonder what I'm supposed to do now. Does the lack of updated reviews mean the current version is implicitly accepted? (the requested issue was addressed)

I think I've finally figured out what was occurring. At rG:46ba7a35f2ea, FreeBSD was still using compilers/settings where for variables the default behavior was static if it wasn't explicitly declared static or extern. As a result at that time if DEFINE_CLASS_#() was used without a DECLARE_CLASS() in a header, the kobj was private/static. Some time after that the standards changed.

The pattern static DEFINE_CLASS_0() works for no parents, but nothing can be done for kobjs which have one or more parents. As such with that standards change, the macros became far less attractive.

The benefit of single macros handling arbitrary numbers of parents is it means rather than needing 8 macros (2 for each parent count), it can simply be kept at one for private and one for public.

I do not know whether this is merely a reason for the very inconsistent declaration of kobjs, a major reason or the reason. With no activity, it seemed @peterj might be interested?

[ehem_freebsd_m5p.com]

Updating to current repository state. Mark older macros as deprecated. Trivial reduction in diff size. May need to update D48449 to match the commit message as that no longer resembles what it did when when created.

[ehem_freebsd_m5p.com]

Now looking for this as a commit series will be adding a single parent kernel object to several objects; without something like this several of those will become global (though undeclared).

The one potential alternate I see is to perhaps have the list of classes be a parenthesized list instead of varargs. Mainly instead have PRIVATE_DEFINE_CLASSN(class0, class0_driver, class0_methods, 0, ); for zero parents, PRIVATE_DEFINE_CLASSN(class1, class1_driver, class1_methods, 0, (base0)) for one parent and PRIVATE_DEFINE_CLASSN(class2, class2_driver, class2_methods, 0, (base0, base1));.

Also wondering about the name choice.

[ehem_freebsd_m5p.com]

An alternative. Considering whether the _Static_assert() will hopefully generate a better error message. Near-certain to be a giant explosion no matter what if the circumstance occurs.