Page MenuHomeFreeBSD

Kernel build changes for openzfs vendor import
Needs ReviewPublic

Authored by mmacy on Fri, Jun 26, 9:10 PM.

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

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 32001

Event Timeline

mmacy created this revision.Fri, Jun 26, 9:10 PM
mmacy requested review of this revision.Fri, Jun 26, 9:10 PM
mmacy edited the summary of this revision. (Show Details)Fri, Jun 26, 9:17 PM
delphij requested changes to this revision.Fri, Jun 26, 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.Fri, Jun 26, 9:35 PM
sjg added inline comments.Fri, Jun 26, 9:43 PM
sys/conf/kern.pre.mk
209–243

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

mmacy updated this revision to Diff 73725.Fri, Jun 26, 10:44 PM

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

sjg added inline comments.Fri, Jun 26, 11:02 PM
sys/conf/kern.pre.mk
209–243

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.

imp added a comment.Fri, Jun 26, 11:05 PM

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
12

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

freqlabs added inline comments.Fri, Jun 26, 11:07 PM
sys/conf/kern.pre.mk
209–243

I'll second this

mmacy marked 4 inline comments as done.Fri, Jun 26, 11:12 PM
mmacy added inline comments.
sys/conf/kern.pre.mk
209–243

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.

mmacy added inline comments.Fri, Jun 26, 11:23 PM
sys/modules/dtrace/dtnfscl/Makefile
12

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

Where? kmod.mk?

mmacy updated this revision to Diff 73727.Fri, Jun 26, 11:42 PM

make s/current/curstate/g separate

imp added inline comments.Fri, Jun 26, 11:54 PM
sys/modules/dtrace/dtnfscl/Makefile
12

Yes please. Thanks!

mmacy updated this revision to Diff 73729.Sat, Jun 27, 1:20 AM
  • rebase
  • add macro for dtrace makefiles
mmacy updated this revision to Diff 73731.Sat, Jun 27, 1:39 AM

-kern.pre.mk formatting feedback

  • fix a dtrace makefile
mmacy updated this revision to Diff 73732.Sat, Jun 27, 1:43 AM

update opensolaris module Makefile

delphij accepted this revision.Sat, Jun 27, 6:49 AM

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?

imp added a comment.Sat, Jun 27, 2:12 PM

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–243

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
61

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
11

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
6

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

mmacy added inline comments.Sat, Jun 27, 3:56 PM
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

mmacy added inline comments.Sat, Jun 27, 3:58 PM
sys/conf/kern.pre.mk
209–243

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
61

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

mmacy added inline comments.Sat, Jun 27, 4:03 PM
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
6

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

sjg accepted this revision.Sat, Jun 27, 6:08 PM

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.

mmacy removed a reviewer: gnn.Mon, Jun 29, 5:20 PM
This revision is now accepted and ready to land.Mon, Jun 29, 5:20 PM
This revision now requires review to proceed.Mon, Jun 29, 5:20 PM
mmacy removed a reviewer: gnn.Mon, Jun 29, 5:20 PM
This revision is now accepted and ready to land.Mon, Jun 29, 5:20 PM
This revision now requires review to proceed.Mon, Jun 29, 5:20 PM
imp accepted this revision.Mon, Jun 29, 11: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

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
61

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
6

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