> > Ah my bad, I should have been clearer. > > I was looking at the zswap shrinker patch series (specifically the > cgroup-aware LRU patch), which moves the counter update out of > zswap_writeback_entry. If we apply that patch on top of that series, we will > screw up the counter. Should be easily fixable anyway though. Got it. > > Ah I think I understand the point of the patch a bit better now. > > Essentially, we're invalidating these entries, which does reclaim the > memory used for these compressed objects, but there is no IO involved. > Writeback-less shrinking, if you will. > > This will still screw up one of the heuristics I'm using for the zswap > shrinker a bit, but that should be easily fixable with some plumbing. > Same goes for the writeback counter - but depending on the order in > which Andrew apply the patches, you might have to resolve the conflicts > there :) OK, I will fix it. > > Other than this objection, I think this optimization makes sense to me: > > In the first case, we already freed the swap entry. Might as well also > dropped the zswap entry. > > In the second case, we already have another copy in memory, so > dropping the compressed copy to make space for warmer objects > coming into zswap makes sense to me. Might be worth doing a > micro-benchmark to verify this intuition, but I agree that it's more > important to maintain the LRU ordering than any CPU saving from > skipping re-compression. > > I would suggest that you should expand on this on the commit log > to make clearer the motivation behind this optimization, if you were > to re-submit this patch for some reason (for e.g to resolve the > aforementioned conflicts with the zswap shrinker series). > > But otherwise, LGTM! > > Feel free to add the following tag: > Reviewed-by: Nhat Pham <nphamcs@xxxxxxxxx> Thanks, there are still some commits from Yosry, after that, I will send it again.
