Date: Thu, 5 Aug 2021 13:46:25 -0400 From: Andrew Gallatin <gallatin@cs.duke.edu> To: Ian Lepore <ian@freebsd.org>, Andrew Gallatin <gallatin@FreeBSD.org>, src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 2694c869ff9f - main - ktls: fix a panic with INVARIANTS Message-ID: <69995f5d-9228-f956-aef5-d3cd4a4caa3d@cs.duke.edu> In-Reply-To: <1013cb35c9a2c3cc55b4f3bd10d14b1540c9462f.camel@freebsd.org> References: <202108051710.175HAP1D031061@gitrepo.freebsd.org> <1013cb35c9a2c3cc55b4f3bd10d14b1540c9462f.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?69995f5d-9228-f956-aef5-d3cd4a4caa3d>