Re: [PATCH v4] mm/migrate.c: Rework migration_entry_wait() to not take a pageref

[Alistair Popple]

On Tuesday, 23 November 2021 5:15:27 AM AEDT David Hildenbrand wrote:

[...]

> > +#ifdef CONFIG_MIGRATION
> > +/**
> > + * migration_entry_wait_on_locked - Wait for a migration entry to be removed
> > + * @folio: folio referenced by the migration entry.
> > + * @ptep: mapped pte pointer. This function will return with the ptep unmapped.
> > + * @ptl: already locked ptl. This function will drop the lock.
> > + *
> > + * Wait for a migration entry referencing the given page to be removed. This is
> > + * equivalent to put_and_wait_on_page_locked(page, TASK_UNINTERRUPTIBLE) except
> > + * this can be called without taking a reference on the page. Instead this
> > + * should be called while holding the ptl for the migration entry referencing
> > + * the page.
> > + *
> > + * Returns after unmapping and unlocking the pte/ptl with pte_unmap_unlock().
> 
> You could maybe make it clear that callers have to pass the ptep only
> for PTE migration entries. For a PMD migration entry, pass NULL.

Will do.
 
> > + *
> > + * This follows the same logic as wait_on_page_bit_common() so see the comments
> 
> s/wait_on_page_bit_common/folio_wait_bit_common/ ?

Evidently this escaped my s// when rebasing on top of folio's. Will fix.

> > + * there.
> > + */
> > +void migration_entry_wait_on_locked(struct folio *folio, pte_t *ptep,
> > +				spinlock_t *ptl)
> > +{
> > +	struct wait_page_queue wait_page;
> > +	wait_queue_entry_t *wait = &wait_page.wait;
> > +	bool thrashing = false;
> > +	bool delayacct = false;
> > +	unsigned long pflags;
> > +	wait_queue_head_t *q;
> > +
> > +	q = folio_waitqueue(folio);
> > +	if (!folio_test_uptodate(folio) && folio_test_workingset(folio)) {
> > +		if (!folio_test_swapbacked(folio)) {
> > +			delayacct_thrashing_start();
> > +			delayacct = true;
> > +		}
> > +		psi_memstall_enter(&pflags);
> > +		thrashing = true;
> > +	}
> > +
> > +	init_wait(wait);
> > +	wait->func = wake_page_function;
> > +	wait_page.folio = folio;
> > +	wait_page.bit_nr = PG_locked;
> > +	wait->flags = 0;
> > +
> > +	spin_lock_irq(&q->lock);
> > +	folio_set_waiters(folio);
> > +	if (!folio_trylock_flag(folio, PG_locked, wait))
> > +		__add_wait_queue_entry_tail(q, wait);
> > +	spin_unlock_irq(&q->lock);
> > +
> > +	/*
> > +	 * If a migration entry exists for the page the migration path must hold
> > +	 * a valid reference to the page, and it must take the ptl to remove the
> > +	 * migration entry. So the page is valid until the ptl is dropped.
> > +	 */
> > +	if (ptep)
> > +		pte_unmap_unlock(ptep, ptl);
> > +	else
> > +		spin_unlock(ptl);
> > +
> > +	for (;;) {
> > +		unsigned int flags;
> > +
> > +		set_current_state(TASK_UNINTERRUPTIBLE);
> > +
> > +		/* Loop until we've been woken or interrupted */
> > +		flags = smp_load_acquire(&wait->flags);
> > +		if (!(flags & WQ_FLAG_WOKEN)) {
> > +			if (signal_pending_state(TASK_UNINTERRUPTIBLE, current))
> > +				break;
> > +
> > +			io_schedule();
> > +			continue;
> > +		}
> > +		break;
> > +	}
> > +
> > +	finish_wait(q, wait);
> > +
> > +	if (thrashing) {
> > +		if (delayacct)
> > +			delayacct_thrashing_end();
> > +		psi_memstall_leave(&pflags);
> > +	}
> > +}
> > +#endif
> > +
> 
> I'm fairly new to the glory details of core migration entry and page bit
> waiting code, but it makes sense to me and removing the temporary extra
> references is very nice! Feel free to add my
> 
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>

Thanks for taking a look, really appreciate it!

 - Alistair