On Sat, Apr 26, 2014 at 4:37 AM, Weijie Yang <weijie.yang.kh@xxxxxxxxx> wrote: > On Sat, Apr 19, 2014 at 11:52 PM, Dan Streetman <ddstreet@xxxxxxxx> 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, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>