Re: [PATCH v2] zbud: allow up to PAGE_SIZE allocations

[Vitaly Wool <vitalywool@gmail.com>]

Hello Seth,

On Thu, Sep 24, 2015 at 12:41 AM, Seth Jennings
<sjennings@variantweb.net> wrote:
> On Wed, Sep 23, 2015 at 10:59:00PM +0200, Vitaly Wool wrote:
>> Okay, how about this? It's gotten smaller BTW :)
>>
>> zbud: allow up to PAGE_SIZE allocations
>>
>> Currently zbud is only capable of allocating not more than
>> PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE. This is okay as
>> long as only zswap is using it, but other users of zbud may
>> (and likely will) want to allocate up to PAGE_SIZE. This patch
>> addresses that by skipping the creation of zbud internal
>> structure in the beginning of an allocated page. As a zbud page
>> is no longer guaranteed to contain zbud header, the following
>> changes have to be applied throughout the code:
>> * page->lru to be used for zbud page lists
>> * page->private to hold 'under_reclaim' flag
>>
>> page->private will also be used to indicate if this page contains
>> a zbud header in the beginning or not ('headless' flag).
>>
>> Signed-off-by: Vitaly Wool <vitalywool@gmail.com>
>> ---
>>  mm/zbud.c | 167 ++++++++++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 113 insertions(+), 54 deletions(-)
>>
>> diff --git a/mm/zbud.c b/mm/zbud.c
>> index fa48bcdf..3946fba 100644
>> --- a/mm/zbud.c
>> +++ b/mm/zbud.c
>> @@ -105,18 +105,20 @@ struct zbud_pool {
>>
>>  /*
>>   * struct zbud_header - zbud page metadata occupying the first chunk of each
>> - *                   zbud page.
>> + *                   zbud page, except for HEADLESS pages
>>   * @buddy:   links the zbud page into the unbuddied/buddied lists in the pool
>> - * @lru:     links the zbud page into the lru list in the pool
>>   * @first_chunks:    the size of the first buddy in chunks, 0 if free
>>   * @last_chunks:     the size of the last buddy in chunks, 0 if free
>>   */
>>  struct zbud_header {
>>       struct list_head buddy;
>> -     struct list_head lru;
>>       unsigned int first_chunks;
>>       unsigned int last_chunks;
>> -     bool under_reclaim;
>> +};
>> +
>> +enum zbud_page_flags {
>> +     UNDER_RECLAIM = 0,
>
> Don't need the "= 0"
>
>> +     PAGE_HEADLESS,
>
> Also I think we should prefix the enum values here. With ZPF_ ?
>
>>  };
>>
>>  /*****************
>> @@ -221,6 +223,7 @@ MODULE_ALIAS("zpool-zbud");
>>  *****************/
>>  /* Just to make the code easier to read */
>>  enum buddy {
>> +     HEADLESS,
>>       FIRST,
>>       LAST
>>  };
>> @@ -238,11 +241,14 @@ static int size_to_chunks(size_t size)
>>  static struct zbud_header *init_zbud_page(struct page *page)
>>  {
>>       struct zbud_header *zhdr = page_address(page);
>> +
>> +     INIT_LIST_HEAD(&page->lru);
>> +     clear_bit(UNDER_RECLAIM, &page->private);
>> +     clear_bit(HEADLESS, &page->private);
>
> I know we are using private in a bitwise flags mode, but maybe we
> should just init with page->private = 0
>
>> +
>>       zhdr->first_chunks = 0;
>>       zhdr->last_chunks = 0;
>>       INIT_LIST_HEAD(&zhdr->buddy);
>> -     INIT_LIST_HEAD(&zhdr->lru);
>> -     zhdr->under_reclaim = 0;
>>       return zhdr;
>>  }
>>
>> @@ -267,11 +273,22 @@ static unsigned long encode_handle(struct zbud_header *zhdr, enum buddy bud)
>>        * over the zbud header in the first chunk.
>>        */
>>       handle = (unsigned long)zhdr;
>> -     if (bud == FIRST)
>> +     switch (bud) {
>> +     case FIRST:
>>               /* skip over zbud header */
>>               handle += ZHDR_SIZE_ALIGNED;
>> -     else /* bud == LAST */
>> +             break;
>> +     case LAST:
>>               handle += PAGE_SIZE - (zhdr->last_chunks  << CHUNK_SHIFT);
>> +             break;
>> +     case HEADLESS:
>> +             break;
>> +     default:
>> +             /* this should never happen */
>> +             pr_err("zbud: invalid buddy value %d\n", bud);
>> +             handle = 0;
>> +             break;
>> +     }
>
> Don't need this default case since we have a case for each valid value
> of the enum.
>
> Also, I think we want to add some code to free_zbud_page() to clear
> page->private and init page->lru so we don't leave dangling pointers.

Right, maybe it makes sense for free_zbud_page() to take struct page
pointer as an argument, too, to minimize back-and-forth conversions?

> Looks good though :)

Thanks, I'll come up with the new version shortly. :)

~vitaly

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>