Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754708AbcKYOp2 (ORCPT ); Fri, 25 Nov 2016 09:45:28 -0500 Received: from mail-lf0-f66.google.com ([209.85.215.66]:36028 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754265AbcKYOpR (ORCPT ); Fri, 25 Nov 2016 09:45:17 -0500 MIME-Version: 1.0 In-Reply-To: <20161103220428.984a8d09d0c9569e6bc6b8cc@gmail.com> References: <20161103220428.984a8d09d0c9569e6bc6b8cc@gmail.com> From: Dan Streetman Date: Fri, 25 Nov 2016 09:43:37 -0500 X-Google-Sender-Auth: ADDVqvd_b57-ReH02hW-QiIIsMY Message-ID: Subject: Re: [PATH] z3fold: extend compaction function To: Vitaly Wool Cc: Linux-MM , linux-kernel , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4226 Lines: 109 On Thu, Nov 3, 2016 at 5:04 PM, Vitaly Wool 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 with the bikeshedding comments below, looks good. Acked-by: Dan Streetman > --- > 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