Re: [PATCH] mm/zswap: Check all pool pages instead of one pool pages

[Minchan Kim]

On Tue, Jan 14, 2014 at 02:15:44PM +0800, Weijie Yang wrote:
> On Tue, Jan 14, 2014 at 1:42 PM, Bob Liu <bob.liu@xxxxxxxxxx> wrote:
> >
> > On 01/14/2014 01:05 PM, Minchan Kim wrote:
> >> On Tue, Jan 14, 2014 at 01:50:22PM +0900, Minchan Kim wrote:
> >>> Hello Bob,
> >>>
> >>> On Tue, Jan 14, 2014 at 09:19:23AM +0800, Bob Liu wrote:
> >>>>
> >>>> On 01/14/2014 07:35 AM, Minchan Kim wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On Sat, Jan 11, 2014 at 03:43:07PM +0800, Cai Liu wrote:
> >>>>>> zswap can support multiple swapfiles. So we need to check
> >>>>>> all zbud pool pages in zswap.
> >>>>>
> >>>>> True but this patch is rather costly that we should iterate
> >>>>> zswap_tree[MAX_SWAPFILES] to check it. SIGH.
> >>>>>
> >>>>> How about defining zswap_tress as linked list instead of static
> >>>>> array? Then, we could reduce unnecessary iteration too much.
> >>>>>
> >>>>
> >>>> But if use linked list, it might not easy to access the tree like this:
> >>>> struct zswap_tree *tree = zswap_trees[type];
> >>>
> >>> struct zswap_tree {
> >>>     ..
> >>>     ..
> >>>     struct list_head list;
> >>> }
> >>>
> >>> zswap_frontswap_init()
> >>> {
> >>>     ..
> >>>     ..
> >>>     zswap_trees[type] = tree;
> >>>     list_add(&tree->list, &zswap_list);
> >>> }
> >>>
> >>> get_zswap_pool_pages(void)
> >>> {
> >>>     struct zswap_tree *cur;
> >>>     list_for_each_entry(cur, &zswap_list, list) {
> >>>         pool_pages += zbud_get_pool_size(cur->pool);
> >>>     }
> >>>     return pool_pages;
> >>> }
> >
> > Okay, I see your point. Yes, it's much better.
> > Cai, Please make an new patch.
> 
> This improved patch could reduce unnecessary iteration too much.
> 
> But I still have a question: why do we need so many zbud pools?
> How about use only one global zbud pool for all zswap_tree?
> I do not test it, but I think it can improve the strore density.

Just a quick glance,

I don't know how multiple swap configuration is popular?
With your approach, what kinds of change do we need in frontswap_invalidate_area?
You will add encoded *type* in offset of entry?
So we always should decode it when we need search opeartion?
We lose speed but get a density(? but not sure because it's dependent on workload)
for rare configuration(ie, multiple swap) and rare event(ie, swapoff).
It's just popped question, not strong objection.
Anyway, point is that you can try it if you want and then, report the number. :)

Thanks.

> 
> Just for your reference, Thanks!
> 
-- 
Kind regards,
Minchan Kim
--
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/