Re: [PATCH] z3fold: stricter locking and more careful reclaim

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 4, 2020 at 10:47 PM Mike Galbraith <efault@xxxxxx> wrote:
>
> On Fri, 2020-12-04 at 22:04 +0200, Vitaly Wool wrote:
> > Use temporary slots in reclaim function to avoid possible race when
> > freeing those.
> >
> > While at it, make sure we check CLAIMED flag under page lock in the
> > reclaim function to make sure we are not racing with z3fold_alloc().
> >
> > Signed-off-by: Vitaly Wool <vitaly.wool@xxxxxxxxxxxx>
>
> FYI, with this applied, ltp::mm still crashes the RT kernel, with the
> same rwlock corruption being what takes it down.

So sad it does, but thanks a lot for the quick response.

~Vitaly

>         -Mike
> > ---
> >  mm/z3fold.c | 133 +++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 74 insertions(+), 59 deletions(-)
> >
> > diff --git a/mm/z3fold.c b/mm/z3fold.c
> > index 18feaa0bc537..47f05228abdf 100644
> > --- a/mm/z3fold.c
> > +++ b/mm/z3fold.c
> > @@ -186,6 +186,7 @@ enum z3fold_page_flags {
> >   */
> >  enum z3fold_handle_flags {
> >       HANDLES_ORPHANED = 0,
> > +     HANDLES_NOFREE = 1,
> >  };
> >
> >  /*
> > @@ -320,7 +321,7 @@ static inline void free_handle(unsigned long handle)
> >       slots = handle_to_slots(handle);
> >       write_lock(&slots->lock);
> >       *(unsigned long *)handle = 0;
> > -     if (zhdr->slots == slots) {
> > +     if (zhdr->slots == slots || test_bit(HANDLES_NOFREE, &slots->pool)) {
> >               write_unlock(&slots->lock);
> >               return; /* simple case, nothing else to do */
> >       }
> > @@ -653,6 +654,28 @@ static inline void add_to_unbuddied(struct z3fold_pool *pool,
> >       }
> >  }
> >
> > +static inline enum buddy get_free_buddy(struct z3fold_header *zhdr, int chunks)
> > +{
> > +     enum buddy bud = HEADLESS;
> > +
> > +     if (zhdr->middle_chunks) {
> > +             if (!zhdr->first_chunks &&
> > +                 chunks <= zhdr->start_middle - ZHDR_CHUNKS)
> > +                     bud = FIRST;
> > +             else if (!zhdr->last_chunks)
> > +                     bud = LAST;
> > +     } else {
> > +             if (!zhdr->first_chunks)
> > +                     bud = FIRST;
> > +             else if (!zhdr->last_chunks)
> > +                     bud = LAST;
> > +             else
> > +                     bud = MIDDLE;
> > +     }
> > +
> > +     return bud;
> > +}
> > +
> >  static inline void *mchunk_memmove(struct z3fold_header *zhdr,
> >                               unsigned short dst_chunk)
> >  {
> > @@ -714,18 +737,7 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
> >               if (WARN_ON(new_zhdr == zhdr))
> >                       goto out_fail;
> >
> > -             if (new_zhdr->first_chunks == 0) {
> > -                     if (new_zhdr->middle_chunks != 0 &&
> > -                                     chunks >= new_zhdr->start_middle) {
> > -                             new_bud = LAST;
> > -                     } else {
> > -                             new_bud = FIRST;
> > -                     }
> > -             } else if (new_zhdr->last_chunks == 0) {
> > -                     new_bud = LAST;
> > -             } else if (new_zhdr->middle_chunks == 0) {
> > -                     new_bud = MIDDLE;
> > -             }
> > +             new_bud = get_free_buddy(new_zhdr, chunks);
> >               q = new_zhdr;
> >               switch (new_bud) {
> >               case FIRST:
> > @@ -847,9 +859,8 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> >               return;
> >       }
> >
> > -     if (unlikely(PageIsolated(page) ||
> > -                  test_bit(PAGE_CLAIMED, &page->private) ||
> > -                  test_bit(PAGE_STALE, &page->private))) {
> > +     if (test_bit(PAGE_STALE, &page->private) ||
> > +         test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> >               z3fold_page_unlock(zhdr);
> >               return;
> >       }
> > @@ -858,13 +869,16 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> >           zhdr->mapped_count == 0 && compact_single_buddy(zhdr)) {
> >               if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> >                       atomic64_dec(&pool->pages_nr);
> > -             else
> > +             else {
> > +                     clear_bit(PAGE_CLAIMED, &page->private);
> >                       z3fold_page_unlock(zhdr);
> > +             }
> >               return;
> >       }
> >
> >       z3fold_compact_page(zhdr);
> >       add_to_unbuddied(pool, zhdr);
> > +     clear_bit(PAGE_CLAIMED, &page->private);
> >       z3fold_page_unlock(zhdr);
> >  }
> >
> > @@ -1109,17 +1123,8 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> >  retry:
> >               zhdr = __z3fold_alloc(pool, size, can_sleep);
> >               if (zhdr) {
> > -                     if (zhdr->first_chunks == 0) {
> > -                             if (zhdr->middle_chunks != 0 &&
> > -                                 chunks >= zhdr->start_middle)
> > -                                     bud = LAST;
> > -                             else
> > -                                     bud = FIRST;
> > -                     } else if (zhdr->last_chunks == 0)
> > -                             bud = LAST;
> > -                     else if (zhdr->middle_chunks == 0)
> > -                             bud = MIDDLE;
> > -                     else {
> > +                     bud = get_free_buddy(zhdr, chunks);
> > +                     if (bud == HEADLESS) {
> >                               if (kref_put(&zhdr->refcount,
> >                                            release_z3fold_page_locked))
> >                                       atomic64_dec(&pool->pages_nr);
> > @@ -1265,7 +1270,6 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> >               pr_err("%s: unknown bud %d\n", __func__, bud);
> >               WARN_ON(1);
> >               put_z3fold_header(zhdr);
> > -             clear_bit(PAGE_CLAIMED, &page->private);
> >               return;
> >       }
> >
> > @@ -1280,8 +1284,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> >               z3fold_page_unlock(zhdr);
> >               return;
> >       }
> > -     if (unlikely(PageIsolated(page)) ||
> > -         test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
> > +     if (test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
> >               put_z3fold_header(zhdr);
> >               clear_bit(PAGE_CLAIMED, &page->private);
> >               return;
> > @@ -1345,6 +1348,10 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
> >       struct page *page = NULL;
> >       struct list_head *pos;
> >       unsigned long first_handle = 0, middle_handle = 0, last_handle = 0;
> > +     struct z3fold_buddy_slots slots __attribute__((aligned(SLOTS_ALIGN)));
> > +
> > +     rwlock_init(&slots.lock);
> > +     slots.pool = (unsigned long)pool | (1 << HANDLES_NOFREE);
> >
> >       spin_lock(&pool->lock);
> >       if (!pool->ops || !pool->ops->evict || retries == 0) {
> > @@ -1359,35 +1366,36 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
> >               list_for_each_prev(pos, &pool->lru) {
> >                       page = list_entry(pos, struct page, lru);
> >
> > -                     /* this bit could have been set by free, in which case
> > -                      * we pass over to the next page in the pool.
> > -                      */
> > -                     if (test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> > -                             page = NULL;
> > -                             continue;
> > -                     }
> > -
> > -                     if (unlikely(PageIsolated(page))) {
> > -                             clear_bit(PAGE_CLAIMED, &page->private);
> > -                             page = NULL;
> > -                             continue;
> > -                     }
> >                       zhdr = page_address(page);
> >                       if (test_bit(PAGE_HEADLESS, &page->private))
> >                               break;
> >
> > +                     if (kref_get_unless_zero(&zhdr->refcount) == 0) {
> > +                             zhdr = NULL;
> > +                             break;
> > +                     }
> >                       if (!z3fold_page_trylock(zhdr)) {
> > -                             clear_bit(PAGE_CLAIMED, &page->private);
> > +                             if (kref_put(&zhdr->refcount,
> > +                                             release_z3fold_page))
> > +                                     atomic64_dec(&pool->pages_nr);
> >                               zhdr = NULL;
> >                               continue; /* can't evict at this point */
> >                       }
> > -                     if (zhdr->foreign_handles) {
> > -                             clear_bit(PAGE_CLAIMED, &page->private);
> > -                             z3fold_page_unlock(zhdr);
> > +
> > +                     /* test_and_set_bit is of course atomic, but we still
> > +                      * need to do it under page lock, otherwise checking
> > +                      * that bit in __z3fold_alloc wouldn't make sense
> > +                      */
> > +                     if (zhdr->foreign_handles ||
> > +                         test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> > +                             if (kref_put(&zhdr->refcount,
> > +                                             release_z3fold_page))
> > +                                     atomic64_dec(&pool->pages_nr);
> > +                             else
> > +                                     z3fold_page_unlock(zhdr);
> >                               zhdr = NULL;
> >                               continue; /* can't evict such page */
> >                       }
> > -                     kref_get(&zhdr->refcount);
> >                       list_del_init(&zhdr->buddy);
> >                       zhdr->cpu = -1;
> >                       break;
> > @@ -1409,12 +1417,16 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
> >                       first_handle = 0;
> >                       last_handle = 0;
> >                       middle_handle = 0;
> > +                     memset(slots.slot, 0, sizeof(slots.slot));
> >                       if (zhdr->first_chunks)
> > -                             first_handle = encode_handle(zhdr, FIRST);
> > +                             first_handle = __encode_handle(zhdr, &slots,
> > +                                                             FIRST);
> >                       if (zhdr->middle_chunks)
> > -                             middle_handle = encode_handle(zhdr, MIDDLE);
> > +                             middle_handle = __encode_handle(zhdr, &slots,
> > +                                                             MIDDLE);
> >                       if (zhdr->last_chunks)
> > -                             last_handle = encode_handle(zhdr, LAST);
> > +                             last_handle = __encode_handle(zhdr, &slots,
> > +                                                             LAST);
> >                       /*
> >                        * it's safe to unlock here because we hold a
> >                        * reference to this page
> > @@ -1429,19 +1441,16 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
> >                       ret = pool->ops->evict(pool, middle_handle);
> >                       if (ret)
> >                               goto next;
> > -                     free_handle(middle_handle);
> >               }
> >               if (first_handle) {
> >                       ret = pool->ops->evict(pool, first_handle);
> >                       if (ret)
> >                               goto next;
> > -                     free_handle(first_handle);
> >               }
> >               if (last_handle) {
> >                       ret = pool->ops->evict(pool, last_handle);
> >                       if (ret)
> >                               goto next;
> > -                     free_handle(last_handle);
> >               }
> >  next:
> >               if (test_bit(PAGE_HEADLESS, &page->private)) {
> > @@ -1455,9 +1464,11 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int retries)
> >                       spin_unlock(&pool->lock);
> >                       clear_bit(PAGE_CLAIMED, &page->private);
> >               } else {
> > +                     struct z3fold_buddy_slots *slots = zhdr->slots;
> >                       z3fold_page_lock(zhdr);
> >                       if (kref_put(&zhdr->refcount,
> >                                       release_z3fold_page_locked)) {
> > +                             kmem_cache_free(pool->c_handle, slots);
> >                               atomic64_dec(&pool->pages_nr);
> >                               return 0;
> >                       }
> > @@ -1573,8 +1584,7 @@ static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode)
> >       VM_BUG_ON_PAGE(!PageMovable(page), page);
> >       VM_BUG_ON_PAGE(PageIsolated(page), page);
> >
> > -     if (test_bit(PAGE_HEADLESS, &page->private) ||
> > -         test_bit(PAGE_CLAIMED, &page->private))
> > +     if (test_bit(PAGE_HEADLESS, &page->private))
> >               return false;
> >
> >       zhdr = page_address(page);
> > @@ -1586,6 +1596,8 @@ static bool z3fold_page_isolate(struct page *page, isolate_mode_t mode)
> >       if (zhdr->mapped_count != 0 || zhdr->foreign_handles != 0)
> >               goto out;
> >
> > +     if (test_and_set_bit(PAGE_CLAIMED, &page->private))
> > +             goto out;
> >       pool = zhdr_to_pool(zhdr);
> >       spin_lock(&pool->lock);
> >       if (!list_empty(&zhdr->buddy))
> > @@ -1612,16 +1624,17 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
> >
> >       VM_BUG_ON_PAGE(!PageMovable(page), page);
> >       VM_BUG_ON_PAGE(!PageIsolated(page), page);
> > +     VM_BUG_ON_PAGE(!test_bit(PAGE_CLAIMED, &page->private), page);
> >       VM_BUG_ON_PAGE(!PageLocked(newpage), newpage);
> >
> >       zhdr = page_address(page);
> >       pool = zhdr_to_pool(zhdr);
> >
> > -     if (!z3fold_page_trylock(zhdr)) {
> > +     if (!z3fold_page_trylock(zhdr))
> >               return -EAGAIN;
> > -     }
> >       if (zhdr->mapped_count != 0 || zhdr->foreign_handles != 0) {
> >               z3fold_page_unlock(zhdr);
> > +             clear_bit(PAGE_CLAIMED, &page->private);
> >               return -EBUSY;
> >       }
> >       if (work_pending(&zhdr->work)) {
> > @@ -1663,6 +1676,7 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
> >       queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work);
> >
> >       page_mapcount_reset(page);
> > +     clear_bit(PAGE_CLAIMED, &page->private);
> >       put_page(page);
> >       return 0;
> >  }
> > @@ -1686,6 +1700,7 @@ static void z3fold_page_putback(struct page *page)
> >       spin_lock(&pool->lock);
> >       list_add(&page->lru, &pool->lru);
> >       spin_unlock(&pool->lock);
> > +     clear_bit(PAGE_CLAIMED, &page->private);
> >       z3fold_page_unlock(zhdr);
> >  }
> >
>




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux