Page MenuHomeFreeBSD

This allows us to have dtrace in kernel (i.e. not as a modules). Required for aarch64
ClosedPublic

Authored by br on Mar 2 2015, 12:14 PM.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

br retitled this revision from to This allow us to have dtrace in kernel (i.e. not as a modules).
br updated this object.
br edited the test plan for this revision. (Show Details)
br added reviewers: ARM, imp.
andrew requested changes to this revision.Mar 2 2015, 12:23 PM
andrew added a reviewer: andrew.
andrew added a subscriber: andrew.

Can you upload the diff using either of the following commands to get the context (or use arc).

git diff -U999999 other-branch
svn diff --diff-cmd=diff -x -U999999

There needs to be a warning on CDDL code being compiled into the kernel.

You should move the list of files to build from sys/conf/files.arm to sys/conf/files as its doesn't appear to be arm specific.

This revision now requires changes to proceed.Mar 2 2015, 12:23 PM
br edited edge metadata.
br edited edge metadata.

This seems to be half dealing with dtrace on arm, and half allowing dtrace to be in kernel config files.
Or, I'm misreading it and it's going to break the latter.
I'd suggest splitting things.

sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4068)

Why would this be defined already?

sys/conf/files
107–112 ↗(On Diff #4068)

And how are these picked up now with zfs?

sys/conf/kern.post.mk
188–191 ↗(On Diff #4068)

How was this not working before?

sys/conf/kern.pre.mk
152 ↗(On Diff #4068)

Lots of repetition with ZFS_CLFAGS. Perhaps you could combine the lists?

br edited edge metadata.
rpaulo requested changes to this revision.Mar 3 2015, 6:26 PM
rpaulo added a reviewer: rpaulo.
rpaulo added a subscriber: rpaulo.

The reason why we have DTrace and ZFS only as modules is because of the CDDL. Can you please make sure core is okay with this change? They will have access to all the email archives discussing the problems and the outcomes. They can tell you exactly the reasons why we never had DTrace/ZFS in the kernel.

This revision now requires changes to proceed.Mar 3 2015, 6:26 PM
In D1997#15, @rpaulo wrote:

The reason why we have DTrace and ZFS only as modules is because of the CDDL. Can you please make sure core is okay with this change? They will have access to all the email archives discussing the problems and the outcomes. They can tell you exactly the reasons why we never had DTrace/ZFS in the kernel.

core is ok. this only allows to proceed the build for someone who want to try it locally

sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4068)

It is defined here:

<built-in>:316:9: note: previous definition is here
#define LOCORE 1

what it that <built-in> ? clang ?

sys/conf/files
107–112 ↗(On Diff #4068)

Now it is assumed people should include 'device opensolaris' also with 'device zfs' in their kernel configs

107–112 ↗(On Diff #4068)

alternatively we can do something like:
optional zfs | dtrace compile-with "${ZFS_C}"

is it better ?

sys/conf/kern.post.mk
188–191 ↗(On Diff #4068)

these was working before fine for ZFS, but now it also work for dtrace.
Should we rename these defines to not confuse ?

br marked an inline comment as done.May 22 2015, 2:09 PM
br marked 2 inline comments as done.May 22 2015, 2:16 PM
br added inline comments.
sys/conf/kern.post.mk
187–192 ↗(On Diff #4090)

defines renamed to 'CDDL' assuming it is both for zfs and dtrace now

br marked 4 inline comments as done.May 22 2015, 2:18 PM
sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4090)

Doesn't it come from the command line?

sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4090)

yes it comes from sys/conf/kern.pre.mk:

DTRACE_ASM_CFLAGS= -x assembler-with-cpp -DLOCORE ${DTRACE_CFLAGS}

(I copied it from ZFS_ASM_CFLAGS)

should it be removed both from ZFS_ASM_CFLAGS and DTRACE_ASM_CFLAGS ?

sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4090)

The other way around I think: since it's now defined in the command line, can't you remove it from this file?

sys/cddl/dev/dtrace/arm/dtrace_asm.S
32 ↗(On Diff #4090)

yes it is also works both with modules & with no modules

br edited edge metadata.
br retitled this revision from This allow us to have dtrace in kernel (i.e. not as a modules) to This allows us to have dtrace in kernel (i.e. not as a modules). Required for aarch64.
sys/conf/files
248 ↗(On Diff #5680)

I think you should make the 'device' or 'options' lines match with the layout of the modules, so I would maybe make this 'optional dtmalloc | dtraceall' and similarly for the other things like lockstat, sdt, etc.

255 ↗(On Diff #5680)

This one should be something like 'optional dtnfscl nfscl | dtraceall nfscl'.

In addition, there should be some updates to some NOTES files, either sys/conf/NOTES for things that are truly MI or MD NOTES files for things that are only supported on a subset of platforms.

br edited edge metadata.

consider modules layout, add all the devices possible to arm/NOTES

br updated this revision to Diff 5686.
br marked 2 inline comments as done.
sys/conf/files
255 ↗(On Diff #5686)

sounds reasonable, thanks -- I updated the patch

Thanks. I would please ask that you consider adding the dtrace lines to the x86 and powerpc NOTES file as well (the other architectures that build the dtrace modules).

sys/amd64/conf/NOTES
20 ↗(On Diff #5688)

nitpick: DTrace - two initial capitals

I also wonder if we should mention the license issue in here, something like NOTE: introduces CDDL-licensed components into the kernel

do not include DTrace to LINT kernels

This revision was automatically updated to reflect the committed changes.