Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 3 Nov 2022 09:51:23 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
CC:     Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Matthew Auld <matthew.auld@intel.com>,
        Thomas =?iso-8859-1?Q?Hellstr=F6m?= 
        <thomas.hellstrom@linux.intel.com>,
        Nirmoy Das <nirmoy.das@intel.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Chris Wilson <chris@chris-wilson.co.uk>,
        Christian =?iso-8859-1?Q?K=F6nig?= <christian.koenig@amd.com>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        Zhao Liu <zhao1.liu@linux.intel.com>,
        Zhenyu Wang <zhenyu.z.wang@intel.com>,
        Zhao Liu <zhao1.liu@intel.com>,
        Dave Hansen <dave.hansen@intel.com>
Subject: Re: [PATCH 1/9] drm/i915: Use kmap_local_page() in
 gem/i915_gem_object.c
Message-ID: <Y2Pxi9FsdeULhHKI@iweiny-desk3>
References: <20221017093726.2070674-1-zhao1.liu@linux.intel.com>
 <20221017093726.2070674-2-zhao1.liu@linux.intel.com>
 <2541717.Lt9SDvczpP@suse>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2541717.Lt9SDvczpP@suse>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?HrhMyXggiUoHTc5ZRlv1jn2Jwa4M40uiEBA7qFGqe7TYsRb2WJTK1o9zww?=
 =?iso-8859-1?Q?Tg2WM/ueJ5I206SWSN7fMB86oaYyBaZORLod9gzSU8HXEG4u3bRHkpCCmb?=
 =?iso-8859-1?Q?NwMQS6uPTypC8MOsKXsTBPd33lry3Rb1tASlF7Eh+lKivANhthPW6lc2Vz?=
 =?iso-8859-1?Q?6NVqJYoCYoiSNsFANR0c0MYl/CjV10Xg4bMkoXLsXGBLyuEQFMJj0mtnud?=
 =?iso-8859-1?Q?e2CamhQA/Cif1nQ2LDbWrqs0SXsNrnrVPHW3vu2uN8fauYnDa6V8VHQl0c?=
 =?iso-8859-1?Q?HJHpTYg2xlwXfwKjoOY4x9BfDI7ZDr/kXhnUdpe5KRcapM/EuaDQXH5fqG?=
 =?iso-8859-1?Q?X2rADvWezD7P8KxfsICnWqKnmluYdnUOTC0Gqp+74hKy2tN0KMOOQBIF9m?=
 =?iso-8859-1?Q?Yz4iLUJVp+S1YKrEB9bAJyTtXIPZZzTODUO8TvNx6FnCROpgC+JylSZcTe?=
 =?iso-8859-1?Q?eKMg3grq4RuwyL0yk58tH5SyRiWnUPv6X/pq486gNqmU7AY8Kzo8DwM81f?=
 =?iso-8859-1?Q?xNTLl8ScRXGQt7rYX32VjvEhf0NoUmVyYUrJSP8Hre4RmMx5wsRDbPJ2jf?=
 =?iso-8859-1?Q?HUCetYvfa4dmsZ9nvSDkp1mqXcAWYkcWfohilgnRGUDjbbdude3WhXBAUX?=
 =?iso-8859-1?Q?tELwpQ1tuugzmbC1gNhH1stUFZO1iOi6NsBV4742YWFOrUX9KbaMeChiCQ?=
 =?iso-8859-1?Q?qZPhRjxlIMiui5fB9RiqFqoAMiABMogzGQmhVey9xeq3eOHUHLn7t5h2oS?=
 =?iso-8859-1?Q?n9/Gs4ns6mjT8m78arGy7begGqdeM7TQqwRDfX6CO9ITuhhZTH7ayKhNNt?=
 =?iso-8859-1?Q?YJ6CwVOKmZAh1yTth0EbaFzhHs8A/N9T6cJ0RMroWmRu2ZFuLIoc3LWRdy?=
 =?iso-8859-1?Q?i4Ltx/WbL6Py0C2AeDnefHGq4TXL304qZwWsZoJ15vYkBsziduezLUerI3?=
 =?iso-8859-1?Q?qFSOIB3WNGz6OTK2M43GZB7vnTCcOJS7G7TiN0nUZJOs0jDyf92LC9WBxT?=
 =?iso-8859-1?Q?VI5DauzniBMPXx8YGLAV2Ji2Jj2pGjLGb4f4Z5xqHdbd1LR1RXMQ/9Izfd?=
 =?iso-8859-1?Q?iF7VMTORT73JodH7oSJQi9JhK7sVDSncF+NKDvU8V1ihFWLohczJzR62ps?=
 =?iso-8859-1?Q?9wXyaIsDQGYTiAEXUCcPX4x1ZUDD6E+blKUdTHzyXmSwQXaQ0Q4BBjo6BO?=
 =?iso-8859-1?Q?MDkyRwUtjXI1icra+eheSEcHLeXXCALam+FKqJEcKOdFYsoE7GI6DTvt4G?=
 =?iso-8859-1?Q?ZanS61ybs1CYxX5f2AXjW77xvAbO1TSjoTKwoiT7/CIhvpX8zsEdbuJYin?=
 =?iso-8859-1?Q?1f0euRrKaoBnqeP/3SJrT7Dreh93XeoepmZyYjf55J3jH+Ewuymb04fmAR?=
 =?iso-8859-1?Q?xAviu3QYag/IqWxFXuz7KHyUEaW/V6DjBihbe0VisdIzJC8elW5+LSsCsu?=
 =?iso-8859-1?Q?bR03xv1bjKDlo5nPNSmZN0KbD6rf1enEH9VuurfqMAK3n3XLH1R8g/RAwW?=
 =?iso-8859-1?Q?c2g/iRzyd/cL+ajV8bdfslSsrksPUnaEXmEAUP3lk7d7EBV8c2u1JBi3ny?=
 =?iso-8859-1?Q?ET82kUP75PbyUytiDnNcY0u0Q6C8PfjEl/fgPlhdHCJWieSTe8O7aHcdD6?=
 =?iso-8859-1?Q?CsAfnTdbzv3x4Cb9a+DaYR/nNY/7gUZdBR?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 723167a9-a3ef-4a33-1381-08dabdbba6f6
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Nov 2022 16:51:28.5744
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hK1R9/I/s4ZOo8j8JGCNsZmlle+kDWlU/W8+sMTeOBCiMQv8RpCZ1e2G4BIG3verG4AHmF6hjfPIS5xux1v1cQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6232
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        SPF_HELO_NONE,SPF_NONE 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: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> On luned? 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > 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).
> 
> > 
> > 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?

