Re: [PATCH v9 14/19] zsmalloc: introduce new object mapping API

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Thu, Feb 27, 2025 at 01:35:32PM +0900, Sergey Senozhatsky wrote:
> Current object mapping API is a little cumbersome.  First, it's
> inconsistent, sometimes it returns with page-faults disabled and
> sometimes with page-faults enabled.  Second, and most importantly,
> it enforces atomicity restrictions on its users.  zs_map_object()
> has to return a liner object address which is not always possible
> because some objects span multiple physical (non-contiguous)
> pages.  For such objects zsmalloc uses a per-CPU buffer to which
> object's data is copied before a pointer to that per-CPU buffer
> is returned back to the caller.  This leads to another, final,
> issue - extra memcpy().  Since the caller gets a pointer to
> per-CPU buffer it can memcpy() data only to that buffer, and
> during zs_unmap_object() zsmalloc will memcpy() from that per-CPU
> buffer to physical pages that object in question spans across.
> 
> New API splits functions by access mode:
> - zs_obj_read_begin(handle, local_copy)
>   Returns a pointer to handle memory.  For objects that span two
>   physical pages a local_copy buffer is used to store object's
>   data before the address is returned to the caller.  Otherwise
>   the object's page is kmap_local mapped directly.
> 
> - zs_obj_read_end(handle, buf)
>   Unmaps the page if it was kmap_local mapped by zs_obj_read_begin().
> 
> - zs_obj_write(handle, buf, len)
>   Copies len-bytes from compression buffer to handle memory
>   (takes care of objects that span two pages).  This does not
>   need any additional (e.g. per-CPU) buffers and writes the data
>   directly to zsmalloc pool pages.
> 
> In terms of performance, on a synthetic and completely reproducible
> test that allocates fixed number of objects of fixed sizes and
> iterates over those objects, first mapping in RO then in RW mode:
> 
> OLD API
> =======
> 
> 3 first results out of 10
> 
>   369,205,778      instructions        #    0.80  insn per cycle
>    40,467,926      branches            #  113.732 M/sec
> 
>   369,002,122      instructions        #    0.62  insn per cycle
>    40,426,145      branches            #  189.361 M/sec
> 
>   369,036,706      instructions        #    0.63  insn per cycle
>    40,430,860      branches            #  204.105 M/sec
> 
> [..]
> 
> NEW API
> =======
> 
> 3 first results out of 10
> 
>   265,799,293      instructions        #    0.51  insn per cycle
>    29,834,567      branches            #  170.281 M/sec
> 
>   265,765,970      instructions        #    0.55  insn per cycle
>    29,829,019      branches            #  161.602 M/sec
> 
>   265,764,702      instructions        #    0.51  insn per cycle
>    29,828,015      branches            #  189.677 M/sec
> 
> [..]
> 
> T-test on all 10 runs
> =====================
> 
> Difference at 95.0% confidence
>    -1.03219e+08 +/- 55308.7
>    -27.9705% +/- 0.0149878%
>    (Student's t, pooled s = 58864.4)
> 
> The old API will stay around until the remaining users switch
> to the new one.  After that we'll also remove zsmalloc per-CPU
> buffer and CPU hotplug handling.
> 
> The split of map(RO) and map(WO) into read_{begin/end}/write is
> suggested by Yosry Ahmed.
> 
> Suggested-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>

I see my Reviewed-by was removed at some point. Did something change in
this patch (do I need to review it again) or was it just lost?