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.
-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);
> }
>
