Re: [PATH] z3fold: extend compaction function

[Vitaly Wool]

On Fri, Nov 25, 2016 at 10:17 PM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
> On Fri, Nov 25, 2016 at 9:43 AM, Dan Streetman <ddstreet@xxxxxxxx> wrote:
>> 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;
>
> after closer review, I see that this is wrong.  NCHUNKS isn't the
> total number of page chunks, it's the total number of chunks minus the
> header chunk(s).  so that calculation of where the new start is, is
> wrong.  it should use the total page chunks, not the NCHUNKS, because
> start_middle already accounts for the header chunk(s).  Probably a new
> macro would help.
>
> Also, the num_free_chunks() function makes the same mistake:
>
> int nfree_after = zhdr->last_chunks ?
>   0 : NCHUNKS - zhdr->start_middle - zhdr->middle_chunks;
>
> that's wrong, it should be something like:
>
> #define TOTAL_CHUNKS (PAGE_SIZE >> CHUNK_SHIFT)
> ...
> int nfree_after = zhdr->last_chunks ?
>   0 : TOTAL_CHUNKS - zhdr->start_middle - zhdr->middle_chunks;

Right, will fix.

~vitaly