Page MenuHomeFreeBSD
Differential D21418

Centralize __pcpu definitions.
ClosedPublic
Actions

Authored by kib on Aug 26 2019, 4:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 12:35 AM
Unknown Object (File)
Fri, Nov 22, 11:52 AM
Unknown Object (File)
Fri, Nov 22, 11:52 AM
Unknown Object (File)
Fri, Nov 22, 11:52 AM
Unknown Object (File)
Fri, Nov 22, 11:51 AM
Unknown Object (File)
Fri, Nov 22, 11:51 AM
Unknown Object (File)
Fri, Nov 22, 11:50 AM
Unknown Object (File)
Fri, Nov 22, 11:50 AM
View All 91 Files
Subscribers
andrew
bcran
bdragon
emaste
imp
jhibbits
rgrimes
swills

Details

Reviewers
markj
jhb
lwhsu
imp
manu
Group Reviewers
bhyve
Commits
rS351594: Centralize __pcpu definitions.
Summary

Many extern struct pcpu <something>__pcpu declarations were copied/pasted in sources. The issue is the definition is MD, but it cannot be provided by machine/pcpu.h due to actual struct pcpu defined in sys/pcpu.h later than the inclusion of machine/pcpu.h. There is no way around it, due to machine/pcpu.h supplying part of struct pcpu fields.

To work around the problem, add a new machine/pcpu_aux.h header, which should fill any needed MD definitions after struct pcpu definition is completed. This allows to remove copies of __pcpu spread around the source.

While there, fix bhyve.

[I only handled x86 and riscv, the later was not compiled. I will finish the patch after the discussion]

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kib created this revision.Aug 26 2019, 4:32 PM
Herald added a reviewer: bhyve. · View Herald TranscriptAug 26 2019, 4:32 PM
Herald added subscribers: bcran, rgrimes, imp. · View Herald Transcript
Harbormaster completed remote builds in B26108: Diff 61292.Aug 26 2019, 4:32 PM
imp accepted this revision.Aug 26 2019, 4:41 PM

Seems a bit wasteful to have multiple files with the same bits, but since some are arrays and others are pointers, I'm unsure of what to suggest instead.

