lkml.org 
[lkml]   [2022]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] erofs: use kmap_local_page() only for erofs_bread()
    Date
    On Wednesday, October 19, 2022 1:36:55 AM CEST Gao Xiang wrote:
    > On Wed, Oct 19, 2022 at 01:21:27AM +0200, Fabio M. De Francesco wrote:
    > > On Tuesday, October 18, 2022 11:29:21 PM CEST Gao Xiang wrote:
    >
    > ...
    >
    > >
    > > > One of what I need to care is nested kmap() usage,
    > > > some unmap/remap order cannot be simply converted to kmap_local()
    > >
    > > Correct about nesting. If we map A and then B, we must unmap B and then A.
    > >
    > > However, as you seem to convey, not always unmappings in right order
    (stack
    > > based) is possible, sometimes because very long functions have many loop's
    > > breaks and many goto exit labels.
    > >
    > > > but I think
    > > > it's not the case for erofs_bread(). Actually EROFS has one of such
    nested
    > > > kmap() usage, but I don't really care its performance on HIGHMEM
    platforms,
    > > > so I think kmap() is still somewhat useful compared to kmap_local() from
    > > this
    > > > point of view],
    > >

    fs/erofs conversions are in our (Ira's and my) list. So I'm am happy to see
    that we can delete some entries because of your changes. :-)

    > > In Btrfs I solved (thanks to David S.' advice) by mapping only one of two
    > > pages, only the one coming from the page cache.
    > >
    > > The other page didn't need the use of kmap_local_page() because it was
    > > allocated in the filesystem with "alloc_page(GFP_NOFS)". GFP_NOFS won't
    ever
    > > allocate from ZONE_HIGHMEM, therefore a direct page_address() could avoid
    the
    > > mapping and the nesting issues.
    > >
    > > Did you check if you may solve the same way?
    >
    > That is not simple. Currently we have compressed pages and decompressed
    > pages (page cache or others), and they can be unmapped when either data
    > is all consumed, so compressed pages can be unmapped first, or
    > decompressed pages can be unmapped first. That quite depends on which
    > pages goes first.
    >
    > I think such usage is a quite common pattern for decoder or encoder,
    > you could take a look at z_erofs_lzma_decompress() in
    > fs/erofs/decompressor_lzma.c.

    I haven't yet read that code, however I may attempt to propose a pattern for
    solving this kinds of issue, I mean where you don't know which page got mapped
    last...

    It's not elegant but it may work. You have compressed and decompressed pages
    and you can't know in advance what page should be unmapped first because you
    can't know in which order they where mapped, right?

    I'd use a variable to save two different values, each representing the last
    page mapped. When the code gets to the unmapping block (perhaps in an "out"
    label), just check what that variable contains. Depending on that value, say
    'c' or 'd', you will be able to know what must be unmapped for first. An "if /
    else" can do the work.

    What do you think of this?

    > So kmap() is still useful for such cases
    > since I don't really care the HIGHMEM performance but correctness, but
    > other alternative could churn/complex the map/unmap/remap pattern.

    Sooner or later someone will have to address this issue and remove those
    kmap() call sites. We are working on this and hope to always figure out a way
    to work it out.

    I hope that what I wrote above may help, although I'm writing on a purely
    theoretically bases, since, as said, I haven't yet seen that code.

    If due to my poor English I've not been able to convey my thoughts please let
    me know, so that I'll try to reword.

    Thanks,

    Fabio

    > Thanks,
    > Gao Xiang
    >
    > >
    > > A little group of people are working to remove all kmap() and
    kmap_atomic() we
    > > meet across the whole kernel. I have not yet encountered conversions which
    > > cannot be made. Sometimes we may refactor, if what I said above cannot
    apply.
    > >
    > > > but in order to make it all work properly, I will try to do
    > > > stress test with 32-bit platform later.
    > >
    > > I use QEMU/KVM x86_32 VM, 6GB RAM, and a kernel with HIGHMEM64G enabled
    and an
    > > openSUSE Tumbleweed 32 distro. I've heard that Debian too provides an
    x86_32
    > > distribution.
    > >
    > > > Since it targets for the next cycle
    > > > 6.2, I will do a full stress test in the next following weeks.
    > > >
    > > > Thanks,
    > > > Gao Xiang
    > > >
    > >
    > > Thanks,
    > >
    > > Fabio
    > >
    >



    \
     
     \ /
      Last update: 2022-10-19 20:18    [W:5.629 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site