From owner-svn-src-projects@FreeBSD.ORG Sat Sep 8 18:37:49 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id BEDF7106564A; Sat, 8 Sep 2012 18:37:49 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-lpp01m010-f54.google.com (mail-lpp01m010-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 43E998FC17; Sat, 8 Sep 2012 18:37:47 +0000 (UTC) Received: by lage12 with SMTP id e12so598684lag.13 for ; Sat, 08 Sep 2012 11:37:46 -0700 (PDT) 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=Utg6i+DX+j1JNLxrRgLwotfg+jZ2c4Ze37PWdyJfXqw=; b=uCn9X4221pyn1+RY8LNIAsKAQ0qFevXgl14WM9KFW+GoPn7GJ05hHdvd1d/NjJYf2m +8p6WhS8cnnptVTrCe3+0JsV827nkaspJIorro6WF0WxEiGamD2VHNF1mA5cOJqlw7kh 1ZZbeHkzgtkqUB67MIZcAFiobnlwWNtGs3vbdEPl4sgJid/HHokUS6dhFfrc3DtqtVtU X9ZwrmPQXSrtWvyls7HVTqwgsdBai4z4fks4JMOOlpCiz2e1v+O8yCYGZiovCly8GaiF 2MymxonovnjDKCQtM1gUS8QzSIXE2s5o8EXwqUFMAdQmJp1YlFu6nxbcuWyJGQh1xT6t l2Ng== MIME-Version: 1.0 Received: by 10.152.46.209 with SMTP id x17mr8282554lam.38.1347129466734; Sat, 08 Sep 2012 11:37:46 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.102.39 with HTTP; Sat, 8 Sep 2012 11:37:46 -0700 (PDT) In-Reply-To: <20120908180028.GJ33100@deviant.kiev.zoral.com.ua> References: <20120730143943.GY2676@deviant.kiev.zoral.com.ua> <5016A21B.6090409@FreeBSD.org> <5016A8E4.7070405@FreeBSD.org> <20120908153839.GF33100@deviant.kiev.zoral.com.ua> <20120908180028.GJ33100@deviant.kiev.zoral.com.ua> Date: Sat, 8 Sep 2012 19:37:46 +0100 X-Google-Sender-Auth: WCfzKCT3Rm6expcFnu1QHaGpGSo Message-ID: From: Attilio Rao To: Konstantin Belousov Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , svn-src-projects@freebsd.org, src-committers@freebsd.org, Andriy Gapon Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 08 Sep 2012 18:37:49 -0000 On Sat, Sep 8, 2012 at 7:00 PM, Konstantin Belousov wrote: > On Sat, Sep 08, 2012 at 06:14:41PM +0100, Attilio Rao wrote: >> On Sat, Sep 8, 2012 at 4:38 PM, Konstantin Belousov wrote: >> > On Fri, Sep 07, 2012 at 11:35:15PM +0100, Attilio Rao wrote: >> >> On Mon, Jul 30, 2012 at 9:39 PM, Attilio Rao wrote: >> >> > On Mon, Jul 30, 2012 at 4:31 PM, Andriy Gapon wrote: >> >> >> on 30/07/2012 18:04 Attilio Rao said the following: >> >> >>> On 7/30/12, Andriy Gapon wrote: >> >> >>>> on 30/07/2012 17:56 Attilio Rao said the following: >> >> >>>>> More explicitly, I think such combination TDP_NOSLEEPING + >> >> >>>>> TDP_NOBLOCKING (name invented) should be set on entering the interrupt >> >> >>>>> context, not only related to this part of callouts. This would be a >> >> >>>>> very good help for catching buggy situations. >> >> >>>> >> >> >>>> Something very tangential. I think it would also be nice to check if a >> >> >>>> thread has >> >> >>>> any(?) locks held when returning to userland. >> >> >>> >> >> >>> This happens already for INVARIANTS case, with td_locks counters. >> >> >>> In the !INVARIANTS case, this doesn't happen because you don't want to >> >> >>> add the burden to bump td_locks for the fast case and I think it is a >> >> >>> good approach. >> >> >> >> >> >> Ah, I missed that, thank you. >> >> >> BTW, it seems that td_locks is checked twice in normal syscallret() path: once in >> >> >> syscallret() itself and then in userret(). On this note, would it make sense to >> >> >> move the whole nine yards of asserts from syscallret() to userret()? >> >> >> I mean it might make sense to have those checks (td_critnest, td_pflags) in other >> >> >> paths to userland. >> >> > >> >> > Nice catch. >> >> > The checks were added to syscallret() in r208453. While this is fine, >> >> > I think that putting them in userret() may give them more exposure and >> >> > cover also cases like traps which are not covered right now. >> >> > If you want to make a patch that moves these conditions in userret() >> >> > I'd be in favor of it. >> >> >> >> More specifically, what do you think about this patch?: >> >> http://www.freebsd.org/~attilio/userret_diag.patch >> >> >> >> Of course I moved the XEN par too before the checks. >> >> The patch survived to few consecutive and parallel buildworlds, FWIW. >> > >> > At least in fork_return(), the last assert which checks that Giant is not >> > held, is no longer needed. >> >> Actually, this is unnecessary also with -CURRENT stock code today, >> because userret() already checks for td_locks. And it seems >> fork_return() is not the only function where this happens, as trap() >> did this too on x86 and possibly all the other architectures grew it >> with cut&paste. Possibly we need this further, separate, patch before >> userred_diag: >> http://www.freebsd.org/~attilio/userret_nogiant.patch > Yes, this looks good. I suggest to move #ifdef XEN part of the userret() > to befire KASSERT(td->td_locks == 0). All committed as r240244,240245,240246. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein