Re: [PATCH v5 4/6] mm/zswap: Implement proactive writeback

[Yosry Ahmed]

> > Before going through more versions we need to figure out if this will
> > pivot to be a proactive demotion interfcae for swap tiering.
> >
>
> Yes. Should I drop patches 4-6 in the next version and wait for swap
> tiering to be finalized?
> We can try to get the non-memcg parts (patches 1-3) merged upstream
> first. This would also give them plenty of time to bake and catch any
> potential regressions. Thoughts?

Patches 1-2 can be sent and merged separately, yes. For patch 2,
please include some numbers for the writeback performance before and
after batching.

Patch 3 does refactoring in preparation for patch 4, so I don't think
it makes sense on its own.

> >> +int zswap_proactive_writeback(struct mem_cgroup *memcg, u64 bytes_to_writeback)
> >> +{
> >> +    struct zswap_shrink_state s = {};
> >> +    struct mem_cgroup *iter = NULL;
> >> +    u64 bytes_written = 0;
> >> +    int ret = 0;
> >> +
> >> +    if (!memcg)
> >> +            return -EINVAL;
> >
> > Can this ever happen? It would be a bug in the caller.
>
> IIRC，Writing the following to the NUMA node sysfs entry triggers this
> check:
> echo "10M source=zswap" > /sys/devices/system/node/nodeN/reclaim

Oh yeah, I forgot about that one :)

If we keep this, probably combine the !memcg and writeback check below.

>
> >
> >> +    if (!mem_cgroup_zswap_writeback_enabled(memcg))
> >> +            return -EINVAL;
> >> +    if (!bytes_to_writeback)
> >> +            return 0;
> >
> > Do we need this? I think the loop will just never enter and
> > mem_cgroup_iter_break() will do nothing.
>
> Will do.
> >
> >> +
> >> +    while (bytes_written < bytes_to_writeback) {
> >> +            long shrunk;
> >> +
> >> +            cond_resched();
> >> +
> >> +            if (signal_pending(current)) {
> >> +                    ret = -EINTR;
> >> +                    break;
> >> +            }
> >> +
> >> +            /*
> >> +             * Use a local iterator to walk the memcg and its online descendants
> >> +             * in a round-robin manner. Upon exiting the loop, mem_cgroup_iter_break()
> >> +             * must be called to drop the iterator reference.
> >> +             */
> >> +            do {
> >> +                    iter = mem_cgroup_iter(memcg, iter, NULL);
> >> +            } while (iter && !mem_cgroup_tryget_online(iter));
> >> +
> >> +            shrunk = zswap_shrink_one_memcg(iter, &s);
> >> +            if (shrunk > 0)
> >> +                    bytes_written += shrunk;
> >> +
> >> +            /* drop the extra reference taken by mem_cgroup_tryget_online() */
> >> +            mem_cgroup_put(iter);
> >
> >
> > Can we just use mem_cgroup_online() instead since mem_cgroup_iter()
> > already graps a ref?
> >
> Will do.

If you're looking for another cleanup to do, shrink_worker() should
probably also use mem_cgroup_online() and avoid taking/dropping an
extra ref :)