thread: stash domain id to work around vtophys problems on ppc64
ClosedPublic
Actions

Authored by mjg on Nov 19 2020, 7:04 PM.

Details

Reviewers

kib
markj

Commits

rS367961: thread: stash domain id to work around vtophys problems on ppc64

Summary

Adding to zombie list can be perfomed by idle threads, which on ppc64 leads to the following panic:

panic: mtx_lock() by idle thread 0xc008000006437100 on sleep mutex kernelpmap @ /usr/home/luporl/git/master/sys/powerpc/aim/mmu_oea64.c:2237
cpuid = 31
time = 1605806644
KDB: stack backtrace:
0xc0080000014d91e0: at kdb_backtrace+0x60
0xc0080000014d92f0: at vpanic+0x1e0
0xc0080000014d93a0: at panic+0x40
0xc0080000014d93d0: at __mtx_lock_flags+0x23c
0xc0080000014d9480: at moea64_kextract+0x68
0xc0080000014d9560: at thread_stash+0x48
0xc0080000014d95a0: at mi_switch+0x1fc
0xc0080000014d9620: at critical_exit_preempt+0x88
0xc0080000014d9650: at cpu_idle+0xd4
0xc0080000014d96c0: at sched_idletd+0x440
0xc0080000014d9820: at fork_exit+0xc4
0xc0080000014d98c0: at fork_trampoline+0x18
0xc0080000014d98f0: at cpu_reset_handler+0x64

Work around the problem by saving the id on allocation time.

The field is added into a 4 byte hole, so there is no growth on LP64. I can make it conditional on NUMA to spare others.

Another option is to recognize domain ids would easily fit in char, add a typedef and stash the one byte in a smaller hole elsewhere.

Test Plan

Reported and tested by luporl.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mjg created this revision.Nov 19 2020, 7:04 PM

Herald added a subscriber: imp. · View Herald TranscriptNov 19 2020, 7:04 PM

Harbormaster completed remote builds in B34919: Diff 79778.Nov 19 2020, 7:04 PM

mjg requested review of this revision.Nov 19 2020, 7:04 PM

On such arches, for idle threads, you can add to the numa domain 0 regardless of actual domain. Or process stash directly, after all we are in idle thread and running.

Processing the list can take even more sleepable locks for example to free TIDs. At the end of it uma is also going to make the call in item_domain.

I don't see a good reason to penalize the platform or to create differences to the rest. If 32-bit struct thread growth is a problem that can easily by combated by either ifdefing on NUMA or making the field smaller -- there are many sub 4 byte holes in the struct.

markj accepted this revision.Nov 20 2020, 7:24 PM

markj added inline comments.

sys/sys/proc.h
246	It's safe to use a u_char for the domain, so the sizeof the structure remains the same on all platforms.

This revision is now accepted and ready to land.Nov 20 2020, 7:24 PM

stuff the domain in a 3 byte hole with u_char

This revision now requires review to proceed.Nov 20 2020, 8:07 PM

This is how it looks like on i386 (ptype /o struct thread):

/*  148      |     1 */    u_char td_lend_user_pri;
/*  149      |     1 */    u_char td_allocdomain;
/* XXX  2-byte hole  */
/*  152      |     4 */    int td_flags;

kib accepted this revision.Nov 21 2020, 12:29 AM

kib added inline comments.

sys/sys/proc.h
249	This comment should be improved. It needs to be absolutely clear that it is about struct thread memory, not any thread' affinity policits. Basically, it is of no use for anything except thread_reap() hack. No need to wait for me after you update the comment.