Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Minchan Kim <minchan@kernel.org>
To: Dan Streetman <ddstreet@ieee.org>
Cc: Minchan Kim <minchan@kernel.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Nitin Gupta <ngupta@vflare.org>,
	Seth Jennings <sjenning@redhat.com>, Yu Zhao <yuzhao@google.com>,
	Linux-MM <linux-mm@kvack.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Dan Streetman <dan.streetman@canonical.com>
Subject: Re: [PATCH] mm/zsmalloc: don't fail if can't create debugfs info
Date: Sat, 30 Apr 2016 00:33:09 +0900
Message-ID: <20160429153309.GA2696@blaptop> (raw)
In-Reply-To: <CALZtONCq2QCcg+5S5f-yHiNp6FBPD9sUHjNiFuhJjm-uc6smtg@mail.gmail.com>

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@kernel.org> 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@ieee.org>
> >> > 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@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-28 15:36 Dan Streetman
2016-04-28 22:07 ` Andrew Morton
2016-04-29  0:38   ` Sergey Senozhatsky
2016-04-29  5:37     ` Minchan Kim
2016-04-29 14:50       ` Dan Streetman
2016-04-29 15:33         ` Minchan Kim [this message]
2016-05-03  2:18 ` Ganesh Mahendran
2016-05-19 15:18   ` [PATCHv2] " Dan Streetman
2016-05-20  2:33     ` Ganesh Mahendran
2016-05-20  4:08     ` Sergey Senozhatsky
2016-05-20 10:32       ` Dan Streetman
2016-05-23  3:03     ` Minchan Kim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160429153309.GA2696@blaptop \
    --to=minchan@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=dan.streetman@canonical.com \
    --cc=ddstreet@ieee.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=ngupta@vflare.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=sergey.senozhatsky@gmail.com \
    --cc=sjenning@redhat.com \
    --cc=yuzhao@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git