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

[Cai Liu]

Thanks for your review.

2014/1/21 Minchan Kim <minchan@xxxxxxxxxx>:
> Hello Cai,
>
> On Mon, Jan 20, 2014 at 03:50:18PM +0800, Cai Liu wrote:
>> zswap can support multiple swapfiles. So we need to check
>> all zbud pool pages in zswap.
>>
>> Version 2:
>>   * add *total_zbud_pages* in zbud to record all the pages in pools
>>   * move the updating of pool pages statistics to
>>     alloc_zbud_page/free_zbud_page to hide the details
>>
>> Signed-off-by: Cai Liu <cai.liu@xxxxxxxxxxx>
>> ---
>>  include/linux/zbud.h |    2 +-
>>  mm/zbud.c            |   44 ++++++++++++++++++++++++++++++++------------
>>  mm/zswap.c           |    4 ++--
>>  3 files changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/linux/zbud.h b/include/linux/zbud.h
>> index 2571a5c..1dbc13e 100644
>> --- a/include/linux/zbud.h
>> +++ b/include/linux/zbud.h
>> @@ -17,6 +17,6 @@ void zbud_free(struct zbud_pool *pool, unsigned long handle);
>>  int zbud_reclaim_page(struct zbud_pool *pool, unsigned int retries);
>>  void *zbud_map(struct zbud_pool *pool, unsigned long handle);
>>  void zbud_unmap(struct zbud_pool *pool, unsigned long handle);
>> -u64 zbud_get_pool_size(struct zbud_pool *pool);
>> +u64 zbud_get_pool_size(void);
>>
>>  #endif /* _ZBUD_H_ */
>> diff --git a/mm/zbud.c b/mm/zbud.c
>> index 9451361..711aaf4 100644
>> --- a/mm/zbud.c
>> +++ b/mm/zbud.c
>> @@ -52,6 +52,13 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/zbud.h>
>>
>> +/*********************************
>> +* statistics
>> +**********************************/
>> +
>> +/* zbud pages in all pools */
>> +static u64 total_zbud_pages;
>> +
>>  /*****************
>>   * Structures
>>  *****************/
>> @@ -142,10 +149,28 @@ static struct zbud_header *init_zbud_page(struct page *page)
>>       return zhdr;
>>  }
>>
>> +static struct page *alloc_zbud_page(struct zbud_pool *pool, gfp_t gfp)
>> +{
>> +     struct page *page;
>> +
>> +     page = alloc_page(gfp);
>> +
>> +     if (page) {
>> +             pool->pages_nr++;
>> +             total_zbud_pages++;
>
> Who protect race?

Yes, here the pool->pages_nr and also the total_zbud_pages are not protected.
I will re-do it.

I will change *total_zbud_pages* to atomic type.
For *pool->pages_nr*, one way is to use pool->lock to protect. But I
think it is too heavy.
So does it ok to change pages_nr to atomic type too?


>
>> +     }
>> +
>> +     return page;
>> +}
>> +
>> +
>>  /* Resets the struct page fields and frees the page */
>> -static void free_zbud_page(struct zbud_header *zhdr)
>> +static void free_zbud_page(struct zbud_pool *pool, struct zbud_header *zhdr)
>>  {
>>       __free_page(virt_to_page(zhdr));
>> +
>> +     pool->pages_nr--;
>> +     total_zbud_pages--;
>>  }
>>
>>  /*
>> @@ -279,11 +304,10 @@ int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
>>
>>       /* Couldn't find unbuddied zbud page, create new one */
>>       spin_unlock(&pool->lock);
>> -     page = alloc_page(gfp);
>> +     page = alloc_zbud_page(pool, gfp);
>>       if (!page)
>>               return -ENOMEM;
>>       spin_lock(&pool->lock);
>> -     pool->pages_nr++;
>>       zhdr = init_zbud_page(page);
>>       bud = FIRST;
>>
>> @@ -349,8 +373,7 @@ void zbud_free(struct zbud_pool *pool, unsigned long handle)
>>       if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
>>               /* zbud page is empty, free */
>>               list_del(&zhdr->lru);
>> -             free_zbud_page(zhdr);
>> -             pool->pages_nr--;
>> +             free_zbud_page(pool, zhdr);
>>       } else {
>>               /* Add to unbuddied list */
>>               freechunks = num_free_chunks(zhdr);
>> @@ -447,8 +470,7 @@ next:
>>                        * Both buddies are now free, free the zbud page and
>>                        * return success.
>>                        */
>> -                     free_zbud_page(zhdr);
>> -                     pool->pages_nr--;
>> +                     free_zbud_page(pool, zhdr);
>>                       spin_unlock(&pool->lock);
>>                       return 0;
>>               } else if (zhdr->first_chunks == 0 ||
>> @@ -496,14 +518,12 @@ void zbud_unmap(struct zbud_pool *pool, unsigned long handle)
>>
>>  /**
>>   * zbud_get_pool_size() - gets the zbud pool size in pages
>> - * @pool:    pool whose size is being queried
>>   *
>> - * Returns: size in pages of the given pool.  The pool lock need not be
>> - * taken to access pages_nr.
>> + * Returns: size in pages of all the zbud pools.
>>   */
>> -u64 zbud_get_pool_size(struct zbud_pool *pool)
>> +u64 zbud_get_pool_size(void)
>>  {
>> -     return pool->pages_nr;
>> +     return total_zbud_pages;
>>  }
>>
>>  static int __init init_zbud(void)
>> diff --git a/mm/zswap.c b/mm/zswap.c
>> index 5a63f78..ef44d9d 100644
>> --- a/mm/zswap.c
>> +++ b/mm/zswap.c
>> @@ -291,7 +291,7 @@ static void zswap_free_entry(struct zswap_tree *tree,
>>       zbud_free(tree->pool, entry->handle);
>>       zswap_entry_cache_free(entry);
>>       atomic_dec(&zswap_stored_pages);
>> -     zswap_pool_pages = zbud_get_pool_size(tree->pool);
>> +     zswap_pool_pages = zbud_get_pool_size();
>>  }
>>
>>  /* caller must hold the tree lock */
>> @@ -716,7 +716,7 @@ static int zswap_frontswap_store(unsigned type, pgoff_t offset,
>>
>>       /* update stats */
>>       atomic_inc(&zswap_stored_pages);
>> -     zswap_pool_pages = zbud_get_pool_size(tree->pool);
>> +     zswap_pool_pages = zbud_get_pool_size();
>>
>>       return 0;
>>
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
> --
> 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/