From owner-freebsd-hackers@FreeBSD.ORG Fri Nov 16 14:41:55 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 464A7DA6; Fri, 16 Nov 2012 14:41:55 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx1.freebsd.org (Postfix) with ESMTP id 82A278FC08; Fri, 16 Nov 2012 14:41:53 +0000 (UTC) Received: by mail-lb0-f182.google.com with SMTP id go10so84975lbb.13 for ; Fri, 16 Nov 2012 06:41:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=mMNOsan7hzUfDB0p3b3+z+HEMDsroJEiyFFD90gPlDI=; b=q+WZJA5m7tb8xKHVN4mZDYIr+TlC3zKUaXOXUnhZ0+O0KXu+l38C4VV0FEvhKaYKWz roGCb58uXa5VSjT8PskNow6qaU1jFRQqRQtWWcMtSXYPp14Ssco8NM2S4i0IQphnad+H JuMBeduTMxknryXbcAxp20Gyw3G9GrONALpfSXdb5YXHnMwJVrfLxmedysz6j2NjGmv/ m24pUD9cPtJdL2xNrWMjaW/Q+d4WzMlLxasqvltqMrpiq6l0NtLsRDlllwCXoc31tMfC kBSdnvjWRToz4ESFYDDIG1KSlEM4fNcauJE1rrOHI4o9PMlbXvH0LK6QizalU7l8yxDk 0qVg== MIME-Version: 1.0 Received: by 10.152.162.1 with SMTP id xw1mr4538329lab.3.1353076913094; Fri, 16 Nov 2012 06:41:53 -0800 (PST) Sender: asmrookie@gmail.com Received: by 10.112.134.5 with HTTP; Fri, 16 Nov 2012 06:41:52 -0800 (PST) In-Reply-To: <50A63D1D.9090500@FreeBSD.org> References: <50A5F12C.1050902@FreeBSD.org> <50A63D1D.9090500@FreeBSD.org> Date: Fri, 16 Nov 2012 14:41:52 +0000 X-Google-Sender-Auth: MhT3I1BWQSkQMehTXlTy32gWNNk Message-ID: Subject: Re: stop_cpus_hard when multiple CPUs are panicking from an NMI From: Attilio Rao To: Andriy Gapon Content-Type: text/plain; charset=UTF-8 Cc: freebsd-hackers@freebsd.org, Ryan Stone X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Nov 2012 14:41:55 -0000 On Fri, Nov 16, 2012 at 1:18 PM, Andriy Gapon wrote: > on 16/11/2012 14:30 Attilio Rao said the following: >> On Fri, Nov 16, 2012 at 7:54 AM, Andriy Gapon wrote: >>> on 16/11/2012 00:58 Ryan Stone said the following: >>>> At work we have some custom watchdog hardware that sends an NMI upon >>>> expiry. We've modified the kernel to panic when it receives the watchdog >>>> NMI. I've been trying the "stop_scheduler_on_panic" mode, and I've >>>> discovered that when my watchdog expires, the system gets completely >>>> wedged. After some digging, I've discovered is that I have multiple CPUs >>>> getting the watchdog NMI and trying to panic concurrently. One of the CPUs >>>> wins, and the rest spin forever in this code: >>>> >>>> /* >>>> * We don't want multiple CPU's to panic at the same time, so we >>>> * use panic_cpu as a simple spinlock. We have to keep checking >>>> * panic_cpu if we are spinning in case the panic on the first >>>> * CPU is canceled. >>>> */ >>>> if (panic_cpu != PCPU_GET(cpuid)) >>>> while (atomic_cmpset_int(&panic_cpu, NOCPU, >>>> PCPU_GET(cpuid)) == 0) >>>> while (panic_cpu != NOCPU) >>>> ; /* nothing */ >>>> >>>> The system wedges when stop_cpus_hard() is called, which sends NMIs to all >>>> of the other CPUs and waits for them to acknowledge that they are stopped >>>> before returning. However the CPU will not deliver an NMI to a CPU that is >>>> already handling an NMI, so the other CPUs that got a watchdog NMI and are >>>> spinning will never go into the NMI handler and acknowledge that they are >>>> stopped. >>> >>> I thought about this issue and fixed (in my tree) in a different way: >>> http://people.freebsd.org/~avg/cpu_stop-race.diff >>> >>> The need for spinlock_enter in the patch in not entirely clear. >>> The main idea is that a CPU which calls cpu_stop and loses a race should >>> voluntary enter cpustop_handler. >>> I am also not sure about MI-cleanness of this patch. >> >> It is similar to what I propose but with some differences: >> - It is not clean from MI perspective > > OK. > >> - I don't think we need to treact it specially, I would just >> unconditionally stop all the CPUs entering in the "spinlock zone", >> making the patch simpler. > > I couldn't understand this part. I'm sorry, I think I misread your patch. I was basically suggesting to fix Ryan case by calling cpustop_handler() (or the new MI interface) into the panic() function, in the case the CPU don't win the race for panic_cpu. Basically doing: Index: sys/kern/kern_shutdown.c =================================================================== --- sys/kern/kern_shutdown.c (revision 243154) +++ sys/kern/kern_shutdown.c (working copy) @@ -568,15 +568,11 @@ panic(const char *fmt, ...) #ifdef SMP /* * We don't want multiple CPU's to panic at the same time, so we - * use panic_cpu as a simple spinlock. We have to keep checking - * panic_cpu if we are spinning in case the panic on the first - * CPU is canceled. + * use panic_cpu as a simple lock. */ if (panic_cpu != PCPU_GET(cpuid)) - while (atomic_cmpset_int(&panic_cpu, NOCPU, - PCPU_GET(cpuid)) == 0) - while (panic_cpu != NOCPU) - ; /* nothing */ + if (atomic_cmpset_int(&panic_cpu, NOCPU, PCPU_GET(cpuid)) == 0) + cpustop_handler(); if (stop_scheduler_on_panic) { if (panicstr == NULL && !kdb_active) { Infact it seems to me that the comment is outdated and no longer represent truth. Attilio -- Peace can only be achieved by understanding - A. Einstein