Page MenuHomeFreeBSD

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

Authored by ehem_freebsd_m5p.com on Mon, Jan 13, 5:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Feb 8, 10:59 PM
Unknown Object (File)
Sat, Feb 8, 2:13 AM
Unknown Object (File)
Tue, Feb 4, 10:38 PM
Unknown Object (File)
Tue, Feb 4, 1:57 AM
Unknown Object (File)
Fri, Jan 31, 9:11 AM
Unknown Object (File)
Wed, Jan 29, 1:46 PM
Unknown Object (File)
Tue, Jan 28, 9:42 AM
Unknown Object (File)
Sat, Jan 25, 4:55 AM

Details

Summary

There were two issues with the existing DEFINE_CLASS_#() macros. First,
the parent count was capped at 3 since every count needed a distinct
macro. Second, many uses wanted the class variable to be overtly
static (private).

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
a single static version of the macro. Also start using a designated
initializer for the main structure.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 61703
Build 58587: arc lint + arc unit

Event Timeline

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.

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

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

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...

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 requested changes to this revision.Wed, Feb 5, 11:15 PM
jrtc27 added a subscriber: 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.

This revision now requires changes to proceed.Wed, Feb 5, 11:15 PM

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

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?

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.

@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.