Page MenuHomeFreeBSD

Correct padding length for RISC-V PCPU data.
AcceptedPublic

Authored by jhb on Tue, Jul 28, 6:17 PM.

Details

Reviewers
br
mhorne
jrtc27
Summary

There was an additional 7 bytes of compiler-inserted padding at the
end of the structure visible via 'ptype /o' in gdb.

Obtained from: CheriBSD

Test Plan
  • built a kernel and compared ptype /o before/after

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32762
Build 30197: arc lint + arc unit

Event Timeline

jhb requested review of this revision.Tue, Jul 28, 6:17 PM
jhb created this revision.
jhb added a comment.Tue, Jul 28, 6:19 PM

Before:

(gdb) ptype /o struct pcpu
/* offset    |  size */  type = struct pcpu {
/*    0      |     8 */    struct thread *pc_curthread;
/*    8      |     8 */    struct thread *pc_idlethread;
/*   16      |     8 */    struct thread *pc_fpcurthread;
/*   24      |     8 */    struct thread *pc_deadthread;
/*   32      |     8 */    struct pcb *pc_curpcb;
/*   40      |    16 */    void *pc_sched;
/*   48      |     8 */    uint64_t pc_switchtime;
/*   56      |     4 */    int pc_switchticks;
/*   60      |     4 */    u_int pc_cpuid;
/*   64      |     8 */    struct {
/*   64      |     8 */        struct pcpu *stqe_next;

                               /* total size (bytes):    8 */
                           } pc_allcpu;
/*   72      |     8 */    struct lock_list_entry *pc_spinlocks;
/*   80      |    40 */    long pc_cp_time[5];
/*  120      |     8 */    struct device *pc_device;
/*  128      |    16 */    void *pc_netisr;
/*  136      |     4 */    int pc_unused1;
/*  140      |     4 */    int pc_domain;
/*  144      |    16 */    struct rm_queue {
/*  144      |     8 */        struct rm_queue * volatile rmq_next;
--Type <RET> for more, q to quit, c to continue without paging--
/*  152      |     8 */        struct rm_queue * volatile rmq_prev;

                               /* total size (bytes):   16 */
                           } pc_rm_queue;
/*  160      |     8 */    uintptr_t pc_dynamic;
/*  168      |     8 */    uint64_t pc_early_dummy_counter;
/*  176      |     8 */    uintptr_t pc_zpcpu_offset;
/*  184      |     8 */    struct pmap *pc_curpmap;
/*  192      |     4 */    uint32_t pc_pending_ipis;
/*  196      |     4 */    uint32_t pc_hart;
/*  200      |    49 */    char __pad[49];
/* XXX  7-byte padding */

                           /* total size (bytes):  256 */
                         }

after:

(gdb) ptype /o struct pcpu
/* offset    |  size */  type = struct pcpu {
/*    0      |     8 */    struct thread *pc_curthread;
/*    8      |     8 */    struct thread *pc_idlethread;
/*   16      |     8 */    struct thread *pc_fpcurthread;
/*   24      |     8 */    struct thread *pc_deadthread;
/*   32      |     8 */    struct pcb *pc_curpcb;
/*   40      |     8 */    void *pc_sched;
/*   48      |     8 */    uint64_t pc_switchtime;
/*   56      |     4 */    int pc_switchticks;
/*   60      |     4 */    u_int pc_cpuid;
/*   64      |     8 */    struct {
/*   64      |     8 */        struct pcpu *stqe_next;

                               /* total size (bytes):    8 */
                           } pc_allcpu;
/*   72      |     8 */    struct lock_list_entry *pc_spinlocks;
/*   80      |    40 */    long pc_cp_time[5];
/*  120      |     8 */    struct device *pc_device;
/*  128      |     8 */    void *pc_netisr;
/*  136      |     4 */    int pc_unused1;
/*  140      |     4 */    int pc_domain;
/*  144      |    16 */    struct rm_queue {
/*  144      |     8 */        struct rm_queue * volatile rmq_next;
--Type <RET> for more, q to quit, c to continue without paging--
/*  152      |     8 */        struct rm_queue * volatile rmq_prev;

                               /* total size (bytes):   16 */
                           } pc_rm_queue;
/*  160      |     8 */    uintptr_t pc_dynamic;
/*  168      |     8 */    uint64_t pc_early_dummy_counter;
/*  176      |     8 */    uintptr_t pc_zpcpu_offset;
/*  184      |     8 */    struct pmap *pc_curpmap;
/*  192      |     4 */    uint32_t pc_pending_ipis;
/*  196      |     4 */    uint32_t pc_hart;
/*  200      |    56 */    char __pad[56];

                           /* total size (bytes):  256 */
                         }
  1. Can we please properly document where this magic value comes from?
  1. Can we make pcpu_aux.h also assert that offsetof(__pad) + sizeof(__pad) == sizeof(struct pcpu) so all the padding is accounted for?
jrtc27 added inline comments.Tue, Jul 28, 6:34 PM
sys/riscv/include/pcpu.h
51

i.e. something like this is probably enough for explaining why this exists and where the value comes from

mhorne accepted this revision.Wed, Jul 29, 2:47 PM

Nice find. The patch seems fine as is, but @jrtc27's suggestions are worth implementing.

sys/riscv/include/pcpu.h
51

Seems that the alignment is to CACHE_LINE_SIZE, not page boundary.

This revision is now accepted and ready to land.Wed, Jul 29, 2:47 PM
jrtc27 added inline comments.Wed, Jul 29, 3:00 PM
sys/riscv/include/pcpu.h
51

Ah, it's not even that:

sys/riscv/include/pcpu_aux.h:_Static_assert(PAGE_SIZE % sizeof(struct pcpu) == 0, "fix pcpu size");

It's checking that they are a _factor_ of PAGE_SIZE, presumably so we can guarantee that only one page needs to be mapped for each PCPU (i.e. not just that it fits in a page, but also that it never straddles two pages)?

(except for amd64/i386, which do sizeof(struct pcpu) == UMA_PCPU_ALLOC_SIZE, where the latter is just defined to PAGE_SIZE in sys/sys/pcpu.h)

jhb updated this revision to Diff 75403.Tue, Aug 4, 9:45 PM
  • Add a comment and static assertion for pad size.
This revision now requires review to proceed.Tue, Aug 4, 9:45 PM
mhorne accepted this revision.Tue, Aug 4, 10:01 PM
mhorne added inline comments.
sys/riscv/include/pcpu_aux.h
50

Neat, I hadn't considered that you could get the size of a struct member that way.

This revision is now accepted and ready to land.Tue, Aug 4, 10:01 PM