On Thu, Nov 3, 2016 at 5:04 PM, Vitaly Wool <vitalywool@xxxxxxxxx> wrote:
> z3fold_compact_page() currently only handles the situation when
> there's a single middle chunk within the z3fold page. However it
> may be worth it to move middle chunk closer to either first or
> last chunk, whichever is there, if the gap between them is big
> enough.
>
> This patch adds the relevant code, using BIG_CHUNK_GAP define as
> a threshold for middle chunk to be worth moving.
>
> Signed-off-by: Vitaly Wool <vitalywool@xxxxxxxxx>
with the bikeshedding comments below, looks good.
Acked-by: Dan Streetman <ddstreet@xxxxxxxx>
> ---
> mm/z3fold.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 4d02280..fea6791 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -250,26 +250,60 @@ static void z3fold_destroy_pool(struct z3fold_pool *pool)
> kfree(pool);
> }
>
> +static inline void *mchunk_memmove(struct z3fold_header *zhdr,
> + unsigned short dst_chunk)
> +{
> + void *beg = zhdr;
> + return memmove(beg + (dst_chunk << CHUNK_SHIFT),
> + beg + (zhdr->start_middle << CHUNK_SHIFT),
> + zhdr->middle_chunks << CHUNK_SHIFT);
> +}
> +
> +#define BIG_CHUNK_GAP 3
> /* Has to be called with lock held */
> static int z3fold_compact_page(struct z3fold_header *zhdr)
> {
> struct page *page = virt_to_page(zhdr);
> - void *beg = zhdr;
> + int ret = 0;
> +
> + if (test_bit(MIDDLE_CHUNK_MAPPED, &page->private))
> + goto out;
>
> + if (zhdr->middle_chunks != 0) {
bikeshed: this check could be moved up also, as if there's no middle
chunk there is no compacting to do and we can just return 0. saves a
tab in all the code below.
> + if (zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
> + mchunk_memmove(zhdr, 1); /* move to the beginning */
> + zhdr->first_chunks = zhdr->middle_chunks;
> + zhdr->middle_chunks = 0;
> + zhdr->start_middle = 0;
> + zhdr->first_num++;
> + ret = 1;
> + goto out;
> + }
>
> - if (!test_bit(MIDDLE_CHUNK_MAPPED, &page->private) &&
> - zhdr->middle_chunks != 0 &&
> - zhdr->first_chunks == 0 && zhdr->last_chunks == 0) {
> - memmove(beg + ZHDR_SIZE_ALIGNED,
> - beg + (zhdr->start_middle << CHUNK_SHIFT),
> - zhdr->middle_chunks << CHUNK_SHIFT);
> - zhdr->first_chunks = zhdr->middle_chunks;
> - zhdr->middle_chunks = 0;
> - zhdr->start_middle = 0;
> - zhdr->first_num++;
> - return 1;
> + /*
> + * moving data is expensive, so let's only do that if
> + * there's substantial gain (at least BIG_CHUNK_GAP chunks)
> + */
> + if (zhdr->first_chunks != 0 && zhdr->last_chunks == 0 &&
> + zhdr->start_middle > zhdr->first_chunks + BIG_CHUNK_GAP) {
> + mchunk_memmove(zhdr, zhdr->first_chunks + 1);
> + zhdr->start_middle = zhdr->first_chunks + 1;
> + ret = 1;
> + goto out;
> + }
> + if (zhdr->last_chunks != 0 && zhdr->first_chunks == 0 &&
> + zhdr->middle_chunks + zhdr->last_chunks <=
> + NCHUNKS - zhdr->start_middle - BIG_CHUNK_GAP) {
> + unsigned short new_start = NCHUNKS - zhdr->last_chunks -
> + zhdr->middle_chunks;
> + mchunk_memmove(zhdr, new_start);
> + zhdr->start_middle = new_start;
> + ret = 1;
> + goto out;
> + }
> }
> - return 0;
> +out:
> + return ret;
bikeshed: do we need all the goto out? why not just return 0/1
appropriately above?
> }
>
> /**
> --
> 2.4.2
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>