From nobody Mon May 20 15:48:13 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Vjhmc5Shnz5LWcH; Mon, 20 May 2024 15:48:16 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Vjhmc51S2z4r40; Mon, 20 May 2024 15:48:16 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1716220096; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Wc0jJXdJRJCfOh+9e+YdO+LwyFl4mxarwcEKHs8BFIc=; b=EENyhJKQSePa5dY5B4khV+X6K4CGYAQGCXyRgSXT2llQ6JcLCRWwgLtJfHqkrjHj5Dtnx7 6U+RVvxLM6wxA7kmhbK/M+VNHgHwYiFfJ/cVZWwXZx2JRBDJwNYJwnFQjn6+zfras6Oejb Pet1eO0ydcpXPpLFunZNBCvc5ZHf5TI1GbkXPBnDnDOC6Tb6/zeQ+iAzl9+FfPN3cC/UoC VwUoq7374uXivjUYci+JbFkYtrKfDE7FPXG0Dg9JQdcuqLA2V2At8EzDXvNTKjGpDuYES4 4vvs2RZO5U6nHxu7rdi8TdaIfdwZQHaBzo4QcY7troZ13Bba7WVXYnpo0tzAuw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1716220096; a=rsa-sha256; cv=none; b=Zr+IM6LlA9iGG0TwJ8BpcrvY5cE597Uf+JTq712/SMmhU20xbK3wRDB5SZKpJk2kZD1rbB HOIChN3cg/y+ltJ5SA6rbWWPy53+o0LqK76WZ7nWYwCImyXnQ0FK/bIcAFq9fm5lW399dN YbvB0V0iy7UDAPhuWD66yh3S6ANKxuCKJEKNWeJnxgvrvQlrZiEGWVnqtCdI0TnbRWjE0m H8AK6w/DqqBRSRMNLpl7ZCvUxpvVRPn5Ec7xfOKDc95LN7w/s/xHoMzgy0qqV+7z127ZUs +wPO81e75Yv/NNaYap829RYRzQLRwNxnzywl3QmQdz7yPVkoqMAmq9Hw+PxH6g== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1716220096; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Wc0jJXdJRJCfOh+9e+YdO+LwyFl4mxarwcEKHs8BFIc=; b=VMuONNxD9nAkF8EGwvHvITQlKN1aZRloc3818HpFU3mZI2Lj06IdSZDTZaDlJO+NA5zkb+ nrIIsVpTnB5oGBRJxXwMhuBPXzgWpM1iNWqWEbC00sGug/afXO7pk4UBhnDahRnBIpB6RF XQPjr2VZYTUrrfek5h0uuOjCszPiekwBHrbCGFx02Pc63KZkIwXzwFmTXy2xoYizNPyM1N lvFBTw7yCxjmp8BMGruFuhLO1NvmsiVWvcXmpKWHE+T6kyH1YuLTdlLzs+0hMN6JrlhrM2 LsIq8lL3j+stMEg9761UpRUzA2ka70siptwDV7pWE+1ZFODGEc9at6QBqB4eaw== Received: from [IPV6:2601:644:937f:4c50:59bf:dc4e:16d3:ef1] (unknown [IPv6:2601:644:937f:4c50:59bf:dc4e:16d3:ef1]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4Vjhmb1hWNz1107; Mon, 20 May 2024 15:48:15 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: Date: Mon, 20 May 2024 08:48:13 -0700 List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: git: cff79fd02636 - main - linuxkpi: Fix spin_lock_init Content-Language: en-US To: "Bjoern A. Zeeb" Cc: Emmanuel Vadot , Emmanuel Vadot , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org References: <202405170559.44H5xD7d019861@gitrepo.freebsd.org> <2cd3e698-1b42-4e7f-93a0-aacccb55c8d6@FreeBSD.org> <20240517183350.e45f54df07f670980d5a51c3@bidouilliste.com> <0f2c4189-e259-4bd8-ad31-212a8df0e1b5@FreeBSD.org> <188o7q6o-n0rn-235r-76n4-779o555094r9@SerrOFQ.bet> From: John Baldwin In-Reply-To: <188o7q6o-n0rn-235r-76n4-779o555094r9@SerrOFQ.bet> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 5/17/24 2:36 PM, Bjoern A. Zeeb wrote: > On Fri, 17 May 2024, John Baldwin wrote: > >> On 5/17/24 9:33 AM, Emmanuel Vadot wrote: >>> On Fri, 17 May 2024 09:07:53 -0700 >>> John Baldwin wrote: >>> >>>> On 5/16/24 10:59 PM, Emmanuel Vadot wrote: >>>>> The branch main has been updated by manu: >>>>> >>>>> URL: >>>>> https://cgit.FreeBSD.org/src/commit/?id=cff79fd02636f34010d8b835cc9e55401fa76e74 >>>>> >>>>> commit cff79fd02636f34010d8b835cc9e55401fa76e74 >>>>> Author: Emmanuel Vadot >>>>> AuthorDate: 2024-05-17 04:52:53 +0000 >>>>> Commit: Emmanuel Vadot >>>>> CommitDate: 2024-05-17 05:58:59 +0000 >>>>> >>>>> linuxkpi: Fix spin_lock_init >>>>> Some linux code re-init some spinlock so add MTX_NEW to >>>>> mtx_init. >>>>> Reported by: David Wolfskill >>>>> Fixes: ae38a1a1bfdf ("linuxkpi: spinlock: Simplify code") >>>>> --- >>>>> sys/compat/linuxkpi/common/include/linux/spinlock.h | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>> b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>> index 3f6eb4bb70f6..2992e41c9c02 100644 >>>>> --- a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>> +++ b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>> @@ -140,7 +140,7 @@ typedef struct mtx spinlock_t; >>>>> #define spin_lock_name(name) _spin_lock_name(name, >>>>> __FILE__, __LINE__) >>>>> #define spin_lock_init(lock) mtx_init(lock, >>>>> spin_lock_name("lnxspin"), \ >>>>> - NULL, MTX_DEF | MTX_NOWITNESS) >>>>> + NULL, MTX_DEF | MTX_NOWITNESS | MTX_NEW) >>>>> #define spin_lock_destroy(_l) mtx_destroy(_l) >>>> >>>> This is only ok because of MTX_NOWITNESS. Reiniting locks without >>>> destroying >>>> them corrupts the internal linked lists in WITNESS for locks using >>>> witness. >>>> That may warrant a comment here explaining why we disable witness. >>> >>> I'll try to look at what linux expect for spinlocks, it could also be >>> that we need to do this because some drivers via linuxkpi does weird >>> things ... >>> >>>> It might be nice to add an extension to the various lock inits for code >>>> that >>>> wants to opt-int to using WITNESS where a name can be passed. Using those >>>> would >>>> be relatively small diffs in the client code and let select locks opt into >>>> using WITNESS. You could make it work by adding an optional second >>>> argument >>>> to spin_lock_init, etc. that takes the name. >>> >>> We can't change spin_lock_init, we need to follow linux api here. >> >> You can use macro magic to add support for an optional second argument. >> >> #define _spin_lock_init2(lock, name) mtx_init(lock, name, NULL, MTX_DEF) >> >> #define _spin_lock_init1(lock) mtx_init(lock, spin_lock_name("lnxspin"), ...) >> >> #define _spin_lock_init_macro(lock, name, NAME, ...) >> NAME >> >> #define spin_lock_init(...) \ >> _spin_lock_init_macro(__VA_ARGS__, _spin_lock_init2, >> _spin_lock_init1)(__VA_ARGS__) >> >> Then you can choose to specifically annotate certain locks with a name >> instead in which case they will use WITNESS. > > I think the real confusing here comes from the fact that FreeBSD has > spin_lock_destroy in LinuxKPI which I believe is a local addition > (though not documented as such) for semi-native drivers using parts of > LinuxKPI and in order to use spinlocks according to "FreeBSD > expectations". > > I believe (and still hope someone would correct me) that Linux has not > functions to destroy locks like we do? I believe for rwlocks I had left > that remark on the review: > https://reviews.freebsd.org/D45206#1031316 > > So if you use WITNESS anywhere you could only really do so for > "internal" parts but nowhere in Linux driver code as that will likely > simply break assumptions? You could only do it safely if you added local modifications to the driver, yes. My understanding is that while we aim for as little modifications to drivers as possible to permit merging in updates, we don't mandate absolutely zero local changes. The approach I described above would permit selectively using WITNESS at the expense of local diffs that would have to be maintained across updates. -- John Baldwin