From nobody Tue Jan 18 22:47:43 2022 X-Original-To: dev-commits-src-all@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 ED397195B076; Tue, 18 Jan 2022 22:47:45 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) (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-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JdkSK4pyXz3FlW; Tue, 18 Jan 2022 22:47:45 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-lf1-x130.google.com with SMTP id p27so1505508lfa.1; Tue, 18 Jan 2022 14:47:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4nd0dz0ts9KJB4pBmQNY7LRqAkhMU1NLZvpuTneBYAs=; b=avlbVpko9o0ZeZF1Ov9J1KQP8EzdfL8jU2CIvNwPPbLWmS7Yx6D74gxJTpefNv+8rR kDDcyAfnGzzOg0ZwFRYh/d80Gi/SsIAsxsJNbJViCJPuhYqJwvrss9HtL6vmgpfpGhYJ Ql7EVVJYYV1oSff69tDrPQN6kDxKL3H/YcWMWdq93IO9T16VNLaO1st95HH1dakuo8hl 93LhZYk5lqe+DeBLdeJoqAR2qllZM588KAk/6Sfom+3CyuU4us30APifPqLD/5/1lyib xwYwK7g4WBPR0mlGWhJ4te0qk75vw4iK2bfSq6aCbB5AeOXgq4QJr3Ea+oJkWJcTx3AS UQGA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4nd0dz0ts9KJB4pBmQNY7LRqAkhMU1NLZvpuTneBYAs=; b=soqH1AQtjXqgyJZsdSv61ov3nRdkpftBRpZY3k34MOl/MZHJLk1VA4tfr32Kk0sBMY 1KXqCvwUtB+3Q/mQWYRMbIOkWPlHJ3pjl0DZ84XmiV7Nvu9xwONl9JcUw/uExsqyTpyF eKE0/JCRVCrKGr0dKDNZiXvOGJIJb+Vz9Z7lCXL6S61XdZpCUpw2cq3Wxr7vAAvPbkrO 35Sd3DFwCO/c0sXJ4eY5OdfKjERuHEb6o9cOiQnjtry+Rjo0soYMbPUVc+cUz3LF+EyN Si1BG72eEsOgK0Dy/+avqW4autAi/rIU0dY2e/1rD91S/GHk33DeL7I6fV+slHFOGW8/ s58A== X-Gm-Message-State: AOAM530ty4lycy31UXEIFOGERpG9Sar4mJe4WGJzVKr8CRWnnlXCHWXz A50Verlr9a/5eIOjanRSUM+r/6a7OxwS6FvNqi0= X-Google-Smtp-Source: ABdhPJwdQp9NCrgsDatc9KYBSwJxvJhzV9TvFR1AC11qUA13dY3HpMwDP5u8vq2iduKetS28T7gESf7k3XoKW7098cM= X-Received: by 2002:a05:6512:104f:: with SMTP id c15mr24536843lfb.280.1642546064216; Tue, 18 Jan 2022 14:47:44 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Received: by 2002:aa6:db8e:0:b0:16e:b0b:e03e with HTTP; Tue, 18 Jan 2022 14:47:43 -0800 (PST) In-Reply-To: References: <202201182015.20IKFaWL053942@gitrepo.freebsd.org> <540a6a93-3101-02e8-b86a-50caa19f9653@kondratyev.su> From: Mateusz Guzik Date: Tue, 18 Jan 2022 23:47:43 +0100 Message-ID: Subject: Re: git: 02ea6033020e - main - LinuxKPI: Allow spin_lock_irqsave to be called within a critical section To: Vladimir Kondratyev Cc: Konstantin Belousov , Vladimir Kondratyev , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4JdkSK4pyXz3FlW X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N On 1/18/22, Vladimir Kondratyev wrote: > On 19.01.2022 01:08, Konstantin Belousov wrote: >> On Wed, Jan 19, 2022 at 01:01:45AM +0300, Vladimir Kondratyev wrote: >>> On 19.01.2022 00:48, Konstantin Belousov wrote: >>>> On Wed, Jan 19, 2022 at 12:35:41AM +0300, Vladimir Kondratyev wrote: >>>>> On 18.01.2022 23:22, Konstantin Belousov wrote: >>>>>> On Tue, Jan 18, 2022 at 08:15:36PM +0000, Vladimir Kondratyev wrote: >>>>>>> The branch main has been updated by wulf: >>>>>>> >>>>>>> URL: >>>>>>> https://cgit.FreeBSD.org/src/commit/?id=02ea6033020e11afec6472bf560b0ddebd0fa97a >>>>>>> >>>>>>> commit 02ea6033020e11afec6472bf560b0ddebd0fa97a >>>>>>> Author: Vladimir Kondratyev >>>>>>> AuthorDate: 2022-01-18 20:14:12 +0000 >>>>>>> Commit: Vladimir Kondratyev >>>>>>> CommitDate: 2022-01-18 20:14:12 +0000 >>>>>>> >>>>>>> LinuxKPI: Allow spin_lock_irqsave to be called within a >>>>>>> critical section >>>>>>> with spinning on spin_trylock. dma-buf part of drm-kmod >>>>>>> depends on this >>>>>>> property and absence of it support results in "mi_switch: >>>>>>> switch in a >>>>>>> critical section" assertions [1][2]. >>>>>>> [1] https://github.com/freebsd/drm-kmod/issues/116 >>>>>>> [2] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=261166 >>>>>>> MFC after: 1 week >>>>>>> Reviewed by: manu >>>>>>> Differential Revision: https://reviews.freebsd.org/D33887 >>>>>>> --- >>>>>>> .../linuxkpi/common/include/linux/spinlock.h | 27 >>>>>>> ++++++++++++++++++---- >>>>>>> 1 file changed, 23 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> index a87cb7180b28..31d47fa73986 100644 >>>>>>> --- a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> +++ b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> @@ -37,6 +37,7 @@ >>>>>>> #include >>>>>>> #include >>>>>>> #include >>>>>>> +#include >>>>>>> #include >>>>>>> #include >>>>>>> @@ -117,14 +118,32 @@ typedef struct { >>>>>>> local_bh_disable(); \ >>>>>>> } while (0) >>>>>>> -#define spin_lock_irqsave(_l, flags) do { \ >>>>>>> - (flags) = 0; \ >>>>>>> - spin_lock(_l); \ >>>>>>> +#define __spin_trylock_nested(_l, _n) ({ \ >>>>>>> + int __ret; \ >>>>>>> + if (SPIN_SKIP()) { \ >>>>>>> + __ret = 1; \ >>>>>>> + } else { \ >>>>>>> + __ret = mtx_trylock_flags(&(_l)->m, MTX_DUPOK); \ >>>>>>> + if (likely(__ret != 0)) \ >>>>>>> + local_bh_disable(); \ >>>>>>> + } \ >>>>>>> + __ret; \ >>>>>>> +}) >>>>>>> + >>>>>>> +#define spin_lock_irqsave(_l, flags) do { \ >>>>>>> + (flags) = 0; \ >>>>>>> + if (unlikely(curthread->td_critnest != 0)) \ >>>>>>> + while (!spin_trylock(_l)) {} \ >>>>>>> + else \ >>>>>>> + spin_lock(_l); \ >>>>>>> } while (0) >>>>>>> #define spin_lock_irqsave_nested(_l, flags, _n) do { \ >>>>>>> (flags) = 0; \ >>>>>>> - spin_lock_nested(_l, _n); \ >>>>>>> + if (unlikely(curthread->td_critnest != 0)) \ >>>>>>> + while (!__spin_trylock_nested(_l, _n)) {} \ >>>>>>> + else \ >>>>>>> + spin_lock_nested(_l, _n); \ >>>>>>> } while (0) >>>>>>> #define spin_unlock_irqrestore(_l, flags) do { \ >>>>>> You are spin-waiting for blockable mutex, am I right? >>>>> >>>>> Both, yes and no. On Linux spin_lock_irqsave is generally unblockable >>>>> as it >>>>> disables preemption and interrupts while our version does not do this >>>>> as >>>>> LinuxKPI is not ready for such a tricks. >>>>> It seems that we should explicitly add critical_enter()/critical_exit >>>>> calls >>>>> to related dma-buf parts to make it unblockable too. >>>> LinuxKPI does +1 to the level of locks comparing with Linux, so their >>>> spinlocks >>>> become our blockable mutexes. >>>> >>>> Can you please explain why dmabufs need critical section? What is >>>> achieved there by disabled preemption? >>>> >>> >>> dma-buf uses sequence locks for synchronization. If seqlock is taken for >>> write, than thread it holding enters in to critical section to not force >>> readers to spin if writer is preempted. Unfortunately, dma-buf writers >>> execute callbacks which requires locks and spin_lock_irqsave is used for >>> synchronize these callbacks. >> >> Then, it seems that locking should be changed either to rwlocks or >> rmlocks, >> not sure which. > > That can introduce LORs as seqlock's readers never block writers. It is > probably > easier to skip critical section at all at the cost of extra CPU cycles spent > on > reader spinning. > The reader side could try to get a stable snapshot just once, and failing that, take the lock to get the data. Then you are set. -- Mateusz Guzik