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 <[email protected]>
---
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
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 <[email protected]>
> > > ---
> > > 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
> > >
> > >
>
>
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 <[email protected]>
> > ---
> > 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
> >
> >
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
>
>
>
>
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 <[email protected]>
> ---
> 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
>
>
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/[email protected]/
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
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
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 <[email protected]>
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
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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
I've successfully tested this on both the front and back cams
of a chuwi hi8 tablet:
Tested-by: Hans de Goede <[email protected]>
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;
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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/[email protected]/
https://lore.kernel.org/lkml/Yli+R7iLZKqO8kVP@iweiny-desk3/
https://lore.kernel.org/lkml/[email protected]/
[PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_set()
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/YldNhErgt53RqYp7@iweiny-desk3/
https://lore.kernel.org/lkml/[email protected]/
[PATCH] staging: media: atomisp: Convert kmap() to kmap_local_page()
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Thanks,
Fabio M. De Francesco
P.S.: Do you want me to resend them all and add the above-mentioned tags?