> > [..]
> > > +/*********************************
> > > +* lru functions
> > > +**********************************/
> > > +static bool zswap_lru_add(struct list_lru *list_lru, struct zswap_entry *entry)
> > > +{
> > > + struct mem_cgroup *memcg = get_mem_cgroup_from_entry(entry);
> > > + int nid = entry_to_nid(entry);
> > > + bool added = list_lru_add(list_lru, &entry->lru, nid, memcg);
> > > +
> > > + mem_cgroup_put(memcg);
> >
> > Still not fond of the get/put pattern but okay..
>
> Actually, Johannes and I took another look to see if we can replace
> the memcg reference getting with just rcu_read_lock().
>
> It seems there might be a race between zswap LRU manipulation
> and memcg offlining - not just with the rcu_read_lock() idea, but also
> with our current implementation!
>
> I'll shoot another email with more details later when I'm sure of it
> one way or another...
>
Interesting, well at least something came out of my complaining :)
> > [..]
> > > @@ -652,28 +679,37 @@ static int zswap_reclaim_entry(struct zswap_pool *pool)
> > > */
> > > swpoffset = swp_offset(entry->swpentry);
> > > tree = zswap_trees[swp_type(entry->swpentry)];
> > > - spin_unlock(&pool->lru_lock);
> > > + list_lru_isolate(l, item);
> > > + /*
> > > + * It's safe to drop the lock here because we return either
> > > + * LRU_REMOVED_RETRY or LRU_RETRY.
> > > + */
> > > + spin_unlock(lock);
> > >
> > > /* Check for invalidate() race */
> > > spin_lock(&tree->lock);
> > > - if (entry != zswap_rb_search(&tree->rbroot, swpoffset)) {
> > > - ret = -EAGAIN;
> > > + if (entry != zswap_rb_search(&tree->rbroot, swpoffset))
> > > goto unlock;
> > > - }
> > > +
> > > /* Hold a reference to prevent a free during writeback */
> > > zswap_entry_get(entry);
> > > spin_unlock(&tree->lock);
> > >
> > > - ret = zswap_writeback_entry(entry, tree);
> > > + writeback_result = zswap_writeback_entry(entry, tree);
> > >
> > > spin_lock(&tree->lock);
> > > - if (ret) {
> > > - /* Writeback failed, put entry back on LRU */
> > > - spin_lock(&pool->lru_lock);
> > > - list_move(&entry->lru, &pool->lru);
> > > - spin_unlock(&pool->lru_lock);
> > > + if (writeback_result) {
> > > + zswap_reject_reclaim_fail++;
> > > + memcg = get_mem_cgroup_from_entry(entry);
> >
> > Can this return NULL? Seems like we don't check the return in most/all places.
>
> I believe so, but memcg experts should fact check me on this.
If that's the case, there should be NULL checks, no?
> It's roughly the same pattern as zswap charging/uncharging:
>
> obj_cgroup_uncharge_zswap(entry->objcg, entry->length)
> -> getting memcg (under rcu_read_lock())
>
> >
> > > + spin_lock(lock);
> > > + /* we cannot use zswap_lru_add here, because it increments node's lru count */
> > > + list_lru_putback(&entry->pool->list_lru, item, entry_to_nid(entry), memcg);
> >
> > Perhaps we can move this call with the memcg get/put to a helper like
> > add/del? (e.g. zswap_lru_putback)
> >
> > We would need to move get_mem_cgroup_from_entry() into the lock but I
> > think that's okay.
>
> We probably could, but that sounds like extra code for not a lot of gains, no?
I don't feel strongly, just a fan of consistency.
>
> >
> > > + spin_unlock(lock);
> > > + mem_cgroup_put(memcg);
> > > + ret = LRU_RETRY;
> > > goto put_unlock;
> > > }
> > > + zswap_written_back_pages++;
> > >
> > > /*
> > > * Writeback started successfully, the page now belongs to the
[..]
> > > @@ -696,15 +759,17 @@ static void shrink_worker(struct work_struct *w)
> > > shrink_work);
> > > int ret, failures = 0;
> > >
> > > + /* global reclaim will select cgroup in a round-robin fashion. */
> > > do {
> > > - ret = zswap_reclaim_entry(pool);
> > > - if (ret) {
> > > - zswap_reject_reclaim_fail++;
> > > - if (ret != -EAGAIN)
> > > - break;
> > > - if (++failures == MAX_RECLAIM_RETRIES)
> > > - break;
> > > - }
> > > + pool->next_shrink = mem_cgroup_iter(NULL, pool->next_shrink, NULL);
> >
> > I think this can be a problem. We hold a ref to a memcg here until the
> > next time we shrink, which can be a long time IIUC. This can cause the
> > memcg to linger as a zombie. I understand it is one memcg per-zswap
> > pool, but I am still unsure about it.
> >
> > MGLRU maintains a memcg LRU for global reclaim that gets properly
> > cleaned up when a memcg is going away, so that's one option, although
> > complicated.
> >
> > A second option would be to hold a pointer to the objcg instead, which
> > should be less problematic (although we are still holding that objcg
> > hostage indefinitely). The problem here is that if the objcg gets
> > reparented, next time we will start at the parent of the memcg we
> > stopped at last time, which tbh doesn't sound bad at all to me.
> >
> > A third option would be to flag the memcg such that when it is getting
> > offlined we can call into zswap to reset pool->next_shrink (or move it
> > to the parent) and drop the ref. Although synchronization can get
> > hairy when racing with offlining.
> >
> > Not sure what's the right solution, but I prefer we don't hold any
> > memcgs hostages indefinitely. I also think if we end up using
> > mem_cgroup_iter() then there should be a mem_cgroup_iter_break()
> > somewhere if/when breaking the iteration.
> >
>
> I'm not sure if this is that big of a problem in the first place, but
> if it is, doing something similar to MGLRU is probably the cleanest:
> when the memcg is freed, trigger the zswap_exit_memcg() callback,
> which will loop through all the zswap pools and update pool->next_shrink
> where appropriate.
>
> Note that we only have one pool per (compression algorithm x allocator)
> combinations, so there cannot be that many pools, correct?
>
> Johannes suggests this idea to me (my apologies if I butcher it)
> during one of our conversations. That sounds relatively easy IIUC.
Be careful that there will be a race between memcg offlining and
zswap's usage of pool->next_shrink. AFAICT there is no lock to prevent
offlining so there will have to be some sort of dance here to make
sure everything works correctly.
