Re: [PATCH 6/7] zsmalloc: fold zs_obj_write() into zs_store()

[Sergey Senozhatsky <senozhatsky@chromium.org>]

On (26/11/05 17:29), Yosry Ahmed wrote:
[..]
> >  Most of the memory allocation APIs use GFP flags to express how that
> >  memory should be allocated. The GFP acronym stands for "get free
> > diff --git a/Documentation/mm/zsmalloc.rst b/Documentation/mm/zsmalloc.rst
> > index dbb5c1a8dcc3..52eaba8f97e9 100644
> > --- a/Documentation/mm/zsmalloc.rst
> > +++ b/Documentation/mm/zsmalloc.rst
> > @@ -20,9 +20,9 @@ For simplicity, zsmalloc can only allocate objects of size up to PAGE_SIZE
> >  since this satisfies the requirements of all its current users (in the
> >  worst case, page is incompressible and is thus stored "as-is" i.e. in
> >  uncompressed form). For allocation requests larger than this size, failure
> > -is returned (see zs_malloc).
> > +is returned (see zs_store).
> >  
> > -Additionally, zs_malloc() does not return a dereferenceable pointer.
> > +Additionally, zs_store() does not return a dereferenceable pointer.
> 
> Well, it doesn't return a pointer at all now.

Ack.

[..]
> > -void zs_obj_write(struct zs_pool *pool, struct zs_handle *handle,
> > -		  void *handle_mem, size_t mem_len)
> > +static void obj_write(struct size_class *class, struct zspage *zspage,
> > +		      unsigned long obj, const void *handle_mem,
> > +		      size_t mem_len)
> 
> Should handle_mem and mem_len be renamed now?

Ack.

> Also, there's a comment in zs_page_migrate() referring to zs_obj_read/write().

Ack.

[..]
> > @@ -1318,12 +1306,16 @@ int zs_malloc(struct zs_pool *pool, struct zs_handle *handle, size_t size,
> >  	spin_lock(&class->lock);
> >  	zspage = find_get_zspage(class);
> >  	if (likely(zspage)) {
> > -		obj_malloc(pool, zspage, handle);
> > +		obj = obj_malloc(pool, zspage, handle);
> >  		/* Now move the zspage to another fullness group, if required */
> >  		fix_fullness_group(class, zspage);
> >  		class_stat_add(class, ZS_OBJS_INUSE, 1);
> > +		zspage_read_lock(zspage);
> 
> The zspage lock is now acquired under the class lock instead of the pool
> lock, this is probably fine but worth calling out explicitly in the
> changelog.

Let me take a close look.  Lockdep didn't complain, but let me look more
into it.

> > +		spin_unlock(&class->lock);
> >  
> > -		goto out;
> > +		obj_write(class, zspage, obj, src, size);
> > +		zspage_read_unlock(zspage);
> > +		return 0;
> >  	}
> >  
> >  	spin_unlock(&class->lock);
> > @@ -1332,8 +1324,14 @@ int zs_malloc(struct zs_pool *pool, struct zs_handle *handle, size_t size,
> >  	if (!zspage)
> >  		return -ENOMEM;
> >  
> > +	/*
> > +	 * New zspage is not in class->fullness_list or movable yet, so we
> > +	 * can allocate the object and copy data before taking class->lock.
> > +	 */
> > +	obj = obj_malloc(pool, zspage, handle);
> > +	obj_write(class, zspage, obj, src, size);
> > +
> >  	spin_lock(&class->lock);
> > -	obj_malloc(pool, zspage, handle);
> 
> Should moving obj_malloc() and obj_write() outside class->lock (and
> skipping zspage_read_lock()) for new zspages be split into a separate
> patch on top? AFAICT, it's a separate change from merging the
> interfaces.

Let me take a look.