Re: [PATCHv5 7/8] zswap: add swap page writeback support

[Seth Jennings]

On 02/24/2013 08:54 PM, Minchan Kim wrote:
> Hi Seth,
> 
> On Wed, Feb 13, 2013 at 12:38:50PM -0600, Seth Jennings wrote:
>> This patch adds support for evicting swap pages that are currently
>> compressed in zswap to the swap device.  This functionality is very
>> important and make zswap a true cache in that, once the cache is full
>> or can't grow due to memory pressure, the oldest pages can be moved
>> out of zswap to the swap device so newer pages can be compressed and
>> stored in zswap.
>>
>> This introduces a good amount of new code to guarantee coherency.
>> Most notably, and LRU list is added to the zswap_tree structure,
>> and refcounts are added to each entry to ensure that one code path
>> doesn't free then entry while another code path is operating on it.
>>
>> Signed-off-by: Seth Jennings <sjenning@xxxxxxxxxxxxxxxxxx>
> 
> In this time, I didn't review the code in detail yet but it seems
> resolve of all review point in previous interation. Thanks!
> But unfortunately, I couldn't find anything related to tmppage handling
> so I'd like to ask.
> 
> The reason of tmppage is temporal buffer to keep compressed data during
> writeback to avoid unnecessary compressing again when we retry?

Yes.

> Is it really critical about performance?

It's hard to measure.  There is no guarantee that
zswap_flush_entries() has made room for the allocation so if we fail
again, we've compressed the page twice and still fail

So my motivation was to prevent the second compression.  It does add
significant complexity though without a completely clear (i.e.
measurable) benefit.


What's the wrong if we remove
> tmppage handling?
> 
> zswap_frontswap_store
> retry:
>         get_cpu_var(zswap_dstmem);
>         zswap_com_op(COMPRESS)
>         zs_malloc()
>         if (!handle) {
>                 put_cpu_var(zswap_dstmem);
>                 if (retry > MAX_RETRY)
>                         goto error_nomem;
>                 zswap_flush_entries()
>                 goto retry;
>         }

I dislike "jump up" labels, but yes, something like this could be done.

Thanks,
Seth

--
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/