Re: [PATCH 2/2] zram: clean up handle

[Nitin Gupta]

On 06/07/2012 01:47 PM, Dan Magenheimer wrote:

>> From: Minchan Kim [mailto:minchan@xxxxxxxxxx]
>> Subject: Re: [PATCH 2/2] zram: clean up handle
>>
>> On 06/06/2012 02:04 PM, Nitin Gupta wrote:
>>
>>> On 06/05/2012 12:23 AM, Minchan Kim wrote:
>>>
>>>> zram's handle variable can store handle of zsmalloc in case of
>>>> compressing efficiently. Otherwise, it stores point of page descriptor.
>>>> This patch clean up the mess by union struct.
>>>>
>>>> changelog
>>>>   * from v1
>>>> 	- none(new add in v2)
>>>>
>>>> Cc: Nitin Gupta <ngupta@xxxxxxxxxx>
>>>> Acked-by: Seth Jennings <sjenning@xxxxxxxxxxxxxxxxxx>
>>>> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>>> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
>>>> ---
>>>>  drivers/staging/zram/zram_drv.c |   77 ++++++++++++++++++++-------------------
>>>>  drivers/staging/zram/zram_drv.h |    5 ++-
>>>>  2 files changed, 44 insertions(+), 38 deletions(-)
>>>
>>> I think page vs handle distinction was added since xvmalloc could not
>>> handle full page allocation. Now that zsmalloc allows full page
>>
>> I see. I didn't know that because I'm blind on xvmalloc.
>>
>>> allocation, we can just use it for both cases. This would also allow
>>> removing the ZRAM_UNCOMPRESSED flag. The only downside will be slightly
>>> slower code path for full page allocation but this event is anyways
>>> supposed to be rare, so should be fine.
>>
>> Fair enough.
>> It can remove many code of zram.
>> Okay. Will look into that.
> 
> Nitin, can zsmalloc allow full page allocation by assigning
> an actual physical pageframe (which is what zram does now)?
> Or will it allocate PAGE_SIZE bytes which zsmalloc will allocate
> crossing a page boundary which, presumably, will have much worse
> impact on page allocator availability when these pages are
> "reclaimed" via your swap notify callback.
> 


zsmalloc does not add any object headers, so when allocating PAGE_SIZE
you get a separate page from as if you did alloc_page(). So, it does not
span page boundaries.


> Though this may be rare across all workloads, it may turn out
> to be very common for certain workloads (e.g. if the workload
> has many dirty anonymous pages that are already compressed
> by userland).
> 
> It may not be worth cleaning up the code if it causes
> performance issues with this case.
> 
> And anyway can zsmalloc handle and identify to the caller pages
> that are both compressed and "native" (uncompressed)?  It
> certainly has to handle both if you remove ZRAM_UNCOMPRESSED
> as compressing some pages actually results in more than
> PAGE_SIZE bytes.  So you need to record somewhere that
> this "compressed page" is special and that must somehow
> be communicated to the caller of your "get" routine.
> 
> (Just trying to save Minchan from removing all that code but
> then needing to add it back again.)
> 


zsmalloc cannot identify compressed vs uncompressed pages. However, in
zram, we can tell if the page is uncompressed using table[i]->size which
is set to PAGE_SIZE for uncompressed pages.   Pages that compress to
more than PAGE_SIZE (i.e. expand on compression) are stored
as-is/uncompressed and thus will have size field set to PAGE_SIZE.

Thus, we do not require ZRAM_UNCOMPRESSED flag when using zsmalloc for
both compressed and uncompressed pages.

Thanks,
Nitin

Thanks,
Nitin
--
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/