Page MenuHomeFreeBSD

Centralize __pcpu definitions.
ClosedPublic

Authored by kib on Aug 26 2019, 4:32 PM.

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.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
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.

kib updated this revision to Diff 61426.Aug 28 2019, 7:48 PM

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

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