From owner-svn-src-projects@FreeBSD.ORG Sat Sep 8 18:00:40 2012 Return-Path: Delivered-To: svn-src-projects@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 64DB810656D3; Sat, 8 Sep 2012 18:00:40 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id CF1F28FC0C; Sat, 8 Sep 2012 18:00:36 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q88I0fHM074249; Sat, 8 Sep 2012 21:00:41 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q88I0SGT090854; Sat, 8 Sep 2012 21:00:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q88I0S6E090853; Sat, 8 Sep 2012 21:00:28 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sat, 8 Sep 2012 21:00:28 +0300 From: Konstantin Belousov To: Attilio Rao Message-ID: <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> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="R8jsNM7kDXYEcGgQ" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua 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 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:00:40 -0000 --R8jsNM7kDXYEcGgQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wro= te: > >> > On Mon, Jul 30, 2012 at 4:31 PM, Andriy Gapon wrot= e: > >> >> 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 int= errupt > >> >>>>> context, not only related to this part of callouts. This would b= e a > >> >>>>> very good help for catching buggy situations. > >> >>>> > >> >>>> Something very tangential. I think it would also be nice to chec= k 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 wan= t to > >> >>> add the burden to bump td_locks for the fast case and I think it i= s 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_pf= lags) 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 a= nd > >> > 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 n= ot > > held, is no longer needed. >=20 > 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 =3D=3D 0). >=20 > > I had similar thought from the time when I added TDP_NOFAULTING check t= o the > > syscallret(), but the loss of the syscall name in the panic output alwa= ys > > stopped me. >=20 > I think in case of a lock/td_pinned/td_critnest/etc. leak, having the > syscall number in the panic message won't change anything as you will > likely need a coredump and possibly instrument your kernel with > further debugging, etc. to see what's going on. For me, when I debugged TDP_NOFAULTING (which indeed leaked in the intermediate versions of the patch) the syscall name appeared to be enough. But, I do not object strongly. --R8jsNM7kDXYEcGgQ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBLh7wACgkQC3+MBN1Mb4i/KQCg8BVm0w9GU1THNDTnBS+PEKn/ DWkAoMfMe9D9ujRlffU9U6GUIPx6WT7f =1f6K -----END PGP SIGNATURE----- --R8jsNM7kDXYEcGgQ--