All of lore.kernel.org
 help / color / mirror / Atom feed
From: Heesub Shin <heesub.shin@samsung.com>
To: Seth Jennings <sjennings@variantweb.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Nitin Gupta <ngupta@vflare.org>,
	Dan Streetman <ddstreet@ieee.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Sunae Seo <sunae.seo@samsung.com>
Subject: Re: [RFC PATCH 0/9] mm/zbud: support highmem pages
Date: Wed, 28 Jan 2015 16:14:04 +0900	[thread overview]
Message-ID: <54C88C3C.3010604@samsung.com> (raw)
In-Reply-To: <20150127202440.GA13103@cerebellum.variantweb.net>



On 01/28/2015 05:24 AM, Seth Jennings wrote:
> On Tue, Nov 04, 2014 at 10:33:43AM -0600, Seth Jennings wrote:
>> On Tue, Oct 14, 2014 at 08:59:19PM +0900, Heesub Shin wrote:
>>> zbud is a memory allocator for storing compressed data pages. It keeps
>>> two data objects of arbitrary size on a single page. This simple design
>>> provides very deterministic behavior on reclamation, which is one of
>>> reasons why zswap selected zbud as a default allocator over zsmalloc.
>>>
>>> Unlike zsmalloc, however, zbud does not support highmem. This is
>>> problomatic especially on 32-bit machines having relatively small
>>> lowmem. Compressing anonymous pages from highmem and storing them into
>>> lowmem could eat up lowmem spaces.
>>>
>>> This limitation is due to the fact that zbud manages its internal data
>>> structures on zbud_header which is kept in the head of zbud_page. For
>>> example, zbud_pages are tracked by several lists and have some status
>>> information, which are being referenced at any time by the kernel. Thus,
>>> zbud_pages should be allocated on a memory region directly mapped,
>>> lowmem.
>>>
>>> After some digging out, I found that internal data structures of zbud
>>> can be kept in the struct page, the same way as zsmalloc does. So, this
>>> series moves out all fields in zbud_header to struct page. Though it
>>> alters quite a lot, it does not add any functional differences except
>>> highmem support. I am afraid that this kind of modification abusing
>>> several fields in struct page would be ok.
>>
>> Hi Heesub,
>>
>> Sorry for the very late reply.  The end of October was very busy for me.
>>
>> A little history on zbud.  I didn't put the metadata in the struct
>> page, even though I knew that was an option since we had done it with
>> zsmalloc. At the time, Andrew Morton had concerns about memmap walkers
>> getting messed up with unexpected values in the struct page fields.  In
>> order to smooth zbud's acceptance, I decided to store the metadata
>> inline in the page itself.
>>
>> Later, zsmalloc eventually got accepted, which basically gave the
>> impression that putting the metadata in the struct page was acceptable.
>>
>> I have recently been looking at implementing compaction for zsmalloc,
>> but having the metadata in the struct page and having the handle
>> directly encode the PFN and offset of the data block prevents
>> transparent relocation of the data. zbud has a similar issue as it
>> currently encodes the page address in the handle returned to the user
>> (also the limitation that is preventing use of highmem pages).
>>
>> I would like to implement compaction for zbud too and moving the
>> metadata into the struct page is going to work against that. In fact,
>> I'm looking at the option of converting the current zbud_header into a
>> per-allocation metadata structure, which would provide a layer of
>> indirection between zbud and the user, allowing for transparent
>> relocation and compaction.
>
> I had some downtime and started thinking about this again today (after
> 3 months).
>
> Upon further reflection, I really like this and don't think that it
> inhibits introducing compaction later.
>
> There are just a few places that look messy or problematic to me:
>
> 1. the use of page->private and masking the number of chunks for both
> buddies into it (see suggestion for overlay struct below)
> 2. the use of the second double word &page->index to store a list_head
>
> #2 might be problematic because, IIRC, memmap walkers will check _count
> (or _mapcount).  I think we ran into this in zsmalloc.
>
> Initially, when working on zsmalloc, I just created a structure that
> overlaid the struct page in the memmap, reserving the flags and _count
> areas, so that I wouldn't have to be bound by the field names/boundaries
> in the struct page.
>
> IIRC, Andrew was initially against that, but he was also against the
> whole idea of using the struct page fields for random stuff... I that
> ended up being accepted.
>
> This code looks really good!  I think with a little cleanup and finding
> a way to steer clear of using the _count part of the structure, this
> will be great.

