From nobody Tue May 21 22:07:12 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 4VkT7S5p3kz5Kgqm; Tue, 21 May 2024 22:07:16 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (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 4VkT7S54rYz4TQY; Tue, 21 May 2024 22:07:16 +0000 (UTC) (envelope-from bz@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1716329236; 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: in-reply-to:in-reply-to:references:references; bh=sK6lbZuu0HgRum+krzTvmP0g9e50k8IdaT1QMxB2V8g=; b=EE1Waeb5gqVRihToliT6Lcs/x3c133nQn74MWhXPfRsu3008vLzOFfFExPm1s9UPKerDhJ GKmKBpV9Gd0DzqbDlRQXTW73l1BwmtRZm96DlWqD2NQXE7/m3Y7646NUlMjEmdJ05xuM7X Q2WfDz9Kfkg/9fJ36rPDaW+abmD57c9q0S9DbAdoRdGYGVu1BW+UVMv67dBR0NIf55khx6 xSV/KStjQ6FYzTcwQgVkydCnWj48BWpYAMi/xCnVs08BTo8DkBNrSURzBm1x0ZQvDPCJGh edNNNX9OAylnSLrx+1/jTUjmAcJt93Ofd/xeMWsmxGankktnlnOfpF9v984E6w== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1716329236; a=rsa-sha256; cv=none; b=XrWmAvgppjpX4d5Aqh49wj8CIowAHR9laKnL/rRWD3RjATbEyhgfM0SzMM+kUFvDdMnAvP Rht1WDHDjLVH02Gf9Oh5SQkUYTg9QGyVTGTb1olBD3JcnjdPYJY3DCIXOIia+iHkCGNPbC mQsDNoJQOdd1OsN1mQx9ORSrrVR1rGbPOcTBh1UZJlZ9n05ktpbOc/hEwTYapU0x3dxB8L M0eoEEYjMCuQJX4dgyUlFxjmiLeE1fcuowkYaUhu37EaLeCuYvCgeYvKPxcIPPej/Is6mK oTSIyLmMwtP1e7l76gFh+FkmphIQWXwhtzAr2L+o5CNoka7t7TrG/FPoi3TvlA== 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=1716329236; 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: in-reply-to:in-reply-to:references:references; bh=sK6lbZuu0HgRum+krzTvmP0g9e50k8IdaT1QMxB2V8g=; b=pBdxEWmhkzIOHrRMpcOI0qS4xO8qwuKcdViLoBJAhg9UABv0VJ8pXa2kyopMtndqGZaeeD CtUB4KVHKnV2trQt7Us0sTOz5EK5wL89qao6/HOlQDygRGF2bZc4PRh2+vObejnMsW1eyI 7ZdFV22QWdDJ1o0vUYbzuf4O6dfASV0HGUzFCwc4M9XSIRUBfqfIhrDnt5vm+9LrLcnY2R 50PHM5hu+pN4DlZjd5bUWMMjeAFVVgwGkxxHKzIhibK1SWCkoEto5XaiKoRbgNBWHlyF0a Osrb6ile/mGH9pttSxq5M/uE757UaNEMS+XhiEDe/fP2AmzXDaeOckObkv+dUQ== Received: from mx1.sbone.de (cross.sbone.de [195.201.62.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.sbone.de", Issuer "SBone.DE Root Certificate Authority" (not verified)) (Authenticated sender: bz/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 4VkT7S3xH9zNLR; Tue, 21 May 2024 22:07:16 +0000 (UTC) (envelope-from bz@FreeBSD.org) Received: from mail.sbone.de (mail.sbone.de [IPv6:fde9:577b:c1a9:4902:0:7404:2:1025]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.sbone.de (Postfix) with ESMTPS id E56AE8D4A126; Tue, 21 May 2024 22:07:14 +0000 (UTC) Received: from content-filter.t4-02.sbone.de (content-filter.t4-02.sbone.de [IPv6:fde9:577b:c1a9:4902:0:7404:2:2742]) (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) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPS id 458EA2D029D8; Tue, 21 May 2024 22:07:14 +0000 (UTC) X-Virus-Scanned: amavisd-new at sbone.de Received: from mail.sbone.de ([IPv6:fde9:577b:c1a9:4902:0:7404:2:1025]) by content-filter.t4-02.sbone.de (content-filter.t4-02.sbone.de [IPv6:fde9:577b:c1a9:4902:0:7404:2:2742]) (amavisd-new, port 10024) with ESMTP id yd5-swuxlK9M; Tue, 21 May 2024 22:07:12 +0000 (UTC) Received: from strong-iwl0.sbone.de (strong-iwl0.sbone.de [IPv6:fde9:577b:c1a9:4902:b66b:fcff:fef3:e3d2]) (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) (No client certificate requested) by mail.sbone.de (Postfix) with ESMTPSA id B30012D029D2; Tue, 21 May 2024 22:07:12 +0000 (UTC) Date: Tue, 21 May 2024 22:07:12 +0000 (UTC) From: "Bjoern A. Zeeb" To: John Baldwin cc: Emmanuel Vadot , Emmanuel Vadot , src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: cff79fd02636 - main - linuxkpi: Fix spin_lock_init In-Reply-To: Message-ID: <30s1641n-04ps-4024-0797-45573737pqq6@SerrOFQ.bet> 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> X-OpenPGP-Key-Id: 0x14003F198FEFA3E77207EE8D2B58B8F83CCF1842 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 Content-Type: text/plain; charset=US-ASCII; format=flowed On Mon, 20 May 2024, John Baldwin wrote: > 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. Okay. Makes sense. In that case I think we should more than we do today mark the spin_lock_destroy(), the WTINESS enabled ones, etc. "FreeBSD specific" even in LinuxKPI (marking them with #ifdef or at least comments or calling them slightly differently). -- Bjoern A. Zeeb r15:7