Re: [PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_store()

[Julia Lawall]

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
>
>
>
>