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

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

[Fabio M. De Francesco]

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.

Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
index 46ac082cd3f1..54188197c3dc 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
@@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
 		idx = (virt - bo->start) >> PAGE_SHIFT;
 		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
 
-		if (in_atomic())
-			des = (char *)kmap_atomic(bo->page_obj[idx].page);
-		else
-			des = (char *)kmap(bo->page_obj[idx].page);
+		des = (char *)kmap_local_page(bo->page_obj[idx].page);
 
 		if (!des) {
 			dev_err(atomisp_dev,
@@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
 
 		clflush_cache_range(des, len);
 
-		if (in_atomic())
-			/*
-			 * Note: kunmap_atomic requires return addr from
-			 * kmap_atomic, not the page. See linux/highmem.h
-			 */
-			kunmap_atomic(des - offset);
-		else
-			kunmap(bo->page_obj[idx].page);
+		kunmap_local(des);
 	}
 
 	return 0;
-- 
2.34.1

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

[Alison Schofield]

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
or do we need to test each one. If the latter, then how?


> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index 46ac082cd3f1..54188197c3dc 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  		idx = (virt - bo->start) >> PAGE_SHIFT;
>  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
>  
> -		if (in_atomic())
> -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> -		else
> -			des = (char *)kmap(bo->page_obj[idx].page);
> +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
>  
>  		if (!des) {
>  			dev_err(atomisp_dev,
> @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  
>  		clflush_cache_range(des, len);
>  
> -		if (in_atomic())
> -			/*
> -			 * Note: kunmap_atomic requires return addr from
> -			 * kmap_atomic, not the page. See linux/highmem.h
> -			 */
> -			kunmap_atomic(des - offset);
> -		else
> -			kunmap(bo->page_obj[idx].page);
> +		kunmap_local(des);
>  	}
>  
>  	return 0;
> -- 
> 2.34.1
> 
> 

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

[Ira Weiny]

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.

> or do we need to test each one. If the latter, then how?

Generally there is no test if we don't have the hardware.  Some of the more
difficult conversions will probably need to have some testing done but that
will need to be discussed with the subsystem maintainers at the time.

Ira

> 
> 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > index 46ac082cd3f1..54188197c3dc 100644
> > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> >  		idx = (virt - bo->start) >> PAGE_SHIFT;
> >  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
> >  
> > -		if (in_atomic())
> > -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> > -		else
> > -			des = (char *)kmap(bo->page_obj[idx].page);
> > +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
> >  
> >  		if (!des) {
> >  			dev_err(atomisp_dev,
> > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> >  
> >  		clflush_cache_range(des, len);
> >  
> > -		if (in_atomic())
> > -			/*
> > -			 * Note: kunmap_atomic requires return addr from
> > -			 * kmap_atomic, not the page. See linux/highmem.h
> > -			 */
> > -			kunmap_atomic(des - offset);
> > -		else
> > -			kunmap(bo->page_obj[idx].page);
> > +		kunmap_local(des);
> >  	}
> >  
> >  	return 0;
> > -- 
> > 2.34.1
> > 
> > 

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

[Julia Lawall]

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.  It doesn't have to do with whether some data can be shared.  Is
that the concern here?

julia

>
> > or do we need to test each one. If the latter, then how?
>
> Generally there is no test if we don't have the hardware.  Some of the more
> difficult conversions will probably need to have some testing done but that
> will need to be discussed with the subsystem maintainers at the time.
>
> Ira
>
> >
> >
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
> > >  1 file changed, 2 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > index 46ac082cd3f1..54188197c3dc 100644
> > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> > > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> > >  		idx = (virt - bo->start) >> PAGE_SHIFT;
> > >  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
> > >
> > > -		if (in_atomic())
> > > -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> > > -		else
> > > -			des = (char *)kmap(bo->page_obj[idx].page);
> > > +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
> > >
> > >  		if (!des) {
> > >  			dev_err(atomisp_dev,
> > > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
> > >
> > >  		clflush_cache_range(des, len);
> > >
> > > -		if (in_atomic())
> > > -			/*
> > > -			 * Note: kunmap_atomic requires return addr from
> > > -			 * kmap_atomic, not the page. See linux/highmem.h
> > > -			 */
> > > -			kunmap_atomic(des - offset);
> > > -		else
> > > -			kunmap(bo->page_obj[idx].page);
> > > +		kunmap_local(des);
> > >  	}
> > >
> > >  	return 0;
> > > --
> > > 2.34.1
> > >
> > >
>
>

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

[Fabio M. De Francesco]

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.

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

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

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

[Fabio M. De Francesco]

On gioved? 14 aprile 2022 11:12:39 CEST Julia Lawall wrote:
> 
> 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.

Yes the concern is something else here (sorry for my poor English). It was 
not my intention to contradict you :) 

The concern for "locality" here is that if the variable is shared between 
different functions there are cases where we cannot use kmap_local_page().
For example, those mappings with kmap_local_page() are per thread and CPU 
local.

I think that the best means to convey what I want to express is pointing to 
my patch to Documentation/vm/highmem.rst:

https://lore.kernel.org/lkml/20220412124003.10736-1-fmdefrancesco@gmail.com/

I hope that this can help to understand why we care of "locality" in this 
specific case where we use kmap_local_page().

Again, sorry for not being clear.

Thanks,

Fabio M. De Francesco

> 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

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

[Ira Weiny]

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.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

I've had to research this a bit myself because I've not seen this pattern
before.

kmap_atomic() had 2 uses:

	1) a situation where the operation on the page requires pagefaults
	   and/or preemption to be disabled
	2) in a situation where one knows the code can't sleep

kmap_local_page() removes the second use case because kmap() is no longer the
only alternative; from the kdoc for kmap_atomic():

...
 * kmap_atomic - Atomically map a page for temporary usage - Deprecated!
...
 * Effectively a wrapper around kmap_local_page() which disables pagefaults
 * and preemption.
...

The deprecation is because any pagefault/preemption disabling should probably
be done explicitly from now on but allows for existing kmap_atomic() callers to
live on.

In this case, I suspect the original driver writer wanted to use kmap() but
hmm_store() was called from various contexts.  So they incorrectly tried to
protect against the (potentially) sleeping kmap() call.  In reality, I think
they could have simply called kmap_atomic() and skipped 'in_atomic()' check
altogether.

Regardless now that kmap_local_page() exists, this is correct.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index 46ac082cd3f1..54188197c3dc 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  		idx = (virt - bo->start) >> PAGE_SHIFT;
>  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
>  
> -		if (in_atomic())
> -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> -		else
> -			des = (char *)kmap(bo->page_obj[idx].page);
> +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
>  
>  		if (!des) {
>  			dev_err(atomisp_dev,
> @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  
>  		clflush_cache_range(des, len);
>  
> -		if (in_atomic())
> -			/*
> -			 * Note: kunmap_atomic requires return addr from
> -			 * kmap_atomic, not the page. See linux/highmem.h
> -			 */
> -			kunmap_atomic(des - offset);
> -		else
> -			kunmap(bo->page_obj[idx].page);
> +		kunmap_local(des);
>  	}
>  
>  	return 0;
> -- 
> 2.34.1
> 

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

[Fabio M. De Francesco]

On venerd? 15 aprile 2022 02:37:27 CEST Ira Weiny wrote:

> Regardless now that kmap_local_page() exists, this is correct.
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

I'm pretty sure this particular patch would have had a hard time getting 
accepted without any authoritative "Reviewed-by" tag from you or other 
people more experienced than me.

Thank you very much!

Fabio M. De Francesco

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

[Hans de Goede]

Hi,

On 4/14/22 00:55, 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.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>


I've successfully tested this on both the front and back cams
of a chuwi hi8 tablet:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> index 46ac082cd3f1..54188197c3dc 100644
> --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c
> +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c
> @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  		idx = (virt - bo->start) >> PAGE_SHIFT;
>  		offset = (virt - bo->start) - (idx << PAGE_SHIFT);
>  
> -		if (in_atomic())
> -			des = (char *)kmap_atomic(bo->page_obj[idx].page);
> -		else
> -			des = (char *)kmap(bo->page_obj[idx].page);
> +		des = (char *)kmap_local_page(bo->page_obj[idx].page);
>  
>  		if (!des) {
>  			dev_err(atomisp_dev,
> @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes)
>  
>  		clflush_cache_range(des, len);
>  
> -		if (in_atomic())
> -			/*
> -			 * Note: kunmap_atomic requires return addr from
> -			 * kmap_atomic, not the page. See linux/highmem.h
> -			 */
> -			kunmap_atomic(des - offset);
> -		else
> -			kunmap(bo->page_obj[idx].page);
> +		kunmap_local(des);
>  	}
>  
>  	return 0;

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

[Fabio M. De Francesco]

On giovedì 14 aprile 2022 00:55:31 CEST 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.
> 
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)

Hi Mauro,

I'm writing for just a gentle ping for this and two other staging/atomisp/ 
patches that still seem to be waiting to be applied.

In the meantime I would like to remind you that Hans de Goede has 
successfully tested this patch and the other two on both the front and back 
cams of a chuwi hi8 tablet.

Please let me know if there is anything else that is required to be done in 
order to accept the three patches.

Thanks,

Fabio M. De Francesco

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

[Fabio M. De Francesco]

On lunedì 25 aprile 2022 20:29:03 CEST Fabio M. De Francesco wrote:
> On giovedì 14 aprile 2022 00:55:31 CEST 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.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------
> >  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> Hi Mauro,
> 
> I'm writing for just a gentle ping for this and two other staging/
atomisp/ 
> patches that still seem to be waiting to be applied.
> 
> In the meantime I would like to remind you that Hans de Goede has 
> successfully tested this patch and the other two on both the front and 
back 
> cams of a chuwi hi8 tablet.

Hi Mauro,

In my previous message I forgot to remind you that two of these three 
patches have also been reviewed by Ira Weiny (Intel).

I'd like to ask you again if there is still something left which prevents 
these three patches from being accepted. The first of the three patches was 
submitted on April 9, 2022.

For you convenience here are the links to the patches, the "Reviewed-by:" 
and "Tested-by:" tags:

[PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_store()
https://lore.kernel.org/lkml/20220413225531.9425-1-fmdefrancesco@gmail.com/
https://lore.kernel.org/lkml/Yli+R7iLZKqO8kVP@iweiny-desk3/
https://lore.kernel.org/lkml/2d096f20-dbaa-1d49-96e9-a7ae6c19f7fe@redhat.com/

[PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_set()
https://lore.kernel.org/lkml/20220413212210.18494-1-fmdefrancesco@gmail.com/
https://lore.kernel.org/lkml/YldNhErgt53RqYp7@iweiny-desk3/
https://lore.kernel.org/lkml/0b04ad1a-e442-1728-ef2c-bab386a4c64c@redhat.com/

[PATCH] staging: media: atomisp: Convert kmap() to kmap_local_page()
https://lore.kernel.org/lkml/20220408223129.3844-1-fmdefrancesco@gmail.com/
https://lore.kernel.org/lkml/b0aed731-b56f-4378-b50e-fc0cbccbdb84@redhat.com/

Thanks,

Fabio M. De Francesco

P.S.: Do you want me to resend them all and add the above-mentioned tags?