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

[Konrad Rzeszutek Wilk]

>>
>> 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);
>
>
> It's not a good idea.
> For it, zs_malloc needs memory space to keep zs_handle internally.
> Why should zsallocator do it? Just for zcache?

How different is from now? The zs_malloc keeps the handle internally
as well - it just that is is a void * pointer. Internally, the
ownership and the responsibility to free it lays with zsmalloc.

> It's not good abstraction.

If we want good abstraction, then I don't think 'unsigned long' is
either? I mean it will do for the conversion from 'void *'. Perhaps I
am being a bit optimistic here - and I am trying to jam in this
'struct zs_handle' in all cases but in reality it needs a more
iterative process. So first do 'void *' -> 'unsigned long', and then
later on if we can come up with something more nicely that abstracts
- then use that?
.. snip ..
>>> 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 not same with pte_t.
> We normally don't use pte_val to (void*) for unique index of slot.

Right, but I thought we want to get rid of all of the '(void *)'
usages and instead
pass some opaque pointer.

> The problem is that zcache assume handle of zsmalloc is a sizeof(void*)'s
> unique value but zcache never assume it's a sizeof(void*).

Huh? I am parsing your sentence as: "zcache assumes .. sizeof(void *),
but zcache never assumes its .. sizeof(void *)"?

Zcache has to assume it is a pointer. And providing a 'struct
zs_handle *' would fit the bill?
>>
>>
>>> 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.
>
>
> I mean we might change zs_handle to following as, in future.
> (It's insane but who know it?)

OK, so BUILD_BUG(sizeof(struct zs_handle *) != sizeof(void *))
with a big fat comment saying that one needs to go over the other users
of zcache/zram/zsmalloc to double check?

But why would it matter? The zs_handle would be returned as a pointer
- so the size is the same to the caller.

>
> struct zs_handle {
>        int upper;
>        int middle;
>        int lower;
> };
>
> How could you handle this for zs_handle_to_ptr?

Gosh, um, I couldn't :-) Well, maybe with something that does
 return "upper | middle | lower", but yeah that is not the goal.


>>>>> 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.
>
>
> No. What I want is to remove coupling zsallocator's handle with zram/zcache.
> They shouldn't know internal of handle and assume it's a pointer.

I concur. And hence I was thinking that the 'struct zs_handle *'
pointer would work.

>
> If Nitin confirm zs_handle's format can never change in future, I prefer "unsigned long" Nitin suggested than (void *).
> It can prevent confusion that normal allocator's return value is pointer for address so the problem is easy.
> But I am not sure he can make sure it.

Well, everything changes over time  so putting a stick in the ground
and saying 'this must
be this way' is not really the best way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/