On Fri, Apr 29, 2016 at 10:50:13AM -0400, Dan Streetman wrote: > On Fri, Apr 29, 2016 at 1:37 AM, Minchan Kim <minchan@xxxxxxxxxx> wrote: > > On Fri, Apr 29, 2016 at 09:38:24AM +0900, Sergey Senozhatsky wrote: > >> On (04/28/16 15:07), Andrew Morton wrote: > >> > Needed a bit of tweaking due to > >> > http://ozlabs.org/~akpm/mmotm/broken-out/zsmalloc-reordering-function-parameter.patch > >> > >> Thanks. > >> > >> > From: Dan Streetman <ddstreet@xxxxxxxx> > >> > Subject: mm/zsmalloc: don't fail if can't create debugfs info > >> > > >> > Change the return type of zs_pool_stat_create() to void, and > >> > remove the logic to abort pool creation if the stat debugfs > >> > dir/file could not be created. > >> > > >> > The debugfs stat file is for debugging/information only, and doesn't > >> > affect operation of zsmalloc; there is no reason to abort creating > >> > the pool if the stat file can't be created. This was seen with > >> > zswap, which used the same name for all pool creations, which caused > >> > zsmalloc to fail to create a second pool for zswap if > >> > CONFIG_ZSMALLOC_STAT was enabled. > >> > >> no real objections from me. given that both zram and zswap now provide > >> unique names for zsmalloc stats dir, this patch does not fix any "real" > >> (observed) problem /* ENOMEM in debugfs_create_dir() is a different > >> case */. so it's more of a cosmetic patch. > >> > > > > Logically, I agree with Dan that debugfs is just optional so it > > shouldn't affect the module running *but* practically, debugfs_create_dir > > failure with no memory would be rare. Rather than it, we would see > > error from same entry naming like Dan's case. > > > > If we removes such error propagation logic in case of same naming, > > how do zsmalloc user can notice that debugfs entry was not created > > although zs_creation was successful returns success? > > Does it actually matter to the caller? > > Since there's no way for zsmalloc to know if the stats dir/file > creation failed because of EEXIST or because of ENOMEM, there's no way > for it to let the caller know why it failed, either. Thus all > zsmalloc can do is return a generic error, or possibly-wrong ENOMEM. > In that case what will the caller do? Change the name and try again? > How does the caller know what name to change it to, maybe the new name > is taken too? > > The point of debugfs is to provide debug; failures should be ignored, > because it's just debug. It should never prevent actual operation of > the driver. > > > > > Otherwise, future user of zsmalloc can miss it easily if they repeates > > same mistakes. So, what's the gain with this patch in real practice? > > Well as far as future users, zs_create_pool doesn't document 'name' at > all, and certainly doesn't clarify that 'name' should be unique across > *all* zs pools that exist. And zsmalloc behavior should not be > different depending on whether the ZSMALLOC_STAT param - which appears > to be a debug/info only param - is enabled or not. Fair enough. Then, could you apply it to zs_stat_init? We could make zs_stat_init to return void to not affect module loading, too. As well, we should be okay in zs_stat_root failue, too. I hope your patch handles them all in this chance. Thanks for the looking this. > > But after any future driver using zsmalloc is created, if it did use > an already-existing name - either because it was not coded to use > unique names, or because of a bug that reused an existing name - which > is worse? > 1) driver suddenly stops working because new zs pools can't be created? > 2) statistics information isn't available for some of the pools created? > > And, even though zswap is now patched to provide a unique name, why > does zswap have to bear the burden of that? zswap doesn't care at all > about the pool name, and there's no way for users to tell which > zsmalloc pool corresponds to which zswap pool parameters. Future > users of zsmalloc (from a code point of view, not person) probably > will also not care about the zsmalloc pool name. And zsmalloc still > logs the failure - so anyone looking for the stats and not finding it > can easily check the logs to see the reason. > > The other alternative that I mentioned before, is for zsmalloc to take > care of the problem itself. If the debugfs dir creation fails, it > should change the name and retry; or zsmalloc can keep a list of > active pool names so it knows if a new pool's name exists already or > not. But neither expecting the calling code to retry with a different > name, nor failing pool creation, seem like a good response when the > only failure is providing debug/stats information. > -- 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>