From owner-freebsd-net@freebsd.org Mon Jun 20 17:46:04 2016 Return-Path: Delivered-To: freebsd-net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 677C9A7A15B for ; Mon, 20 Jun 2016 17:46:04 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: from mailman.ysv.freebsd.org (unknown [127.0.1.3]) by mx1.freebsd.org (Postfix) with ESMTP id 4B1511814 for ; Mon, 20 Jun 2016 17:46:04 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: by mailman.ysv.freebsd.org (Postfix) id 43234A7A158; Mon, 20 Jun 2016 17:46:04 +0000 (UTC) Delivered-To: net@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 42807A7A156; Mon, 20 Jun 2016 17:46:04 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D9A50180F; Mon, 20 Jun 2016 17:46:03 +0000 (UTC) (envelope-from julien.charbon@gmail.com) Received: by mail-wm0-f49.google.com with SMTP id a66so89228845wme.0; Mon, 20 Jun 2016 10:46:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=I5fID0TamFhGqwFKXXP2M0uoJBITkuSZcBf7+qEgfMo=; b=BcGz9FqHx9fnP5e+VxXgPXos0tfIb7tYpHJ4OOqi0IohnGCjS9fN+tEOC/5mJy9K2J Ynwlp6ssMdoU+vgbEWeKlOQtsAkJuGBDeoEIZ64u+m7rdZ6BFoaGJavC+nlSyIfJyRqM fWQz6Vc1i8DxRWnR7WeqD1T3SeYrFhndN+DsNTpybm1ubxePTsJINW03yEi+qGXvdWRZ AX1OjIaeFZH6Jxe4tHr9trxWO+1Mb+6wftSEQgP+ORNLp/mqaatL5sAQ3mzQ1lMETpFJ 0HPhat8ZdV8TJE46BSRdXFPfu9i8oiLk0X1qq5H6VO8/p0TaqOrGzXZ8033OVq32EX9M I6XA== X-Gm-Message-State: ALyK8tKA1HntFiEpMIbttz1Rqb5rnlFTZzEIQ/rCtdWXIFCDkaVedxFKvrxLMatHruyq7Q== X-Received: by 10.28.157.1 with SMTP id g1mr12886852wme.34.1466444755441; Mon, 20 Jun 2016 10:45:55 -0700 (PDT) Received: from [10.100.64.16] ([217.30.88.7]) by smtp.gmail.com with ESMTPSA id li10sm20146765wjb.5.2016.06.20.10.45.54 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 20 Jun 2016 10:45:54 -0700 (PDT) Subject: Re: panic with tcp timers To: Gleb Smirnoff References: <20160617045319.GE1076@FreeBSD.org> <1f28844b-b4ea-b544-3892-811f2be327b9@freebsd.org> <20160620073917.GI1076@FreeBSD.org> <1d18d0e2-3e42-cb26-928c-2989d0751884@freebsd.org> <20160620095822.GJ1076@FreeBSD.org> Cc: rrs@FreeBSD.org, hselasky@FreeBSD.org, net@FreeBSD.org, current@FreeBSD.org From: Julien Charbon Message-ID: <74bb31b7-a9f5-3d0c-eea0-681872e6f09b@freebsd.org> Date: Mon, 20 Jun 2016 19:45:53 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160620095822.GJ1076@FreeBSD.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Jun 2016 17:46:04 -0000 Hi, On 6/20/16 11:58 AM, Gleb Smirnoff wrote: > On Mon, Jun 20, 2016 at 11:55:55AM +0200, Julien Charbon wrote: > J> > On Fri, Jun 17, 2016 at 11:27:39AM +0200, Julien Charbon wrote: > J> > J> > Comparing stable/10 and head, I see two changes that could > J> > J> > affect that: > J> > J> > > J> > J> > - callout_async_drain > J> > J> > - switch to READ lock for inp info in tcp timers > J> > J> > > J> > J> > That's why you are in To, Julien and Hans :) > J> > J> > > J> > J> > We continue investigating, and I will keep you updated. > J> > J> > However, any help is welcome. I can share cores. > J> > > J> > Now, spending some time with cores and adding a bunch of > J> > extra CTRs, I have a sequence of events that lead to the > J> > panic. In short, the bug is in the callout system. It seems > J> > to be not relevant to the callout_async_drain, at least for > J> > now. The transition to READ lock unmasked the problem, that's > J> > why NetflixBSD 10 doesn't panic. > J> > > J> > The panic requires heavy contention on the TCP info lock. > J> > > J> > [CPU 1] the callout fires, tcp_timer_keep entered > J> > [CPU 1] blocks on INP_INFO_RLOCK(&V_tcbinfo); > J> > [CPU 2] schedules the callout > J> > [CPU 2] tcp_discardcb called > J> > [CPU 2] callout successfully canceled > J> > [CPU 2] tcpcb freed > J> > [CPU 1] unblocks... panic > J> > > J> > When the lock was WLOCK, all contenders were resumed in a > J> > sequence they came to the lock. Now, that they are readers, > J> > once the lock is released, readers are resumed in a "random" > J> > order, and this allows tcp_discardcb to go before the old > J> > running callout, and this unmasks the panic. > J> > J> Highly interesting. I should be able to reproduce that (will be useful > J> for testing the corresponding fix). > J> > J> Fix proposal: If callout_async_drain() returns 0 (fail) (instead of 1 > J> (success) here) when the callout cancellation is a success _but_ the > J> callout is current running, that should fix it. > J> > J> For the history: It comes back to my old callout question: > J> > J> Does _callout_stop_safe() is allowed to return 1 (success) even if the > J> callout is still currently running; a.k.a. it is not because you > J> successfully cancelled a callout that the callout is not currently running. > J> > J> We did propose a patch to make _callout_stop_safe() returns 0 (fail) > J> when the callout is currently running: > J> > J> callout_stop() should return 0 when the callout is currently being > J> serviced and indeed unstoppable > J> https://reviews.freebsd.org/differential/changeset/?ref=62513&whitespace=ignore-most > J> > J> But this change impacted too many old code paths and was interesting > J> only for TCP timers and thus was abandoned. > > The fix I am working on now is doing exactly that. callout_reset must > return 0 if the callout is currently running. > > What are the old paths impacted? Actually all the paths that check the callout_stop() return value AND call both callout_reset() and callout_stop() AND use mpsafe callout(). And for each path, we would have to check our patch was ok (or not). Thus, if you only do the change in callout_async_drain() context, you don't impact the "old" callout_stop() behavior and get the desired behavior for the TCP timers. Might be a good trade-off... My 2 cents. -- Julien