On Sat, Apr 26, 2014 at 4:37 AM, Weijie Yang <[email protected]> wrote: > On Sat, Apr 19, 2014 at 11:52 PM, Dan Streetman <[email protected]> wrote: >> Add zs_shrink() and helper functions to zsmalloc. Update zsmalloc >> zs_create_pool() creation function to include ops param that provides >> an evict() function for use during shrinking. Update helper function >> fix_fullness_group() to always reinsert changed zspages even if the >> fullness group did not change, so they are updated in the fullness >> group lru. Also update zram to use the new zsmalloc pool creation >> function but pass NULL as the ops param, since zram does not use >> pool shrinking. >> > > I only review the code without test, however, I think this patch is > not acceptable. > > The biggest problem is it will call zswap_writeback_entry() under lock, > zswap_writeback_entry() may sleep, so it is a bug. see below
thanks for catching that! > > The 3/4 patch has a lot of #ifdef, I don't think it's a good kind of > abstract way. it has the #ifdef's because there's no point in compiling in code to use zbud/zsmalloc if zbud/zsmalloc isn't compiled...what alternative to #ifdef's would you suggest? Or are there just specific #ifdefs you suggest to remove? > > What about just disable zswap reclaim when using zsmalloc? > There is a long way to optimize writeback reclaim(both zswap and zram) , > Maybe a small and simple step forward is better. I think it's possible to just remove the zspage from the class while under lock, then unlock and reclaim it. As long as there's a guarantee that zswap (or any zpool/zsmalloc reclaim user) doesn't map/access the handle after evict() completes successfully, that should work. There does need to be some synchronization between zs_free() and each handle's eviction though, similar to zbud's under_reclaim flag. I'll work on a v2 patch. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
