Page MenuHomeFreeBSD

Kernel build changes for openzfs vendor import
AbandonedPublic

Authored by imp on Jun 26 2020, 9:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Mar 9, 8:11 AM
Unknown Object (File)
Feb 16 2024, 12:27 AM
Unknown Object (File)
Feb 16 2024, 12:11 AM
Unknown Object (File)
Jan 18 2024, 9:50 PM
Unknown Object (File)
Dec 20 2023, 7:42 AM
Unknown Object (File)
Oct 3 2023, 12:32 PM
Unknown Object (File)
Sep 12 2023, 10:45 AM
Unknown Object (File)
Aug 23 2023, 7:20 AM

Details

Summary

This is a big enough changes that I will do the libraries and tools as a separate review.

I've only removed files that prevented cpp from finding base system header.

Relies on https://github.com/openzfs/zfs/pull/10497

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

mmacy requested review of this revision.Jun 26 2020, 9:10 PM
delphij requested changes to this revision.Jun 26 2020, 9:35 PM

Minor typo fixes.

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
3198

This doesn't seem right...

3198
3511

This should be current

3511
6128

Ditto.

6128
7063

Ditto.

7063
7336

Ditto

This revision now requires changes to proceed.Jun 26 2020, 9:35 PM
sys/conf/kern.pre.mk
209–217

I'm a big fan of formatting long lists one-per-line - just as you have done in some of the other makefiles - blobs like this are very hard to read let alone review chanages to

incorporate kern.pre.mk and dtrace.c comment feedback

sys/conf/kern.pre.mk
209–217

FWIW I literally meant one per line as in
`
CDDL_FLAGS= \

-DFREEBSD_NAMECACHE \
-D_SYS_VMEM_H_  \
-D__KERNEL \
-D__KERNEL__

`

etc (with single TAB) indent, this obviously adds a lot of lines, but IMO aids readability and maintainability

Others might disagree - not sure.

Lots of repetition in the module makefiles should be cleaned up. Normally I wouldn't fuss too much, but in this case it's very pervasive.

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
7524

Can the current-> curstate stuff be done as one commit? That would make this review much shorter

sys/conf/files
642

This rename could also be separate

sys/modules/dtrace/dtnfscl/Makefile
23

This looks oft repeated. Maybe add a macro or two?

sys/conf/kern.pre.mk
209–217

I'll second this

mmacy added inline comments.
sys/conf/kern.pre.mk
209–217

FWIW I literally meant one per line as in
`
CDDL_FLAGS= \

-DFREEBSD_NAMECACHE \
-D_SYS_VMEM_H_  \
-D__KERNEL \
-D__KERNEL__

`

etc (with single TAB) indent, this obviously adds a lot of lines, but IMO aids readability and maintainability

Others might disagree - not sure.

That's going to seriously bloat the file. I'll have to get a quorum on that first because I'd be completely surprised if someone didn't come along and violently insist I revert it. This being FreeBSD and all.

sys/modules/dtrace/dtnfscl/Makefile
23

This looks oft repeated. Maybe add a macro or two?

Where? kmod.mk?

make s/current/curstate/g separate

sys/modules/dtrace/dtnfscl/Makefile
23

Yes please. Thanks!

  • rebase
  • add macro for dtrace makefiles

-kern.pre.mk formatting feedback

  • fix a dtrace makefile

update opensolaris module Makefile

LGTM overall; do we support AVX in kernel code now, or are we just building these but giving kfpu_allowed a false value for now?

Thanks for the earlier cleanups. Mostly minor stuff, and much of it is questions or leading questions...

sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
48

you delete the illumos code in some places but not others. why?

sys/cddl/dev/dtrace/amd64/dtrace_subr.c
39

Wondering if the dtrace stuff could be done now. is it compatible with what's there today? It's kinda small, though, so might not be worth the hassle if the rest of this is almost ready to roll into the tree.