I agree with Fabio here.  I'm not making the connection between cpu hotplug and
this code path.

Ira

> 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.
> 
> > 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.  
> 
> > Therefore, i915_gem_object_read_from_page_kmap() is a function where
> > the use of kmap_local_page() in place of kmap_atomic() is correctly
> > suited.
> > 
> > Convert the calls of kmap_atomic() / kunmap_atomic() to
> > kmap_local_page() / kunmap_local().
> > 
> > And remove the redundant variable that stores the address of the mapped
> > page since kunmap_local() can accept any pointer within the page.
> > 
> > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@intel.com
> > 
> > Suggested-by: Dave Hansen <dave.hansen@intel.com>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Suggested-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> > Suggested by credits:
> >   Dave: Referred to his explanation about cache flush.
> >   Ira: Referred to his task document, review comments and explanation about
> >        cache flush.
> >   Fabio: Referred to his boiler plate commit message.
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 
> 369006c5317f..a0072abed75e 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -413,17 +413,15 @@ void __i915_gem_object_invalidate_frontbuffer(struct
> > drm_i915_gem_object *obj, static void
> >  i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64 
> offset, void
> > *dst, int size) {
> > -	void *src_map;
> >  	void *src_ptr;
> > 
> > -	src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >> 
> PAGE_SHIFT));
> > -
> > -	src_ptr = src_map + offset_in_page(offset);
> > +	src_ptr = kmap_local_page(i915_gem_object_get_page(obj, offset >> 
> PAGE_SHIFT))
> > +	          + offset_in_page(offset);
> >  	if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> >  		drm_clflush_virt_range(src_ptr, size);
> >  	memcpy(dst, src_ptr, size);
> > 
> > -	kunmap_atomic(src_map);
> > +	kunmap_local(src_ptr);
> >  }
> > 
> >  static void
> 
> The changes look good, but I'd like to better understand the commit message.
> 
> Thanks,
> 
> Fabio 
> 
>