From owner-dev-commits-src-all@freebsd.org Thu Aug 5 17:46:26 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id BE06C667C8A; Thu, 5 Aug 2021 17:46:26 +0000 (UTC) (envelope-from gallatin@cs.duke.edu) Received: from duke.cs.duke.edu (duke.cs.duke.edu [152.3.140.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4GgbdG3SK9z3hgp; Thu, 5 Aug 2021 17:46:26 +0000 (UTC) (envelope-from gallatin@cs.duke.edu) Received: from [192.168.1.2] (pool-74-110-137-7.rcmdva.fios.verizon.net [74.110.137.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: gallatin) by duke.cs.duke.edu (Postfix) with ESMTPSA id D66062700137; Thu, 5 Aug 2021 13:46:25 -0400 (EDT) DMARC-Filter: OpenDMARC Filter v1.3.1 duke.cs.duke.edu D66062700137 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=cs.duke.edu; s=mail0816; t=1628185586; bh=y7n+r9JdomK31h/wi2srOGwsiANpX5UxKlJG5qUgR3s=; h=Subject:To:From:Date:From; b=J0tR06V5iro1Zgb2Xu3UKLMD30uf2wG+F8CRuZgPmpDGhfIFAOFSEfgySYiJwdzRR b+h4bx8UlNhhenY7vfdAVQFUYG84uoRB8SArnIOMlCsSkgz2bEX2taOBrwWqtVd9yG k6s5LPdV1aOtLMhPz2fdPv/SkHb4TVXW7+hjK2dbxVO5q5J6Yy9S9Hs9MssVb7f/ug /Nwec8LZpf+uLmbdA2s53AOvhGf/KOzGYkQmZutJGQMNKZUiIElocYmv31xD7YXO7N +okq8UNFfNiKrL7MVw4arDc/isvMzf0wnxpJ+jClRU6hxmO9e5OIBnATuVwfIZbGeo AiH35x0rn6vsg== Subject: Re: git: 2694c869ff9f - main - ktls: fix a panic with INVARIANTS To: Ian Lepore , Andrew Gallatin , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202108051710.175HAP1D031061@gitrepo.freebsd.org> <1013cb35c9a2c3cc55b4f3bd10d14b1540c9462f.camel@freebsd.org> From: Andrew Gallatin Message-ID: <69995f5d-9228-f956-aef5-d3cd4a4caa3d@cs.duke.edu> Date: Thu, 5 Aug 2021 13:46:25 -0400 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <1013cb35c9a2c3cc55b4f3bd10d14b1540c9462f.camel@freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 4GgbdG3SK9z3hgp X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Aug 2021 17:46:26 -0000 On 8/5/21 1:41 PM, Ian Lepore wrote:       if (nbufs != ktls_max_alloc) { > > Finding a different way to spell "forever" is not a valid way to fix a > problem where you're being warned that it is not safe to sleep forever. > > The assert was warning you that the code was vulnerable to hanging > forever due to a missed wakeup. The code is still vulnerable to that, > but now the problem is hidden and will be very difficult to find (more > so because the wait message also violates the convention of using a > short name that can be displayed by tools such as ps(1) and SIGINFO, > where the wait-channel display is currently likely to show as > "waitin"). > > I haven't looked at the code outside of the few lines shown in the > commit diff, but based on the names involved, I suspect the right fix > is to protect sc->running with a mutex and use msleep() instead of > trying to roll-your-own with atomics. That would certainly be true if > the wakeup code is some form of "if (!sc->running) wakeup(sc);" > > -- Ian > The code is a case where a missing wakeup does not matter. The thread is woken up by an allocation failure, which are themselves rate-limited to one attempt per second (since failures are expensive, and there is a less expensive fallback). So the worst thing that can happen is that we wait at most an extra second. Adding a mutex adds nothing except unneeded complexity. Drew