On Fri, 04 Oct 2019 16:09:22 +0300 Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx> wrote:
> This is very slow operation. There is no reason to do it again if somebody
> else already drained all per-cpu vectors while we waited for lock.
>
> Piggyback on drain started and finished while we waited for lock:
> all pages pended at the time of our enter were drained from vectors.
>
> Callers like POSIX_FADV_DONTNEED retry their operations once after
> draining per-cpu vectors when pages have unexpected references.
>
> ...
>
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -708,9 +708,10 @@ static void lru_add_drain_per_cpu(struct work_struct *dummy)
> */
> void lru_add_drain_all(void)
> {
> + static seqcount_t seqcount = SEQCNT_ZERO(seqcount);
> static DEFINE_MUTEX(lock);
> static struct cpumask has_work;
> - int cpu;
> + int cpu, seq;
>
> /*
> * Make sure nobody triggers this path before mm_percpu_wq is fully
> @@ -719,7 +720,19 @@ void lru_add_drain_all(void)
> if (WARN_ON(!mm_percpu_wq))
> return;
>
> + seq = raw_read_seqcount_latch(&seqcount);
> +
> mutex_lock(&lock);
> +
> + /*
> + * Piggyback on drain started and finished while we waited for lock:
> + * all pages pended at the time of our enter were drained from vectors.
> + */
> + if (__read_seqcount_retry(&seqcount, seq))
> + goto done;
> +
> + raw_write_seqcount_latch(&seqcount);
> +
> cpumask_clear(&has_work);
>
> for_each_online_cpu(cpu) {
> @@ -740,6 +753,7 @@ void lru_add_drain_all(void)
> for_each_cpu(cpu, &has_work)
> flush_work(&per_cpu(lru_add_drain_work, cpu));
>
> +done:
> mutex_unlock(&lock);
> }
I'm not sure this works as intended.
Suppose CPU #30 is presently executing the for_each_online_cpu() loop
and has reached CPU #15's per-cpu data.
Now CPU #2 comes along, adds some pages to its per-cpu vectors then
calls lru_add_drain_all(). AFAICT the code will assume that CPU #30
has flushed out all of the pages which CPU #2 just added, but that
isn't the case.
Moving the raw_write_seqcount_latch() to the point where all processing
has completed might fix?