Thanks for your comments! I will try to address problems you pointed and 
post a new patchset hopefully soon.

regards,
heesub

>
> Sorry for dismissing it earlier.  Didn't give it enough credit.
>
> Thanks,
> Seth
>
>>
>> However, I do like the part about letting zbud use highmem pages.
>>
>> I have something in mind that would allow highmem pages _and_ move
>> toward something that would support compaction.  I'll see if I can put
>> it into code today.
>>
>> Thanks,
>> Seth
>>
>>>
>>> Heesub Shin (9):
>>>    mm/zbud: tidy up a bit
>>>    mm/zbud: remove buddied list from zbud_pool
>>>    mm/zbud: remove lru from zbud_header
>>>    mm/zbud: remove first|last_chunks from zbud_header
>>>    mm/zbud: encode zbud handle using struct page
>>>    mm/zbud: remove list_head for buddied list from zbud_header
>>>    mm/zbud: drop zbud_header
>>>    mm/zbud: allow clients to use highmem pages
>>>    mm/zswap: use highmem pages for compressed pool
>>>
>>>   mm/zbud.c  | 244 ++++++++++++++++++++++++++++++-------------------------------
>>>   mm/zswap.c |   4 +-
>>>   2 files changed, 121 insertions(+), 127 deletions(-)
>>>
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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>
>>
>> --
>> 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>
>
> --
> 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>
>

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

WARNING: multiple messages have this Message-ID (diff)
From: Heesub Shin <heesub.shin@samsung.com>
To: Seth Jennings <sjennings@variantweb.net>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Nitin Gupta <ngupta@vflare.org>,
	Dan Streetman <ddstreet@ieee.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Sunae Seo <sunae.seo@samsung.com>
Subject: Re: [RFC PATCH 0/9] mm/zbud: support highmem pages
Date: Wed, 28 Jan 2015 16:14:04 +0900	[thread overview]
Message-ID: <54C88C3C.3010604@samsung.com> (raw)
In-Reply-To: <20150127202440.GA13103@cerebellum.variantweb.net>



On 01/28/2015 05:24 AM, Seth Jennings wrote:
> On Tue, Nov 04, 2014 at 10:33:43AM -0600, Seth Jennings wrote:
>> On Tue, Oct 14, 2014 at 08:59:19PM +0900, Heesub Shin wrote:
>>> zbud is a memory allocator for storing compressed data pages. It keeps
>>> two data objects of arbitrary size on a single page. This simple design
>>> provides very deterministic behavior on reclamation, which is one of
>>> reasons why zswap selected zbud as a default allocator over zsmalloc.
>>>
>>> Unlike zsmalloc, however, zbud does not support highmem. This is
>>> problomatic especially on 32-bit machines having relatively small
>>> lowmem. Compressing anonymous pages from highmem and storing them into
>>> lowmem could eat up lowmem spaces.
>>>
>>> This limitation is due to the fact that zbud manages its internal data
>>> structures on zbud_header which is kept in the head of zbud_page. For
>>> example, zbud_pages are tracked by several lists and have some status
>>> information, which are being referenced at any time by the kernel. Thus,
>>> zbud_pages should be allocated on a memory region directly mapped,
>>> lowmem.
>>>
>>> After some digging out, I found that internal data structures of zbud
>>> can be kept in the struct page, the same way as zsmalloc does. So, this
>>> series moves out all fields in zbud_header to struct page. Though it
>>> alters quite a lot, it does not add any functional differences except
>>> highmem support. I am afraid that this kind of modification abusing
>>> several fields in struct page would be ok.
>>
>> Hi Heesub,
>>
>> Sorry for the very late reply.  The end of October was very busy for me.
>>
>> A little history on zbud.  I didn't put the metadata in the struct
>> page, even though I knew that was an option since we had done it with
>> zsmalloc. At the time, Andrew Morton had concerns about memmap walkers
>> getting messed up with unexpected values in the struct page fields.  In
>> order to smooth zbud's acceptance, I decided to store the metadata
>> inline in the page itself.
>>
>> Later, zsmalloc eventually got accepted, which basically gave the
>> impression that putting the metadata in the struct page was acceptable.
>>
>> I have recently been looking at implementing compaction for zsmalloc,
>> but having the metadata in the struct page and having the handle
>> directly encode the PFN and offset of the data block prevents
>> transparent relocation of the data. zbud has a similar issue as it
>> currently encodes the page address in the handle returned to the user
>> (also the limitation that is preventing use of highmem pages).
>>
>> I would like to implement compaction for zbud too and moving the
>> metadata into the struct page is going to work against that. In fact,
>> I'm looking at the option of converting the current zbud_header into a
>> per-allocation metadata structure, which would provide a layer of
>> indirection between zbud and the user, allowing for transparent
>> relocation and compaction.
>
> I had some downtime and started thinking about this again today (after
> 3 months).
>
> Upon further reflection, I really like this and don't think that it
> inhibits introducing compaction later.
>
> There are just a few places that look messy or problematic to me:
>
> 1. the use of page->private and masking the number of chunks for both
> buddies into it (see suggestion for overlay struct below)
> 2. the use of the second double word &page->index to store a list_head
>
> #2 might be problematic because, IIRC, memmap walkers will check _count
> (or _mapcount).  I think we ran into this in zsmalloc.
>
> Initially, when working on zsmalloc, I just created a structure that
> overlaid the struct page in the memmap, reserving the flags and _count
> areas, so that I wouldn't have to be bound by the field names/boundaries
> in the struct page.
>
> IIRC, Andrew was initially against that, but he was also against the
> whole idea of using the struct page fields for random stuff... I that
> ended up being accepted.
>
> This code looks really good!  I think with a little cleanup and finding
> a way to steer clear of using the _count part of the structure, this
> will be great.

