linux-kernel - Re: [PATCH v2] zbud: allow up to PAGE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMJBoFPwAq+ywq5g=a8mtis6w4Qe9gZobVMv1SZExhg1_bWBzw@mail.gmail.com>
Date:	Fri, 25 Sep 2015 07:56:17 +0200
From:	Vitaly Wool <vitalywool@...il.com>
To:	Seth Jennings <sjennings@...iantweb.net>
Cc:	Dan Streetman <ddstreet@...e.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan@...nel.org>,
	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Linux-MM <linux-mm@...ck.org>
Subject: Re: [PATCH v2] zbud: allow up to PAGE_SIZE allocations

Hello Seth,

On Thu, Sep 24, 2015 at 12:41 AM, Seth Jennings
<sjennings@...iantweb.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@...il.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/