Received: by 2002:a05:7412:3784:b0:e2:908c:2ebd with SMTP id jk4csp1994565rdb; Tue, 3 Oct 2023 07:22:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFA603abAkZWjB4F8YkQeKAZ+099rkPPY/61IasoBMBecjuH/zXkbUzSsNwqaBw8F3VF+gY X-Received: by 2002:a17:902:d2ca:b0:1c6:de6:5eb4 with SMTP id n10-20020a170902d2ca00b001c60de65eb4mr4683567plc.13.1696342933109; Tue, 03 Oct 2023 07:22:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696342933; cv=none; d=google.com; s=arc-20160816; b=RlKF6RGdPmzjLHSx57fWDoj+M4pMKNWxZLt+7rYdnUzN8Sw5vpvZ61/Gynk9Yh6zs3 r7nV394yw8zZNSJbOGkAfBz3cmPb1Rzg823++M05I1te9J4YWz5Y2eu5xmnuEjXa9UTA TRjuirX0gRGILQ4grsgFhe1yt81v3zHcDBNuP9Q+ftDNJpocQg8cHG8732iPnFdlzC0C 6qBYBLmQAyHcGeOH6OC6NdF7qsF5HnOGEDY0dKknR/CYwjQDPjobjtkAzJI/jqPKh841 a/oymIQNLsJ29xdd29ydnNhuh6lqUTFJ57vYtux77E5ooHCjdmwrSshVXtD+snLozPD8 KWvw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :date:dkim-signature; bh=vQsEqcx27Cx2pqsisOvj5U0vFcuJ4TRd6QOxjrgsi8c=; fh=IsvNflLAOTOdxBwCPG0gJzLIfvv1/vZsMZJ1RkrV/Co=; b=i996D5v67n7gxGo9y1nY62UqfnQRiRQqnyY9f+P/Q1qs4I4wuAyjlZApx3bhenTP3z MVhbFGkYWUhPwPPX6TZ8wvKgutXsgUHpsRkJXhmlM2ApOHcWgBC3Ja6+Eg3GFgjVeiK9 YDDqM8oAwbzLpTzKRx/hcZn6IWlnrvHqNyaARTh5BPLQntK9ZaNHNm80zIuukucO5y+g rI98f/wDwx5iJhbriHruoakcArnySSSJEssh6Ltrp97EkX2frTXl+ikq5O9CbIwTq92e gAfoUX/O1APZfgxIi72Y+B+dzxYhNv1/7YJIK6DqKiRuYR5rPtd6rDc5z2qjqUuEguQJ RwZQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=KYHtfHXn; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Return-Path: Received: from howler.vger.email (howler.vger.email. [23.128.96.34]) by mx.google.com with ESMTPS id h11-20020a65480b000000b005655e87c8aasi1490433pgs.192.2023.10.03.07.22.12 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Oct 2023 07:22:13 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=KYHtfHXn; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by howler.vger.email (Postfix) with ESMTP id D0F2A8283A59; Tue, 3 Oct 2023 07:22:11 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at howler.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239645AbjJCOV7 (ORCPT + 99 others); Tue, 3 Oct 2023 10:21:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52916 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239048AbjJCOV6 (ORCPT ); Tue, 3 Oct 2023 10:21:58 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 20D37115 for ; Tue, 3 Oct 2023 07:21:47 -0700 (PDT) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (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) (Authenticated sender: bbrezillon) by madras.collabora.co.uk (Postfix) with ESMTPSA id E645A6602173; Tue, 3 Oct 2023 15:21:45 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1696342906; bh=bbwykdAiuIs9XwhdXrg3tGyfYHFQW7o1p4869nxxDKc=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=KYHtfHXn+ikjp8zYsWxIA5WcBTOFiiphjRuRj8exzQUBrnFylO6qvui2An27jalxX 7uC0vyGkVHjDpB/gqe/v5UtsTPjYkA6Dq0yrj58jgfK/HN+VU82Q4qbyS+C1okaLbv V3pnuHvgQkRHAejyNJjd9HEThu14B8SyMRwtXQqsczEyrQ/IObm5/+4q/WP9i1FG3d YJ++m1kK3EfBncSoCoj/B0AU47XeLLQXuIKNLPettyIIGEhBk5ItviPQTsuDoOGQB+ RmDXw9gachcLZd0audALZtbb9hOK+KHJ4PNWkF/Y9bzK77d0GlGnke3az/H5ejWBSr ynufj4GX9l6rA== Date: Tue, 3 Oct 2023 16:21:43 +0200 From: Boris Brezillon To: Thomas =?UTF-8?B?SGVsbHN0csO2bQ==?= Cc: Danilo Krummrich , airlied@gmail.com, daniel@ffwll.ch, matthew.brost@intel.com, sarah.walker@imgtec.com, donald.robson@imgtec.com, christian.koenig@amd.com, faith@gfxstrand.net, dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH drm-misc-next v5 4/6] drm/gpuvm: track/lock/validate external/evicted objects Message-ID: <20231003162143.490e3ef0@collabora.com> In-Reply-To: References: <20230928191624.13703-1-dakr@redhat.com> <20230928191624.13703-5-dakr@redhat.com> <20231003120554.547090bc@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 03 Oct 2023 07:22:12 -0700 (PDT) On Tue, 03 Oct 2023 14:25:56 +0200 Thomas Hellstr=C3=B6m wrote: > > > > +/** > > > > + * get_next_vm_bo_from_list() - get the next vm_bo element > > > > + * @__gpuvm: The GPU VM > > > > + * @__list_name: The name of the list we're iterating on > > > > + * @__local_list: A pointer to the local list used to store > > > > already iterated items > > > > + * @__prev_vm_bo: The previous element we got from > > > > drm_gpuvm_get_next_cached_vm_bo() > > > > + * > > > > + * This helper is here to provide lockless list iteration. > > > > Lockless as in, the > > > > + * iterator releases the lock immediately after picking the > > > > first element from > > > > + * the list, so list insertion deletion can happen concurrently. > > > > + * > > > > + * Elements popped from the original list are kept in a local > > > > list, so removal > > > > + * and is_empty checks can still happen while we're iterating > > > > the list. > > > > + */ > > > > +#define get_next_vm_bo_from_list(__gpuvm, __list_name, > > > > __local_list, __prev_vm_bo)=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0({=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0struct drm_gpuvm_bo *__vm_bo =3D > > > > NULL;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0drm_gpuvm_bo_put(__prev_vm_bo);=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0spin_lock(&(__gpuvm)- =20 > > > > >__list_name.lock);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\=C2= =A0 =20 > > >=20 > > > Here we unconditionally take the spinlocks while iterating, and the > > > main=20 > > > point of DRM_GPUVM_RESV_PROTECTED was really to avoid that? > > >=20 > > > =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0if (!(__gpuvm)- =20 > > > > >__list_name.local_list)=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0\ =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0(__= gpuvm)->__list_name.local_list =3D > > > > __local_list;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0else=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0WAR= N_ON((__gpuvm)->__list_name.local_list > > > > !=3D __local_list);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0while (!list_empty(&(__gpuvm)->__list_name.list)) > > > > {=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0__v= m_bo =3D list_first_entry(&(__gpuvm)- =20 > > > > >__list_name.list,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\= =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 struct > > > > drm_gpuvm_bo,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0 > > > > list.entry.__list_name);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if = (kref_get_unless_zero(&__vm_bo->kref)) > > > > {=C2=A0 =20 > > > And unnecessarily grab a reference in the RESV_PROTECTED case. =20 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_move_tail(&(__vm_bo)- =20 > > > > >list.entry.__list_name,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > __local_list);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0break;=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0} e= lse > > > > {=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0list_del_init(&(__vm_bo)- =20 > > > > >list.entry.__list_name);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0__vm_bo =3D > > > > NULL;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0}=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0spin_unlock(&(__gpuvm)- =20 > > > > >__list_name.lock);=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ =20 > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0__vm_bo;=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0})=C2=A0 =20 > > >=20 > > > IMHO this lockless list iteration looks very complex and should be=20 > > > pretty difficult to maintain while moving forward, also since it > > > pulls=20 > > > the gpuvm_bos off the list, list iteration needs to be protected by > > > an=20 > > > outer lock anyway. =20 > >=20 > > As being partly responsible for this convoluted list iterator, I must > > say I agree with you. There's so many ways this can go wrong if the > > user doesn't call it the right way, or doesn't protect concurrent > > list > > iterations with a separate lock (luckily, this is a private > > iterator). I > > mean, it works, so there's certainly a way to get it right, but gosh, > > this is so far from the simple API I had hoped for. > > =20 > > > Also from what I understand from Boris, the extobj=20 > > > list would typically not need the fine-grained locking; only the > > > evict=20 > > > list? =20 > >=20 > > Right, I'm adding the gpuvm_bo to extobj list in the ioctl path, when > > the GEM and VM resvs are held, and I'm deferring the > > drm_gpuvm_bo_put() > > call to a work that's not in the dma-signalling path. This being > > said, > > I'm still not comfortable with the > >=20 > > gem =3D drm_gem_object_get(vm_bo->gem); > > dma_resv_lock(gem->resv); > > drm_gpuvm_bo_put(vm_bo); > > dma_resv_unlock(gem->resv); > > drm_gem_object_put(gem); > >=20 > > dance that's needed to avoid a UAF when the gpuvm_bo is the last GEM > > owner, not to mention that drm_gpuva_unlink() calls > > drm_gpuvm_bo_put() > > after making sure the GEM gpuvm_list lock is held, but this lock > > might > > differ from the resv lock (custom locking so we can call > > gpuvm_unlink() in the dma-signalling path). So we now have paths > > where > > drm_gpuvm_bo_put() are called with the resv lock held, and others > > where > > they are not, and that only works because we're relying on the the > > fact > > those drm_gpuvm_bo_put() calls won't make the refcount drop to zero, > > because the deferred vm_bo_put() work still owns a vm_bo ref. =20 >=20 > I'm not sure I follow to 100% here, but in the code snippet above it's > pretty clear to me that it needs to hold an explicit gem object > reference when calling dma_resv_unlock(gem->resv). Each time you copy a > referenced pointer (here from vm_bo->gem to gem) you need to up the > refcount unless you make sure (by locks or other means) that the source > of the copy has a strong refcount and stays alive, so that's no weird > action to me. Could possibly add a drm_gpuvm_bo_get_gem() to access the > gem member (and that also takes a refcount) for driver users to avoid > the potential pitfall. Except this is only needed because of the GEM-resv-must-be-held locking constraint that was added on vm_bo_put(). I mean, the usual way we do object un-referencing is by calling _put() and letting the internal logic undo things when the refcount drops to zero. If the object needs to be removed from some list, it's normally the responsibility of the destruction method to lock the list, remove the object and unlock the list. Now, we have a refcounted object that's referenced by vm_bo, and whose lock needs to be taken when the destruction happens, which leads to this weird dance described above, when, in normal situations, we'd just call drm_gpuvm_bo_put(vm_bo) and let drm_gpuvm do its thing. >=20 > >=20 > > All these tiny details add to the overall complexity of this common > > layer, and to me, that's not any better than the > > get_next_vm_bo_from_list() complexity you were complaining about > > (might > > be even worth, because this sort of things leak to users). > >=20 > > Having an internal lock partly solves that, in that the locking of > > the > > extobj list is now entirely orthogonal to the GEM that's being > > removed > > from this list, and we can lock/unlock internally without forcing the > > caller to take weird actions to make sure things don't explode. Don't > > get me wrong, I get that this locking overhead is not acceptable for > > Xe, but I feel like we're turning drm_gpuvm into a white elephant > > that > > only few people will get right. =20 >=20 > I tend to agree, but to me the big complication comes from the async > (dma signalling path) state updates. I don't deny updating the VM state from the dma signalling path adds some amount of complexity, but the fact we're trying to use dma_resv locks for everything, including protection of internal datasets doesn't help. Anyway, I think both of us are biased when it comes to judging which approach adds the most complexity :P. Also note that, right now, the only thing I'd like to be able to update from the dma signalling path is the VM mapping tree. Everything else (drm_gpuva_[un]link(), add/remove extobj), we could do outside this path: - for MAP operations, we could call drm_gpuva_link() in the ioctl path (we'd just need to initialize the drm_gpuva object) - for MAP operations, we're already calling drm_gpuvm_bo_obtain() from the ioctl path - for UNMAP operations, we could add the drm_gpuva_unlink() call to the VM op cleanup worker The only problem we'd have is that drm_gpuva_link() needs to be called inside drm_gpuvm_ops::sm_step_remap() when a remap with next/prev !=3D NULL occurs, otherwise we lose track of these mappings. >=20 > Let's say for example we have a lower level lock for the gem object's > gpuvm_bo list. Some drivers grab it from the dma fence signalling path, > other drivers need to access all vm's of a bo to grab their dma_resv > locks using a WW transaction. There will be problems, although probably > solveable. To me, the gpuvm extobj vm_bo list is just an internal list and has an internal lock associated. The lock that's protecting the GEM vm_bo list is a bit different in that the driver gets to decide when a vm_bo is inserted/removed by calling drm_gpuvm_[un]link(), and can easily make sure the lock is held when this happens, while the gpuvm internal lists are kinda transparently updated (for instance, the first caller of drm_gpuvm_bo_obtain() adds the vm_bo to the extobj and the last vm_bo owner calling drm_gpuvm_bo_put() removes it from this list, which is certainly not obvious based on the name of these functions). If we want to let drivers iterate over the extobj/evict lists, and assuming they are considered internal lists maintained by the core and protected with an internal lock, we should indeed provide iteration helpers that: 1/ make sure all the necessary external locks are held (VM resv, I guess) 2/ make sure the internal lock is not held during iteration (the sort of snapshot list trick you're using for the evict list in Xe) > > > Also it seems that if we are to maintain two modes here, for=20 > > > reasonably clean code we'd need two separate instances of=20 > > > get_next_bo_from_list(). > > >=20 > > > For the !RESV_PROTECTED case, perhaps one would want to consider > > > the=20 > > > solution used currently in xe, where the VM maintains two evict > > > lists.=20 > > > One protected by a spinlock and one protected by the VM resv. When > > > the=20 > > > VM resv is locked to begin list traversal, the spinlock is locked > > > *once*=20 > > > and the spinlock-protected list is looped over and copied into the > > > resv=20 > > > protected one. For traversal, the resv protected one is used. =20 > >=20 > > Oh, so you do have the same sort of trick where you move the entire > > list to another list, such that you can let other paths update the > > list > > while you're iterating your own snapshot. That's interesting... =20 >=20 > Yes, it's instead of the "evicted" bool suggested here. I thought the > latter would be simpler. Although that remains to be seen after all > use-cases are implemented. >=20 > But in general I think the concept of copying from a staging list to > another with different protection rather than traversing the first list > and unlocking between items is a good way of solving the locking > inversion problem with minimal overhead. We use it also for O(1) > userptr validation. That's more or less the idea behind get_next_vm_bo_from_list() except it's dequeuing one element at a time, instead of moving all items at once. Note that, if you allow concurrent removal protected only by the spinlock, you still need to take/release this spinlock when iterating over elements of this snapshot list, because all the remover needs to remove an element is the element itself, and it doesn't care in which list it's currently inserted (real or snapshot/staging list), so you'd be iterating over a moving target if you don't protect the iteration with the spinlock.