From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4C4C6C636D7 for ; Tue, 21 Feb 2023 14:59:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 812396B0078; Tue, 21 Feb 2023 09:59:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 7C1F26B007B; Tue, 21 Feb 2023 09:59:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 663A06B007D; Tue, 21 Feb 2023 09:59:45 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 576A36B0078 for ; Tue, 21 Feb 2023 09:59:45 -0500 (EST) Received: from smtpin17.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id F04E516055D for ; Tue, 21 Feb 2023 14:59:44 +0000 (UTC) X-FDA: 80491608288.17.132F407 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf09.hostedemail.com (Postfix) with ESMTP id 96DED140008 for ; Tue, 21 Feb 2023 14:59:42 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Fg3MHoU+; spf=pass (imf09.hostedemail.com: domain of dakr@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=dakr@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1676991582; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zR01m6f0KMxaCdW6/9YON73JqwNOZ58NWx14UlZixkQ=; b=EFlKT1xQ5P9PLpFS0ktDE410FplYbbm0/VJrhifqLYxh7ft2SNWaqJ6WO2kflb1xl3VUre ZUVumU7wOVJPq8pC8cHFsqPJV83d5LuYT3tv6GOhJ+39Sc58orM6whsvMtsAVoE8OO2sio ySOLuZeAypFv11jrxjlGM7/oe7ui6Ek= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Fg3MHoU+; spf=pass (imf09.hostedemail.com: domain of dakr@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=dakr@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1676991582; a=rsa-sha256; cv=none; b=oIKok3xMJ1QhhJ+Fipu4khnFLSMPM5BnA4dlgMzNSooPnxMwwA6YTYjulvug832i3d8Rca LvMLvYzauahFdZMXiFfTOR5QDAgUoKKHvV/6Ns+3Rx5HTLCR1umpw0qQgdvl8Qits+8S+r odntsLtqceh31SOwpBkzH1lGTAipMws= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676991581; 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=zR01m6f0KMxaCdW6/9YON73JqwNOZ58NWx14UlZixkQ=; b=Fg3MHoU+lHyQmBOd2njFZCnYCJD0K+S2jZOAYOQWFj1TCoez7lV/7g1yFwPluBZ1u+ru8H T4OP2zQKoHnTvyyJdgIisZpf13pLSsUJ2hzibWJP9Eyrha6f84M4Wz8rkfx+jp53x+QwvZ 3EzeMIfgqDXw+/+WtKnE9O/r8lyj4gQ= Received: from mail-ed1-f71.google.com (mail-ed1-f71.google.com [209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-650-crLM7pvYMTmj4LWyA3GQSg-1; Tue, 21 Feb 2023 09:59:40 -0500 X-MC-Unique: crLM7pvYMTmj4LWyA3GQSg-1 Received: by mail-ed1-f71.google.com with SMTP id g24-20020a056402321800b004ace77022ebso6436392eda.8 for ; Tue, 21 Feb 2023 06:59:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=zR01m6f0KMxaCdW6/9YON73JqwNOZ58NWx14UlZixkQ=; b=H7p1tEhz2Xl2SeVYX3VpkLePsUtRghUMXM15hWkC6k1j4SfQ1OzwGhz21NjrHA3T6X tnxHCBSRuEF3u/WnHvTbhkJtCx72bVTkKwc9kaQJNZjSqD/0QbGVLdSMUb5zRo+6NyRV T2q7OdLitoV0eBudQnsizW6vDrRNq7oh1BUY7CZSsh+4j6pyGNYDmoGN5vPDEtlbJqM+ iWNJGnyjH8a209pYsxl/gBWlovGe33r2UnIM/W1tfv8vUVvFfspNi57xwn97EiQQ4Pbp rtAHlB08BEgS3/nUNAbfpvxuLWvMLi4ewEzA415x4FdfMQrD3trk4CCK3iIaZOc1kBiE SaTA== X-Gm-Message-State: AO0yUKXy3cEn7Xrav035pWkzMjLDc2jzFoOduTf4cmNpKM/P/77emlHm M+TjyW7rA+T1SoWOxD//P59bsN1IM1aOsOHE851OB8JxSLFAFO6xO4rc3N1l2lTWDOrXX3Bp5F5 UB4lrY1eTdtQ= X-Received: by 2002:a17:906:f298:b0:877:8ae7:2e44 with SMTP id gu24-20020a170906f29800b008778ae72e44mr10449579ejb.5.1676991579520; Tue, 21 Feb 2023 06:59:39 -0800 (PST) X-Google-Smtp-Source: AK7set/rd+60WkR9Pb5ZCfi4/q+0aDmwntl5Cr4CAeiqkkwHXnHhiHRgUbf3dBpXyGaefcHcmhtyMQ== X-Received: by 2002:a17:906:f298:b0:877:8ae7:2e44 with SMTP id gu24-20020a170906f29800b008778ae72e44mr10449559ejb.5.1676991579247; Tue, 21 Feb 2023 06:59:39 -0800 (PST) Received: from pollux ([2a02:810d:4b3f:de78:642:1aff:fe31:a15c]) by smtp.gmail.com with ESMTPSA id dt23-20020a170906b79700b00889db195470sm7194952ejb.82.2023.02.21.06.59.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Feb 2023 06:59:38 -0800 (PST) Date: Tue, 21 Feb 2023 15:37:49 +0100 From: Danilo Krummrich To: Matthew Wilcox Cc: matthew.brost@intel.com, dri-devel@lists.freedesktop.org, corbet@lwn.net, nouveau@lists.freedesktop.org, ogabbay@kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, boris.brezillon@collabora.com, bskeggs@redhat.com, tzimmermann@suse.de, Liam.Howlett@oracle.com, bagasdotme@gmail.com, christian.koenig@amd.com, jason@jlekstrand.net Subject: Re: [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE Message-ID: References: <20230217134422.14116-1-dakr@redhat.com> <20230217134422.14116-5-dakr@redhat.com> <3bb02ec3-4d19-9135-cabc-26ed210f7396@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: 99ob1iyggbofw43dfgh5p9n7t3m9obz6 X-Rspamd-Queue-Id: 96DED140008 X-HE-Tag: 1676991582-52398 X-HE-Meta: U2FsdGVkX18AddoqQMTH+tkHbigcYzFTbG90kiW+ge5ygAQYMimZO6hmPfo4HHV6I3XwkejGcNoZQjkqW7gIAM0W2moK8wSYd9gnCqis/mvM8PoKEBLGNc/RFEdjQ1hg2XE4953bMIVTpnoa0YPGfPVZvx7SNKuJwErIxXZe4g4rF/jtIOdRuF0i1f0LXi1t4A2a5KmPfzM31+ajX80Bjqf9OJTdPyM1Jn8SYzP5vxDAeKorZrrgLiCrFf48Kjy1V/lGpJweo9sOYu2/iX1DCZCvlLh0et8iIfpC+D7sQu0mWLwkW1aKTGEktomtq6vkxf4jXTJvkeb4fEllKC3fgOMCwduq6LFmuTagAPG33g9AOhHZ7I5vtLIJ24p87zAaiDAtCke3QaKHn5IbpgJCDTO14TmyRBG5Vfe03/wxpkXUxXVE5Z2bmY77UNU90OJ9nDtaSAaM1/B/WDiOFIYoAZgPLWVwIYETjCwPmodXOtb7XoHS8Cfb6z7H6IxoxXVPj+4+BN0dKl5GKtnWqmz6EWl2mlZpmGHZRWkdTBreooGi1imqIs6e31AVVoEmT5CVL7bG52IUGXLe1N4ulZrY/6PsU3wIRfOPNdMG0CHTuIWNTra/Q/7oB5cdKGOuO5Wm2c4Ud5fYNeEpns3XOlRhmHT+3QvhFdPlCtd+r0yixD7WE5s4UqffpA4sjg4DJNzMB9LxjxNTsOyMxUDfu6wlKj7Ody6vVwlEfYzyOC6DZ7koUdAS94uQNJIz+0BMQWqDJAmRGvBxLHarWQfRhjJ4cgz5776F2snonzOMcXYqZ6lVpIR7z2/XTerkP0cXdY3t95UICSW365MR2rhXvKKFNo0O0DN5ryF9l5fggtszY4BBmUfswLuOCtqSCSG0wLGmeuHQXnsECekHlP0HDFi3OmBLZ7d4cE+cOVXCjJNHTywPkjaKYe5Xb7Lm+Ldegx6oLWDQ1P+nWyiWWsFl4S2 FOnRpPnS J6Wg7B/TxaNbjVI837IUKKF1OSqMHJbxdqVZciKQxnXRtdWi6eiQUKbjqMJ/eFXnwdKJ3NWjL2p6CJ4lLAVMMU7UKe5MguUuZMbzAX+DFch27idh4UOSSgv9ij48t7WPeHmbMx5hkV0IbpW8YOiJWB0QykiWcsfDeokKwk6c9uXVs0ZTQBgwk/v1m060w3r4vi+37//Lx0CakcsynzjISchBL+jccSikwZ5zGzMWKf84BGTAQBvUBo4RD+npEZGTDCC2WLghi3pJaSpl2wB6iFZ4BL6iC6JJqcx3UZJ5+FWLgNr10MF4VdeG227wEvjDOpbhO2XIlVVo5c/N3ZxgQzfEbCtdgrufBTuvn3FwmE8QCpZ+7gfSJB+mr0Vsjm/Ayh25FDbhHzObd5fSCNRI/NfMFyQUhuT1Fy4Jq X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Feb 20, 2023 at 08:33:35PM +0000, Matthew Wilcox wrote: > On Mon, Feb 20, 2023 at 06:06:03PM +0100, Danilo Krummrich wrote: > > On 2/20/23 16:10, Matthew Wilcox wrote: > > > This is why we like people to use the spinlock embedded in the tree. > > > There's nothing for the user to care about. If the access really is > > > serialised, acquiring/releasing the uncontended spinlock is a minimal > > > cost compared to all the other things that will happen while modifying > > > the tree. > > > > I think as for the users of the GPUVA manager we'd have two cases: > > > > 1) Accesses to the manager (and hence the tree) are serialized, no lock > > needed. > > > > 2) Multiple operations on the tree must be locked in order to make them > > appear atomic. > > Could you give an example here of what you'd like to do? Ideally > something complicated so I don't say "Oh, you can just do this" when > there's a more complex example for which "this" won't work. I'm sure > that's embedded somewhere in the next 20-odd patches, but it's probably > quicker for you to describe in terms of tree operations that have to > appear atomic than for me to try to figure it out. > Absolutely, not gonna ask you to read all of that. :-) One thing the GPUVA manager does is to provide drivers the (sub-)operations that need to be processed in order to fulfill a map or unmap request from userspace. For instance, when userspace asks the driver to map some memory the GPUVA manager calculates which existing mappings must be removed, split up or can be merged with the newly requested mapping. A driver has two ways to fetch those operations from the GPUVA manager. It can either obtain a list of operations or receive a callback for each operation generated by the GPUVA manager. In both cases the GPUVA manager walks the maple tree, which keeps track of existing mappings, for the given range in __drm_gpuva_sm_map() (only considering the map case, since the unmap case is a subset basically). For each mapping found in the given range the driver, as mentioned, either receives a callback or a list entry is added to the list of operations. Typically, for each operation / callback one entry within the maple tree is removed and, optionally at the beginning and end of a new mapping's range, a new entry is inserted. An of course, as the last operation, there is the new mapping itself to insert. The GPUVA manager delegates locking responsibility to the drivers. Typically, a driver either serializes access to the VA space managed by the GPUVA manager (no lock needed) or need to lock the processing of a full set of operations generated by the GPUVA manager. > > In either case the embedded spinlock wouldn't be useful, we'd either need an > > external lock or no lock at all. > > > > If there are any internal reasons why specific tree operations must be > > mutually excluded (such as those you explain below), wouldn't it make more > > sense to always have the internal lock and, optionally, allow users to > > specify an external lock additionally? > > So the way this works for the XArray, which is a little older than the > Maple tree, is that we always use the internal spinlock for > modifications (possibly BH or IRQ safe), and if someone wants to > use an external mutex to make some callers atomic with respect to each > other, they're free to do so. In that case, the XArray doesn't check > the user's external locking at all, because it really can't know. > > I'd advise taking that approach; if there's really no way to use the > internal spinlock to make your complicated updates appear atomic > then just let the maple tree use its internal spinlock, and you can > also use your external mutex however you like. > That sounds like the right thing to do. However, I'm using the advanced API of the maple tree (and that's the reason why the above example appears a little more detailed than needed) because I think with the normal API I can't insert / remove tree entries while walking the tree, right? As by the documentation the advanced API, however, doesn't take care of locking itself, hence just letting the maple tree use its internal spinlock doesn't really work - I need to take care of that myself, right? It feels a bit weird that I, as a user of the API, would need to lock certain (or all?) mas_*() functions with the internal spinlock in order to protect (future) internal features of the tree, such as the slab cache defragmentation you mentioned. Because from my perspective, as the generic component that tells it's users (the drivers) to take care of locking VA space operations (and hence tree operations) I don't have an own purpose of this internal spinlock, right? Also I'm a little confused how I'd know where to take the spinlock? E.g. for inserting entries in the tree I use mas_store_gfp() with GFP_KERNEL.