Re: [PATCH 5/7] zsmalloc: embed struct zs_handle into zram and zswap entries

[Sergey Senozhatsky]

On (26/11/10 11:51), Sergey Senozhatsky wrote:
> Date: Tue, 10 Nov 2026 11:51:21 +0900
> From: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> To: Yosry Ahmed <yosry@xxxxxxxxxx>
> Cc: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>,  Andrew Morton
>  <akpm@xxxxxxxxxxxxxxxxxxxx>, Nhat Pham <nphamcs@xxxxxxxxx>,  Yosry Ahmed
>  <yosry.ahmed@xxxxxxxxx>, Johannes Weiner <hannes@xxxxxxxxxxx>,  Brian
>  Geffon <bgeffon@xxxxxxxxxx>, Minchan Kim <minchan@xxxxxxxxxx>,  Richard
>  Chang <richardycc@xxxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx,
>  linux-block@xxxxxxxxxxxxxxx,  linux-mm@xxxxxxxxx
> Subject: Re: [PATCH 5/7] zsmalloc: embed struct zs_handle into zram and
>  zswap entries
> Message-ID: <avKFc82JS4R9yU-N@xxxxxxxxxx>
> 
> On (26/11/05 17:19), Yosry Ahmed wrote:
> [..]
> > >  Additionally, zs_malloc() does not return a dereferenceable pointer.
> > > -Instead, it returns an opaque handle (unsigned long) which encodes actual
> > > -location of the allocated object. The reason for this indirection is that
> > > +Instead, it stores the encoded location of the allocated object in the
> > > +provided ``struct zs_handle``. The reason for this indirection is that
> > 
> > Should we explicitly call out that zsmalloc will keep a back pointer to
> > the handle and update it asynchronously, so the owner shouldn't modify
> > it, and should keep it allocated until zs_free() (or rebind)?
> > 
> > Probably somewhere in the code comments too?
> 
> Yeah, perhaps.
> 
> > >  zsmalloc does not keep zspages permanently mapped since that would cause
> > >  issues on 32-bit systems where the VA region for kernel space mappings
> > >  is very small. So, using the allocated memory should be done through the
> > > diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> > > index 693c9484b638..6c76fd10344e 100644
> > > --- a/include/linux/zsmalloc.h
> > > +++ b/include/linux/zsmalloc.h
> > > @@ -14,6 +14,7 @@
> > >  #ifndef _ZS_MALLOC_H_
> > >  #define _ZS_MALLOC_H_
> > >  
> > > +#include <linux/compiler.h>
> > >  #include <linux/types.h>
> > >  
> > >  struct zs_pool_stats {
> > > @@ -21,16 +22,39 @@ struct zs_pool_stats {
> > >  	atomic_long_t pages_compacted;
> > >  };
> > >  
> > > +/*
> > > + * zsmalloc writes object location into the handle and updates it when
> > > + * the object moves (compaction, page migration), so the handle must not
> > > + * be on the stack and must not be copied.  Use zs_rebind_handle() to
> > > + * move an object to another handle.
> > > + */
> > > +struct zs_handle {
> > > +	unsigned long __private obj;
> > > +} __aligned(sizeof(unsigned long));
> > 
> > Is alignment guaranteed even if this is embedded in a packed struct? I
> > wonder if we should have an explicit alignment check in
> > zs_malloc()/zs_store(), and probably rebind too?
> 
> I guess only run-time check will work.  I can add those.  I wonder
> if we actually need to worry about this, neither zram nor zswap use
> packed entries, but I think it's cheap enough to make sure in run-time.
> 
> > > +
> > > +static inline void zs_handle_init(struct zs_handle *handle)
> > > +{
> > > +	ACCESS_PRIVATE(handle, obj) = 0;
> > > +}
> > > +
> > > +static inline bool zs_handle_is_allocated(const struct zs_handle *handle)
> > > +{
> > > +	return READ_ONCE(ACCESS_PRIVATE(handle, obj)) != 0;
> > > +}
> > 
> > Can this ever give a false negative?
> 
> That's a very good question.
> 
> > I believe that's only possible with pfn=0, I don't know if this can be the
> > case on any architecture. Should we use something like OBJ_ALLOCATED_TAG?
> 
> Yeah, I don't know, I guess would be better to not rely on "pfn cannot be
> 0".  Let me look into this.

Look what I found:

--------------------------------------------------------------------------
commit 67296874eb1cc80317bf2a8fba22b494e21eb29b
Author: Olav Haugan <ohaugan@xxxxxxxxxxxxxx>
Date:   Fri Nov 22 09:30:41 2013 -0800

    staging: zsmalloc: Ensure handle is never 0 on success
    
    zsmalloc encodes a handle using the pfn and an object
    index. On hardware platforms with physical memory starting
    at 0x0 the pfn can be 0. This causes the encoded handle to be
    0 and is incorrectly interpreted as an allocation failure.
    
    This issue affects all current and future SoCs with physical
    memory starting at 0x0. All MSM8974 SoCs which includes
    Google Nexus 5 devices are affected.
    
    To prevent this false error we ensure that the encoded handle
    will not be 0 when allocation succeeds.
    
    Signed-off-by: Olav Haugan <ohaugan@xxxxxxxxxxxxxx>
    Cc: stable <stable@xxxxxxxxxxxxxxx>
    Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>


 static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
 {
        unsigned long handle;
@@ -441,17 +446,21 @@ static void *obj_location_to_handle(struct page *page, unsigned long obj_idx)
        }
 
        handle = page_to_pfn(page) << OBJ_INDEX_BITS;
-       handle |= (obj_idx & OBJ_INDEX_MASK);
+       handle |= ((obj_idx + 1) & OBJ_INDEX_MASK);
 
        return (void *)handle;
 }

 static void obj_handle_to_location(unsigned long handle, struct page **page,
                                unsigned long *obj_idx)
 {
        *page = pfn_to_page(handle >> OBJ_INDEX_BITS);
-       *obj_idx = handle & OBJ_INDEX_MASK;
+       *obj_idx = (handle & OBJ_INDEX_MASK) - 1;
 }
--------------------------------------------------------------------------

I guess I'll do the same + 1 / -1 trick.