On 2023/12/23 01:37, Chris Li wrote: > Hi Chengming, > > The patch looks good to me. > > Acked-by: Chris Li <chrisl@xxxxxxxxxx> (Google) > Thanks. [...] >> >> I think there are two viewpoints here, both works ok to me. >> >> The first is from ownship or lifetime, these percpu mutex and dstmem >> are shared between all pools, so no one pool own the mutex and dstmem, >> nor does the percpu acomp_ctx in each pool. >> >> The second is from usage, these percpu mutex and dstmem are used by >> the percpu acomp_ctx in each pool, so it's easy to use to put pointers >> to them in the percpu acomp_ctx. >> >> Actually I think it's simpler to let the percpu acomp_ctx has its own >> mutex and dstmem, which in fact are the necessary parts when it use >> the acomp interfaces. > > Agree, that is why I prefer to keep the struct together. I am fine > with what Yosry suggested and the anonymous struct, just consider it > is not critically necessary. > Agree, I have no strong opinion about it, so will just leave it as it is. >> >> This way, we could delete the percpu mutex and dstmem, and its hotplugs, > > That is the real value of this patch. Thanks for doing that. > >> and not shared anymore between all pools. Maybe we would have many pools >> at the same time in the future, like different compression algorithm or >> different zpool for different memcg workloads. Who knows? :-) > > As long as the zswap is not re-enterable, e.g. never have the nested > page fault that causes zswap_load within another zswap_load, I think > we are fine having more than one pool share the buffer. In fact, if we > trigger the nested zswap load, I expect the kernel will dead lock on > the nested mutex acquire because the mutex is already taken. We will > know about kernel panic rather than selient memory corruption. > >> >> So how about this patch below? Just RFC. >> >> Subject: [PATCH] mm/zswap: make each acomp_ctx has its own mutex and dstmem > > Thank you for doing that, you can consider submitting it as the real > patch instead of the RFC. I see real value in this patch removing some > per CPU fields. Ok, will update. Thanks!
