> Date: Thu, 9 Apr 2020 10:01:09 +0200
> From: Martin Pieuchot <[email protected]>
> Cc: [email protected]
> Content-Type: text/plain; charset=utf-8
> Content-Disposition: inline
>
> cwen@ reported that GENERIC.MP on powerpc exposes a bug under high load
> which seems to always involve unlocking a rwlock and UVM swapping:
> https://marc.info/?l=openbsd-bugs&m=158534549422688&w=2
>
> One way to move forward and get more information is to run a bulk with
> WITNESS on powerpc. In order to get WITNESS working, powerpc needs to
> switch to the MI mplock and support stacktrace_save_at().
>
> Diff below does both. It is only compile tested. I don't own any
> power machine anymore. I'd appreciate test on real hardware, preferably
> not the build cluster first ;)
>
> stacktrace_save_at() can also be tested with dt(4)/btrace(8) on a SP
> machine.
>
> Note that if this goes in powerpc/lock_machdep.c can go to the Attic :)
>
> Comments, oks?
>
> Index: arch/powerpc/conf/files.powerpc
> ===================================================================
> RCS file: /cvs/src/sys/arch/powerpc/conf/files.powerpc,v
> retrieving revision 1.55
> diff -u -p -r1.55 files.powerpc
> --- arch/powerpc/conf/files.powerpc 25 Jan 2018 15:06:29 -0000 1.55
> +++ arch/powerpc/conf/files.powerpc 9 Apr 2020 07:38:37 -0000
> @@ -13,7 +13,6 @@ file arch/powerpc/powerpc/process_machde
> file arch/powerpc/powerpc/sys_machdep.c
> file arch/powerpc/powerpc/trap.c
> file arch/powerpc/powerpc/vm_machdep.c
> -file arch/powerpc/powerpc/lock_machdep.c multiprocessor
> file arch/powerpc/powerpc/intr.c
> file arch/powerpc/powerpc/softintr.c
>
> Index: arch/powerpc/ddb/db_trace.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/powerpc/ddb/db_trace.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 db_trace.c
> --- arch/powerpc/ddb/db_trace.c 7 Nov 2019 16:08:08 -0000 1.14
> +++ arch/powerpc/ddb/db_trace.c 9 Apr 2020 07:50:56 -0000
> @@ -31,6 +31,7 @@
> #include <sys/systm.h>
> #include <sys/proc.h>
> #include <sys/user.h>
> +#include <sys/stacktrace.h>
>
> #include <uvm/uvm_extern.h>
>
> @@ -221,4 +222,39 @@ db_stack_trace_print(db_expr_t addr, int
> --count;
> }
> (*pr)("end trace frame: 0x%lx, count: %d\n", sp, count);
> +}
> +
> +void
> +stacktrace_save_at(struct stacktrace *st, unsigned int skip)
> +{
> + vaddr_t lr, sp, lastsp;
> +
> + sp = (vaddr_t)__builtin_frame_address(0);
> + KASSERT(INKERNEL(sp) || ININTSTK(sp));
> +
> + st->st_count = 0;
> + while (st->st_count < STACKTRACE_MAX) {
> + lr = *(vaddr_t *)(sp + 4) - 4;
> + if (lr & 3)
> + break;
> +
> + if (skip == 0)
> + st->st_pc[st->st_count++] = lr;
> + else
> + skip--;
> +
> + lastsp = sp;
> + sp = *(vaddr_t *)sp;
> +
> + if ((sp == 0) || (sp <= lastsp))
> + break;
I think checking the alignment of sp here like you do for lr would be
a good idea. Otherwise an unaligned access might happen. Since
powerpc uses separate interrupt stacks, the (sp <= lastsp) check might
truncate the stack trace.
> + if (!INKERNEL(sp) && !ININTSTK(sp))
> + break;
> + }
> +}
> +
> +void
> +stacktrace_save(struct stacktrace *st)
> +{
> + return stacktrace_save_at(st, 0);
> }
> Index: arch/powerpc/include/mplock.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/powerpc/include/mplock.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 mplock.h
> --- arch/powerpc/include/mplock.h 4 Dec 2017 09:51:03 -0000 1.3
> +++ arch/powerpc/include/mplock.h 9 Apr 2020 07:37:25 -0000
> @@ -1,52 +1,10 @@
> /* $OpenBSD: mplock.h,v 1.3 2017/12/04 09:51:03 mpi Exp $ */
>
> -/*
> - * Copyright (c) 2004 Niklas Hallqvist. All rights reserved.
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - * 1. Redistributions of source code must retain the above copyright
> - * notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - * notice, this list of conditions and the following disclaimer in the
> - * documentation and/or other materials provided with the distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> - * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> - * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> - * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> - * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> - * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> - * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> - */
> +/* public domain */
>
> #ifndef _POWERPC_MPLOCK_H_
> #define _POWERPC_MPLOCK_H_
>
> -/*
> - * Really simple spinlock implementation with recursive capabilities.
> - * Correctness is paramount, no fancyness allowed.
> - */
> -
> -struct __mp_lock {
> - volatile struct cpu_info *mpl_cpu;
> - volatile long mpl_count;
> -};
> -
> -#ifndef _LOCORE
> -
> -void __mp_lock_init(struct __mp_lock *);
> -void __mp_lock(struct __mp_lock *);
> -void __mp_unlock(struct __mp_lock *);
> -int __mp_release_all(struct __mp_lock *);
> -int __mp_release_all_but_one(struct __mp_lock *);
> -void __mp_acquire_count(struct __mp_lock *, int);
> -int __mp_lock_held(struct __mp_lock *, struct cpu_info *);
> -
> -#endif
> +#define __USE_MI_MPLOCK
>
> #endif /* !_POWERPC_MPLOCK_H */
> Index: arch/macppc/conf/GENERIC.MP
> ===================================================================
> RCS file: /cvs/src/sys/arch/macppc/conf/GENERIC.MP,v
> retrieving revision 1.1
> diff -u -p -r1.1 GENERIC.MP
> --- arch/macppc/conf/GENERIC.MP 3 May 2007 21:21:33 -0000 1.1
> +++ arch/macppc/conf/GENERIC.MP 9 Apr 2020 07:39:26 -0000
> @@ -4,5 +4,6 @@ include "arch/macppc/conf/GENERIC"
>
> option MULTIPROCESSOR
> #option MP_LOCKDEBUG
> +#option WITNESS
>
> cpu* at mainbus?
>
>
