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;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     Ira Weiny <ira.weiny@intel.com>,
        Zhao Liu <zhao1.liu@linux.intel.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,
        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
Date:   Thu, 03 Nov 2022 20:22:04 +0100
Message-ID: <12087538.O9o76ZdvQC@suse>
In-Reply-To: <Y2Pxi9FsdeULhHKI@iweiny-desk3>
References: <20221017093726.2070674-1-zhao1.liu@linux.intel.com> <2541717.Lt9SDvczpP@suse> <Y2Pxi9FsdeULhHKI@iweiny-desk3>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

On gioved=EC 3 novembre 2022 17:51:23 CET Ira Weiny wrote:
> On Sat, Oct 29, 2022 at 01:17:03PM +0200, Fabio M. De Francesco wrote:
> > On luned=EC 17 ottobre 2022 11:37:17 CEST Zhao Liu wrote:
> > > From: Zhao Liu <zhao1.liu@intel.com>
> > >=20
> > > The use of kmap_atomic() is being deprecated in favor of
> > > kmap_local_page()[1].
> > >=20
> > > The main difference between atomic and local mappings is that local
> > > mappings doesn't disable page faults or preemption.
> >=20
> > 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.
> >=20
> > Please refer to how kmap_atomic_prot() works (this function is called by
> > kmap_atomic() when kernels have HIGHMEM enabled).
> >=20
> > > There're 2 reasons why i915_gem_object_read_from_page_kmap() doesn't
> > > need to disable pagefaults and preemption for mapping:
> > >=20
> > > 1. The flush operation is safe for CPU hotplug when preemption is not
> > > disabled.
> >=20
> > I'm confused here. Why are you talking about CPU hotplug?
>=20
> I agree with Fabio here.  I'm not making the connection between cpu hotpl=
ug=20
and
> this code path.
>=20
> Ira

@Zhao,

I'd like to add that I was about to put my reviewed-by tag. The other thing=
s I=20
objected are minor nits. Please just clarify this connection.

Your code is good and deserves to be applied.

=46abio

>=20
> > In any case, developers should never rely on implicit calls of
> > preempt_disable() for the reasons said above. Therefore, flush operatio=
ns
> > should be allowed regardless that kmap_atomic() potential side effect.
> >=20
> > > In drm/i915/gem/i915_gem_object.c, the function
> > > i915_gem_object_read_from_page_kmap() calls drm_clflush_virt_range()
> >=20
> > If I recall correctly, drm_clflush_virt_range() can always be called wi=
th=20
page
> > faults and preemption enabled. If so, this is enough to say that the
> > conversion is safe.
> >=20
> > Is this code explicitly related to flushing the cache lines before=20
removing /
> > adding CPUs? If I recall correctly, there are several other reasons beh=
ind=20
the
> > need to issue cache lines flushes. Am I wrong about this?
> >=20
> > Can you please say more about what I'm missing here?
> >=20
> > > 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 flu=
sh
> > > operation is global and any issue with cpu's being added or removed
> > > can be handled safely.
> >=20
> > Again your main concern is about CPU hotplug.
> >=20
> > Even if I'm missing something, do we really need all these details abou=
t=20
the
> > inner workings of drm_clflush_virt_range()?
> >=20
> > I'm not an expert, so may be that I'm wrong about all I wrote above.
> >=20
> > Therefore, can you please elaborate a little more for readers with very=
=20
little
> > knowledge of these kinds of things (like me and perhaps others)?
> >=20
> > > 2. Any context switch caused by preemption or sleep (pagefault may
> > > cause sleep) doesn't affect the validity of local mapping.
> >=20
> > I'd replace "preemption or sleep" with "preemption and page faults" sin=
ce
> > yourself then added that page faults lead to tasks being put to sleep.
> >=20
> > > 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.
> > >=20
> > > Convert the calls of kmap_atomic() / kunmap_atomic() to
> > > kmap_local_page() / kunmap_local().
> > >=20
> > > And remove the redundant variable that stores the address of the mapp=
ed
> > > page since kunmap_local() can accept any pointer within the page.
> > >=20
> > > [1]: https://lore.kernel.org/all/20220813220034.806698-1-ira.weiny@in=
tel.com
> > >=20
> > > 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>
> > > ---
> > >=20
> > > Suggested by credits:
> > >   Dave: Referred to his explanation about cache flush.
> > >   Ira: Referred to his task document, review comments and explanation=
=20
about
> > >  =20
> > >        cache flush.
> > >  =20
> > >   Fabio: Referred to his boiler plate commit message.
> > >=20
> > > ---
> > >=20
> > >  drivers/gpu/drm/i915/gem/i915_gem_object.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >=20
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_object.c index
> >=20
> > 369006c5317f..a0072abed75e 100644
> >=20
> > > --- 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=20
__i915_gem_object_invalidate_frontbuffer(struct
> > > drm_i915_gem_object *obj, static void
> > >=20
> > >  i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj,=
=20
u64
> >=20
> > offset, void
> >=20
> > > *dst, int size) {
> > > -	void *src_map;
> > >=20
> > >  	void *src_ptr;
> > >=20
> > > -	src_map =3D kmap_atomic(i915_gem_object_get_page(obj, offset >>
> >=20
> > PAGE_SHIFT));
> >=20
> > > -
> > > -	src_ptr =3D src_map + offset_in_page(offset);
> > > +	src_ptr =3D kmap_local_page(i915_gem_object_get_page(obj, offset >>
> >=20
> > PAGE_SHIFT))
> >=20
> > > +	          + offset_in_page(offset);
> > >=20
> > >  	if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> > >  =09
> > >  		drm_clflush_virt_range(src_ptr, size);
> > >  =09
> > >  	memcpy(dst, src_ptr, size);
> > >=20
> > > -	kunmap_atomic(src_map);
> > > +	kunmap_local(src_ptr);
> > >=20
> > >  }
> > > =20
> > >  static void
> >=20
> > The changes look good, but I'd like to better understand the commit=20
message.
> >=20
> > Thanks,
> >=20
> > Fabio