On Thu, Aug 29, 2024 at 3:49 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: > > On Thu, Aug 29, 2024 at 2:27 PM Kanchana P Sridhar > > We are basically comparing zram with zswap in this case, and it's not > fair because, as you mentioned, the zswap compressed data is being > accounted for while the zram compressed data isn't. I am not really > sure how valuable these test results are. Even if we remove the cgroup > accounting from zswap, we won't see an improvement, we should expect a > similar performance to zram. > > I think the test results that are really valuable are case 1, where > zswap users are currently disabling CONFIG_THP_SWAP, and get to enable > it after this series. Ah, this is a good point. I think the point of comparing mTHP zswap v.s mTHP (SSD)swap is more of a sanity check. IOW, if mTHP swap outperforms mTHP zswap, then something is wrong (otherwise why would enable zswap - might as well just use swap, since SSD swap with mTHP >>> zswap with mTHP >>> zswap without mTHP). That said, I don't think this benchmark can show it anyway. The access pattern here is such that all the allocated memories are really cold, so swap to disk (or to zram, which does not account memory usage towards cgroup) is better by definition... And Kanchana does not seem to have access to setup with larger SSD swapfiles? :) > > If we really want to compare CONFIG_THP_SWAP on before and after, it > should be with SSD because that's a more conventional setup. In this > case the users that have CONFIG_THP_SWAP=y only experience the > benefits of zswap with this series. You mentioned experimenting with > usemem to keep the memory allocated longer so that you're able to have > a fair test with the small SSD swap setup. Did that work? > > I am hoping Nhat or Johannes would shed some light on whether they > usually have CONFIG_THP_SWAP enabled or not with zswap. I am trying to > figure out if any reasonable setups enable CONFIG_THP_SWAP with zswap. > Otherwise the testing results from case 1 should be sufficient. > > > > > In my opinion, even though the test set up does not provide an accurate > > way for a direct before/after comparison (because of zswap usage being > > counted in cgroup, hence towards the memory.high), it still seems > > reasonable for zswap_store to support (m)THP, so that further performance > > improvements can be implemented. > > This is only referring to the results of case 2, right? > > Honestly, I wouldn't want to merge mTHP swapout support on its own > just because it enables further performance improvements without > having actual patches for them. But I don't think this captures the > results accurately as it dismisses case 1 results (which I think are > more reasonable). > > Thnaks
