Re: [PATCH 3/4] zsmalloc use zs_handle instead of void *

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> Please look.
> 
> struct zs_handle {
> 	void *handle
> };
> 
> 1)
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return handle;

Compiler will complain that you are returning incorrect type.

> }
> 
> handle is on stack so it can't be used by index for slot of radix tree.

The fix is of course to return a pointer (which your function
declared), and instead do this:

{
	struct zs_handle *handle;

	handle = zs_malloc(pool, size);
	return handle;
}

> 
> 2)
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return handle.handle;
> }
> 
> Okay. Now it works but zcache coupled with zsmalloc tightly.
> User of zsmalloc should never know internal of zs_handle.

OK. Then it can just forward declare it:

struct zs_handle;

and zsmalloc will treat it as an opaque pointer.

> 
> 3)
> 
> - zsmalloc.h
> void *zs_handle_to_ptr(struct zs_handle handle)
> {
> 	return handle.hanle;
> }
> 
> static struct zv_hdr *zv_create(..)
> {
> 	struct zs_handle handle;
> 	..
> 	handle = zs_malloc(pool, size);
> 	..
> 	return zs_handle_to_ptr(handle);

> }

> 
> Why should zsmalloc support such interface?

Why not? It is better than a 'void *' or a typedef.

It is modeled after a pte_t.


> It's a zcache problem so it's desriable to solve it in zcache internal.

Not really. We shouldn't really pass any 'void *' pointers around.

> And in future, if we can add/remove zs_handle's fields, we can't make
> sure such API.

Meaning ... what exactly do you mean? That the size of the structure
will change and we won't return the right value? Why not?
If you use the 'zs_handle_to_ptr' won't that work? Especially if you
add new values to the end of the struct it won't cause issues.

> 
> 
> >> Its true that making it a real struct would prevent accidental casts
> >> to void * but due to the above problem, I think we have to stick
> >> with unsigned long.

So the problem you are seeing is that you don't want 'struct zs_handle'
be present in the drivers/staging/zsmalloc/zsmalloc.h header file?
It looks like the proper place.