This revision is now accepted and ready to land.Aug 26 2019, 4:41 PM
jhb added inline comments.Aug 26 2019, 4:55 PM
sys/sys/pcpu.h
175 ↗(On Diff #61292)

Maybe it would be simpler if we inverted these macros and had sys/pcpu.h define a PCPU_MI_FIELDS macro before #including <machine/pcpu.h> and then the MD header would define the actual 'struct pcpu' using 'PCPU_MI_FIELDS' as the first line? We could add a static assert that offsetof(struct pcpu, pc_curthread) == 0 in subr_pcpu.c. Would this let us keep a single header?

kib added inline comments.Aug 26 2019, 5:58 PM
sys/sys/pcpu.h
175 ↗(On Diff #61292)

I suspect it would not, because due some definitions in sys/pcpu.h now would require placement after machine/pcpu.h defined complete struct pcpu. cpuhead is the first example.

Also IMO it would cause much larger churn in source.

kib updated this revision to Diff 61305.Aug 26 2019, 5:59 PM

pcpu_aux.h also allows to heal some wounds like OFFSETOF_CURTHREAD and remove clang-specific workarounds, since offsetof() is not literal 0 AKA NULL.

This revision now requires review to proceed.Aug 26 2019, 5:59 PM
Harbormaster completed remote builds in B26113: Diff 61305.Aug 26 2019, 5:59 PM
markj added a comment.Aug 26 2019, 7:12 PM

It seems reasonable to me.

sys/riscv/include/pcpu_aux.h
46 ↗(On Diff #61305)

"... zones, the page size should be a multiple of the size of struct pcpu."

49 ↗(On Diff #61305)

This could be written more simply as PAGE_SIZE % sizeof(struct pcpu) == 0.

kib updated this revision to Diff 61312.Aug 26 2019, 7:48 PM
kib marked 2 inline comments as done.

risc-v pcpu_aux.h editing.

Harbormaster completed remote builds in B26114: Diff 61312.Aug 26 2019, 7:48 PM
lwhsu mentioned this in D21417: Fix amd64-gcc build.Aug 27 2019, 9:49 AM
kib updated this revision to Diff 61426.Aug 28 2019, 7:48 PM

Complete pass over rest of the arches. This version passes tinderbox.

Herald added a reviewer: manu. · View Herald TranscriptAug 28 2019, 7:48 PM
Herald added subscribers: bdragon, jhibbits, emaste, andrew. · View Herald Transcript
Harbormaster completed remote builds in B26165: Diff 61426.Aug 28 2019, 7:49 PM
lwhsu added inline comments.Aug 28 2019, 7:52 PM
sys/sparc64/include/pcpu_aux.h~
1 ↗(On Diff #61426)

this file looks accidentally added.

markj accepted this revision as: markj.Aug 28 2019, 7:52 PM

There is both sparc64/pcpu_aux.h and sparc64/pcpu_aux.h~ in the diff.

This revision is now accepted and ready to land.Aug 28 2019, 7:52 PM
kib updated this revision to Diff 61429.Aug 28 2019, 8:05 PM

Remove emacs' backup.

This revision now requires review to proceed.Aug 28 2019, 8:05 PM
Harbormaster completed remote builds in B26166: Diff 61429.Aug 28 2019, 8:05 PM
swills added a subscriber: swills.Aug 28 2019, 9:11 PM
This revision was not accepted when it landed; it landed in state Needs Review.Aug 29 2019, 7:25 AM
Closed by commit rS351594: Centralize __pcpu definitions. (authored by kib). · Explain Why
This revision was automatically updated to reflect the committed changes.
kib added a commit: rS351594: Centralize __pcpu definitions..

Revision Contents
Changeset List

PathSize
head/
sys/
amd64/
amd64/
2 lines
4 lines
4 lines
include/
3 lines
29 lines
72 lines
arm/
arm/
1 line
include/
2 lines
52 lines
arm64/
arm64/
3 lines
include/
2 lines
52 lines
i386/
i386/
2 lines
4 lines
include/
2 lines
30 lines
71 lines
mips/
include/
52 lines
powerpc/
include/
2 lines
52 lines
powerpc/
2 lines
riscv/
include/
2 lines
52 lines
riscv/
2 lines
sparc64/
include/
50 lines
sys/
22 lines

Diff 61444

View Options

head/sys/amd64/amd64/mp_machdep.c

Loading...
View Options

head/sys/amd64/amd64/pmap.c

Loading...
View Options

head/sys/amd64/amd64/vm_machdep.c

Loading...
View Options

head/sys/amd64/include/counter.h

Loading...
View Options

head/sys/amd64/include/pcpu.h

Loading...
View Options

head/sys/amd64/include/pcpu_aux.h

Loading...
View Options

head/sys/arm/arm/mp_machdep.c

Loading...
View Options

head/sys/arm/include/counter.h

Loading...
View Options

head/sys/arm/include/pcpu_aux.h

Loading...
View Options

head/sys/arm64/arm64/mp_machdep.c

Loading...
View Options

head/sys/arm64/include/counter.h

Loading...
View Options

head/sys/arm64/include/pcpu_aux.h

Loading...
View Options

head/sys/i386/i386/mp_machdep.c

Loading...
View Options

head/sys/i386/i386/vm_machdep.c

Loading...
View Options

head/sys/i386/include/counter.h

Loading...
View Options

head/sys/i386/include/pcpu.h

Loading...
View Options

head/sys/i386/include/pcpu_aux.h

Loading...
View Options

head/sys/mips/include/pcpu_aux.h

Loading...
View Options

head/sys/powerpc/include/counter.h

Loading...
View Options

head/sys/powerpc/include/pcpu_aux.h

Loading...
View Options

head/sys/powerpc/powerpc/mp_machdep.c

Loading...
View Options

head/sys/riscv/include/counter.h

Loading...
View Options

head/sys/riscv/include/pcpu_aux.h

Loading...
View Options

head/sys/riscv/riscv/mp_machdep.c

Loading...
View Options

head/sys/sparc64/include/pcpu_aux.h

Loading...
View Options

head/sys/sys/pcpu.h

Loading...