Re: [PATCH v2 3/8] vringh: replace kmap_atomic() with kmap_local_page()

On martedì 14 marzo 2023 04:56:08 CET Jason Wang wrote:

On Thu, Mar 2, 2023 at 7:34 PM Stefano Garzarella [off-list ref] 

wrote:

quoted

kmap_atomic() is deprecated in favor of kmap_local_page().

It's better to mention the commit or code that introduces this.

quoted

With kmap_local_page() the mappings are per thread, CPU local, can take
page-faults, and can be called from any context (including interrupts).
Furthermore, the tasks can be preempted and, when they are scheduled to
run again, the kernel virtual addresses are restored and still valid.

kmap_atomic() is implemented like a kmap_local_page() which also disables
page-faults and preemption (the latter only for !PREEMPT_RT kernels,
otherwise it only disables migration).

The code within the mappings/un-mappings in getu16_iotlb() and
putu16_iotlb() don't depend on the above-mentioned side effects of
kmap_atomic(),

Note we used to use spinlock to protect simulators (at least until
patch 7, so we probably need to re-order the patches at least) so I
think this is only valid when:

The vringh IOTLB helpers are not used in atomic context (e.g spinlock,
interrupts).

I'm probably missing some context but it looks that you are saying that 
kmap_local_page() is not suited for any use in atomic context (you are 
mentioning spinlocks).

The commit message (that I know pretty well since it's the exact copy, word by 
word, of my boiler plate commits) explains that kmap_local_page() is perfectly 
usable in atomic context (including interrupts).

I don't know this code, however I am not able to see why these vringh IOTLB 
helpers cannot work if used under spinlocks. Can you please elaborate a little 
more?

If yes, should we document this? (Or should we introduce a boolean to
say whether an IOTLB variant can be used in an atomic context)?

Again, you'll have no problems from the use of kmap_local_page() and so you 
don't need any boolean to tell whether or not the code is running in atomic 
context. 

Please take a look at the Highmem documentation which has been recently 
reworked and extended by me: https://docs.kernel.org/mm/highmem.html

Anyway, I have been ATK 12 or 13 hours in a row. So I'm probably missing the 
whole picture.

Thanks, 

Fabio

Thanks

quoted

so that mere replacements of the old API with the new one
is all that is required (i.e., there is no need to explicitly add calls
to pagefault_disable() and/or preempt_disable()).

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---

Notes:
    v2:
    - added this patch since checkpatch.pl complained about deprecation
    
      of kmap_atomic() touched by next patch
 
 drivers/vhost/vringh.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index a1e27da54481..0ba3ef809e48 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c

@@ -1220,10 +1220,10 @@ static inline int getu16_iotlb(const struct vringh

*vrh,> 
        if (ret < 0)
        
                return ret;

-       kaddr = kmap_atomic(iov.bv_page);
+       kaddr = kmap_local_page(iov.bv_page);

        from = kaddr + iov.bv_offset;
        *val = vringh16_to_cpu(vrh, READ_ONCE(*(__virtio16 *)from));

-       kunmap_atomic(kaddr);
+       kunmap_local(kaddr);

        return 0;
 
 }

@@ -1241,10 +1241,10 @@ static inline int putu16_iotlb(const struct vringh

*vrh,> 
        if (ret < 0)
        
                return ret;

-       kaddr = kmap_atomic(iov.bv_page);
+       kaddr = kmap_local_page(iov.bv_page);

        to = kaddr + iov.bv_offset;
        WRITE_ONCE(*(__virtio16 *)to, cpu_to_vringh16(vrh, val));

-       kunmap_atomic(kaddr);
+       kunmap_local(kaddr);

        return 0;
 
 }

--
2.39.2