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 gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 6FD5EC4332F for ; Fri, 4 Nov 2022 11:24:28 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BAB9B10E73C; Fri, 4 Nov 2022 11:24:27 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id F08C210E73C; Fri, 4 Nov 2022 11:24:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667561066; x=1699097066; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=G1aTmrVsZ8ad6jZu8lVyYIHH8cHgqIzYfaRarwXYjQk=; b=EzXDRZw2S1oXOWtRy6tS8b1SL+miQlLoUsudcWDMkElsz4U5o7eU6isd ANWucTRfldbE7Rdm8/2KfDFx0wFbr+kgUKQhkCHLKaQiJPLqLGUY9kTN6 ++/7D1mdERo0ldvLmwWm9aU4U71E1oBxWhGJuDiJzLkyoE7TRhm7VCR8h hNlytmTwX1L8/PICOTdw1r/eezURqUkOS5eq8MDP9OY73i5rpdUvajVld zcCM8M197Tdo+grFPVDWJqb4HzSpC2Cj1NCGRvhMRid769aygeWCf7H6d FyOgCnqsKNdF6DdlqdlnOLaT6YevboZ8eNO2WhTmTgHFIRsoVSil4iQzr Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10520"; a="311682198" X-IronPort-AV: E=Sophos;i="5.96,137,1665471600"; d="scan'208";a="311682198" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Nov 2022 04:24:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10520"; a="740590212" X-IronPort-AV: E=Sophos;i="5.96,137,1665471600"; d="scan'208";a="740590212" Received: from liuzhao-optiplex-7080.sh.intel.com (HELO localhost) ([10.239.160.132]) by fmsmga002.fm.intel.com with ESMTP; 04 Nov 2022 04:24:09 -0700 Date: Fri, 4 Nov 2022 19:29:54 +0800 From: Zhao Liu To: "Fabio M. De Francesco" , Ira Weiny Message-ID: References: <20221017093726.2070674-1-zhao1.liu@linux.intel.com> <20221017093726.2070674-2-zhao1.liu@linux.intel.com> <2541717.Lt9SDvczpP@suse> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2541717.Lt9SDvczpP@suse> Subject: Re: [Intel-gfx] [PATCH 1/9] drm/i915: Use kmap_local_page() in gem/i915_gem_object.c X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: dri-devel@lists.freedesktop.org, Zhenyu Wang , Thomas =?iso-8859-1?Q?Hellstr=F6m?= , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, Chris Wilson , Dave Hansen , Matthew Auld , Daniel Vetter , Rodrigo Vivi , David Airlie , Christian =?iso-8859-1?Q?K=F6nig?= , Zhao Liu , Nirmoy Das Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote: > Date: Sat, 29 Oct 2022 13:17:03 +0200 > From: "Fabio M. De Francesco" > Subject: Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in > gem/i915_gem_object.c > > On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote: > > From: Zhao Liu > > > > The use of kmap_atomic() is being deprecated in favor of > > kmap_local_page()[1]. > > > > The main difference between atomic and local mappings is that local > > mappings doesn't disable page faults or preemption. > > You are right about about page faults which are never disabled by > kmap_local_page(). However kmap_atomic might not disable preemption. It > depends on CONFIG_PREEMPT_RT. > > Please refer to how kmap_atomic_prot() works (this function is called by > kmap_atomic() when kernels have HIGHMEM enabled). Yes, there is some ambiguity here. What about "The main difference between atomic and local mappings is that local mappings never disable page faults or preemption"? > > > > > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't > > need to disable pagefaults and preemption for mapping: > > > > 1. The flush operation is safe for CPU hotplug when preemption is not > > disabled. > > I'm confused here. Why are you talking about CPU hotplug? > In any case, developers should never rely on implicit calls of > preempt_disable() for the reasons said above. Therefore, flush operations > should be allowed regardless that kmap_atomic() potential side effect. Sorry, it's my fault, my misunderstanding about the connection between hotplug and flush here. When mapping exists, the cpu cannot be unplugged via CPU-hotplug. But whether plug or unplug, it has nothing to do with flush. I will delete this wrong description. My initial consideration is that this interface of flush may require an atomic context, so I want to explain more from the details of its implementation that cache consistency can be guaranteed without atomic context. Is this consideration redundant? Also, do I need to state that migration is still ok for this flush interface here (since __kmap_local_page_prot() doesn't always disable migration)? > > In drm/i915/gem/i915_gem_object.c, the function > > i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range() > > If I recall correctly, drm_clflush_virt_range() can always be called with page > faults and preemption enabled. If so, this is enough to say that the > conversion is safe. > > Is this code explicitly related to flushing the cache lines before removing / > adding CPUs? If I recall correctly, there are several other reasons behind the > need to issue cache lines flushes. Am I wrong about this? > > Can you please say more about what I'm missing here? > > > to > > use CLFLUSHOPT or WBINVD to flush. Since CLFLUSHOPT is global on x86 > > and WBINVD is called on each cpu in drm_clflush_virt_range(), the flush > > operation is global and any issue with cpu's being added or removed > > can be handled safely. > > Again your main concern is about CPU hotplug. > > Even if I'm missing something, do we really need all these details about the > inner workings of drm_clflush_virt_range()? > > I'm not an expert, so may be that I'm wrong about all I wrote above. > > Therefore, can you please elaborate a little more for readers with very little > knowledge of these kinds of things (like me and perhaps others)? > > > 2. Any context switch caused by preemption or sleep (pagefault may > > cause sleep) doesn't affect the validity of local mapping. > > I'd replace "preemption or sleep" with "preemption and page faults" since > yourself then added that page faults lead to tasks being put to sleep. Thanks, good advice. Zhao