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, 14 Apr 2022 11:12:39 +0200 (CEST)
From:   Julia Lawall <julia.lawall@inria.fr>
To:     "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
cc:     Ira Weiny <ira.weiny@intel.com>,
        Julia Lawall <julia.lawall@inria.fr>,
        Alison Schofield <alison.schofield@intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Tsuchiya Yuto <kitakar@gmail.com>,
        Martiros Shakhzadyan <vrzh@vrzh.net>,
        Hans de Goede <hdegoede@redhat.com>,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org, outreachy@lists.linux.dev
Subject: Re: [PATCH] staging: media: atomisp: Use kmap_local_page() in
 hmm_store()
In-Reply-To: <4180675.ejJDZkT8p0@leap>
Message-ID: <alpine.DEB.2.22.394.2204141111250.3023@hadrien>
References: <20220413225531.9425-1-fmdefrancesco@gmail.com> <Yld+wpMe1C51bKU3@iweiny-desk3> <alpine.DEB.2.22.394.2204140902030.3023@hadrien> <4180675.ejJDZkT8p0@leap>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


On Thu, 14 Apr 2022, Fabio M. De Francesco wrote:

> On gioved? 14 aprile 2022 09:03:40 CEST Julia Lawall wrote:
> >
> > On Wed, 13 Apr 2022, Ira Weiny wrote:
> >
> > > On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote:
> > > > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco
> wrote:
> > > > > The use of kmap() is being deprecated in favor of kmap_local_page()
> > > > > where it is feasible. The same is true for kmap_atomic().
> > > > >
> > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in
> atomic
> > > > > context and, if so, it calls kmap_atomic(), if not, it calls
> kmap().
> > > > >
> > > > > First of all, in_atomic() shouldn't be used in drivers. This macro
> > > > > cannot always detect atomic context; in particular, it cannot know
> > > > > about held spinlocks in non-preemptible kernels.
> > > > >
> > > > > Notwithstanding what it is said above, this code doesn't need to
> care
> > > > > whether or not it is executing in atomic context. It can simply use
> > > > > kmap_local_page() / kunmap_local() that can instead do the mapping
> /
> > > > > unmapping regardless of the context.
> > > > >
> > > > > With kmap_local_page(), the mapping is per thread, CPU local and
> not
> > > > > globally visible. Therefore, hmm_store()() is a function where the
> use
> > > > > of kmap_local_page() in place of both kmap() and kmap_atomic() is
> > > > > correctly suited.
> > > > >
> > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() /
> > > > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the
> > > > > unnecessary tests which test if the code is in atomic context.
> > > > >
> > > >
> > > > Not specifically about this patch, but more generally about all
> > > > such conversions - is there a 'proof' that shows this just works
> > >
> > > Just code inspection.  Most of them that I have done have been compile
> tested
> > > only.  Part of the key is that des is a local variable and is not
> aliased by
> > > anything outside this function.
> >
> > Typically, the concern about being in atomic context has to do with
> > whether GFP_KERNEL or GFP_ATOMIC should be used, ie whether allocation
> > can sleep.
>
> I'd add that the concern about being in atomic context has mainly to do
> with calling whatever function that may sleep.
>
> Some time ago I analyzed a calls chain which, under spinlocks and with
> IRQ's disabled, led to console_lock() which is annotated with
> might_sleep(). It took about 8000 ms to recover when executing in a 4 CPU /
> 8 SMT System. Linus T. suggested to make this work asynchronous (commit
> 1ee33b1ca2b8 ("tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous")).
>
> > It doesn't have to do with whether some data can be shared.
>
> Yes, FWIW I agree with you.
>
> > Is that the concern here?
>
> The concern here is about the locality of the pointer variable to which the
> struct page has been mapped to. In atomic context we are not allowed to
> kmap() (this is why in the code we had that in_atomic() test), instead we
> can kmap_local_page() or kmap_atomic(). The latter is strongly discouraged
> in favor of the former.

I have the impression that you are first agreeing with me and then
contradicting me :).  Is your point that in general a concern about atomic
context has to do with whether sleeping is allowed, but that the concern
is something else here?  I'm not familiar with these kmap functions.

thanks,
julia


>
> Furthermore, Alison was asking if we can prove that these kinds of
> conversions can actually work when we have not the hardware for testing. As
> Ira wrote, code inspection is sufficient to prove it.
>
> Thanks,
>
> Fabio M. De Francesco
>
>
>
>