On 05/12/2012 04:28 AM, Konrad Rzeszutek Wilk wrote:
>> 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.
My bad. &handle.
>
>> }
>>
>> 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);
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?
It's not good abstraction.
> 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 not same with pte_t.
We normally don't use pte_val to (void*) for unique index of slot.
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*).
>
>
>> 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?)
struct zs_handle {
int upper;
int middle;
int lower;
};
How could you handle this for zs_handle_to_ptr?
>
>>
>>
>>>> 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.
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.
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>