Thanks for your comments! I will try to address problems you pointed and 
post a new patchset hopefully soon.

regards,
heesub

>
> Sorry for dismissing it earlier.  Didn't give it enough credit.
>
> Thanks,
> Seth
>
>>
>> However, I do like the part about letting zbud use highmem pages.
>>
>> I have something in mind that would allow highmem pages _and_ move
>> toward something that would support compaction.  I'll see if I can put
>> it into code today.
>>
>> Thanks,
>> Seth
>>
>>>
>>> Heesub Shin (9):
>>>    mm/zbud: tidy up a bit
>>>    mm/zbud: remove buddied list from zbud_pool
>>>    mm/zbud: remove lru from zbud_header
>>>    mm/zbud: remove first|last_chunks from zbud_header
>>>    mm/zbud: encode zbud handle using struct page
>>>    mm/zbud: remove list_head for buddied list from zbud_header
>>>    mm/zbud: drop zbud_header
>>>    mm/zbud: allow clients to use highmem pages
>>>    mm/zswap: use highmem pages for compressed pool
>>>
>>>   mm/zbud.c  | 244 ++++++++++++++++++++++++++++++-------------------------------
>>>   mm/zswap.c |   4 +-
>>>   2 files changed, 121 insertions(+), 127 deletions(-)
>>>
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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>
>>
>> --
>> 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>
>
> --
> 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>
>

  reply	other threads:[~2015-01-28  7:13 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-10-14 11:59 [RFC PATCH 0/9] mm/zbud: support highmem pages Heesub Shin
2014-10-14 11:59 ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 1/9] mm/zbud: tidy up a bit Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 2/9] mm/zbud: remove buddied list from zbud_pool Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 3/9] mm/zbud: remove lru from zbud_header Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 4/9] mm/zbud: remove first|last_chunks " Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 5/9] mm/zbud: encode zbud handle using struct page Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 6/9] mm/zbud: remove list_head for buddied list from zbud_header Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 7/9] mm/zbud: drop zbud_header Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 8/9] mm/zbud: allow clients to use highmem pages Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-14 11:59 ` [RFC PATCH 9/9] mm/zswap: use highmem pages for compressed pool Heesub Shin
2014-10-14 11:59   ` Heesub Shin
2014-10-23 23:14 ` [RFC PATCH 0/9] mm/zbud: support highmem pages Dan Streetman
2014-10-23 23:14   ` Dan Streetman
2014-10-24 15:27   ` Seth Jennings
2014-10-24 15:27     ` Seth Jennings
2014-11-04 16:33 ` Seth Jennings
2014-11-04 16:33   ` Seth Jennings
2015-01-27 20:24   ` Seth Jennings
2015-01-27 20:24     ` Seth Jennings
2015-01-28  7:14     ` Heesub Shin [this message]
2015-01-28  7:14       ` Heesub Shin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54C88C3C.3010604@samsung.com \
    --to=heesub.shin@samsung.com \
    --cc=akpm@linux-foundation.org \
    --cc=ddstreet@ieee.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=ngupta@vflare.org \
    --cc=sjennings@variantweb.net \
    --cc=sunae.seo@samsung.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.