sys/conf/kern.pre.mk
209–217

I think there's enough of a consensus you should do this. It's my sense from years of doing it that 70-80% of the people like it, and maybe 10% don't. That's as a consensus (which I've informally been using 2/3 or better, btw, as the yardstick).

sys/modules/dtrace/dtrace/Makefile
72

Why do we need these extra defines?

sys/modules/dtrace/fbt/Makefile
11

I'd delete this line... it's a good sign, btw, that I'm noticing only things this small :)

sys/modules/dtrace/prototype/Makefile
22

Here, and elsewhere: this looks a lot better. Thanks.

sys/modules/zfs/Makefile
28

All these _H files deserve some kind of comment, I think, since this is quite unusual to do / see in modules and can cause trouble if we were ever to switch-up how we do includes.

299

I'm curious, what's the construct that causes us to need this on so many files?

sys/modules/zfs/zfs_config.h
7

So you have a bunch of HAVE_ on the command line. Why aren't there here instead?

sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
48

you delete the illumos code in some places but not others. why?

See line 2 of the summary

sys/conf/kern.pre.mk
209–217

I think there's enough of a consensus you should do this. It's my sense from years of doing it that 70-80% of the people like it, and maybe 10% don't. That's as a consensus (which I've informally been using 2/3 or better, btw, as the yardstick).

\o/

sys/modules/dtrace/dtrace/Makefile
72

Why do we need these extra defines?

It was sucking in those headers I’ll see if other changes have fixed it in the meantime

sys/modules/zfs/Makefile
299

I'm curious, what's the construct that causes us to need this on so many files?

Linux const obliviousness IIRC

sys/modules/zfs/zfs_config.h
7

So you have a bunch of HAVE_ on the command line. Why aren't there here instead?

I could, but this header is generated by autoconf. If we do that I’d trim this down

There is always room for improvement, but this seems generally ok.
FWIW the "how to write makefiles" doc I provide to devs at Juniper includes
"""

  1. Do not put anything in your makefile that you don't need
  2. Do not put anything in your makefile that you cannot explain the need for. Ie. if you cannot explain it, you don't need it, remove it.
  3. Do not cut/paste anything from your friend's makefile (see #1).

Note: #2 does not mean that you should remove everything from an
existing makefile that you don't understand the 1st time you look at
it.
"""

that and guidance on formatting lists as previously described, fwiw I recommend switching to the one-per-line form when a list no longer fits in one line without wrapping.

These seem like little things, but as with C code, consistent and clean style helps.

sys/modules/zfs/Makefile
18

Multiple += like this is a lot of malloc churn for make.
Much more efficient to a single multi-line value.

This revision is now accepted and ready to land.Jun 29 2020, 5:20 PM
This revision now requires review to proceed.Jun 29 2020, 5:20 PM
This revision is now accepted and ready to land.Jun 29 2020, 5:20 PM
This revision now requires review to proceed.Jun 29 2020, 5:20 PM

I'll echo Simon here: any change of this size could always be a bit better, but given the level of nits I'm finding most of them can be addressed by being a little more explicit about the points I've questioned in the commit message.

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
132–139

extra newline here.

sys/cddl/contrib/opensolaris/uts/common/dtrace/fasttrap.c
48

You'll want to be more explicit about this point in the commit message, because even after you pointing me at it I had to think about it and look to understand it.

sys/cddl/dev/fbt/fbt.c
37

How did this and the other files you added sys/endian.h to compile before?

sys/modules/dtrace/dtrace/Makefile
72

Fair enough. We may want to make it possible to include those two files in assembler contexts, but it's a minor point.

sys/modules/zfs/zfs_config.h
7

Yea, it seems weird to mix... But I understand now.

imp commandeered this revision from mmacy.
imp edited reviewers, added: mmacy; removed: imp.

this revision never landed, but all the parts did and its OBE, so I've stolen this and am abandoning it.