All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@lists.01.org>,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 1/6] pmem: remove indirection layer arch_has_pmem_api()
Date: Fri, 07 Aug 2015 12:41:37 -0600	[thread overview]
Message-ID: <1438972897.2293.2.camel@linux.intel.com> (raw)
In-Reply-To: <CAPcyv4jun2dE9=24Wcf4Qh_JruNyf=5qoMckqdg3u0ROUgPZ8w@mail.gmail.com>

On Fri, 2015-08-07 at 09:14 -0700, Dan Williams wrote:
> On Thu, Aug 6, 2015 at 10:43 AM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > Prior to this change arch_has_wmb_pmem() was only called by
> > arch_has_pmem_api().  Both arch_has_wmb_pmem() and arch_has_pmem_api()
> > checked to make sure that CONFIG_ARCH_HAS_PMEM_API was enabled.
> >
> > Instead, remove one extra layer of indirection and the redundant
> > CONFIG_ARCH_HAS_PMEM_API check, and just have arch_has_pmem_api()
> > call __arch_has_wmb_pmem() directly.
> 
> So I think this patch takes us further away from where we want to go
> in the near term which is a finer grained pmem api.  The class of
> systems where (has_pmem_api() && !has_wmb_pmem()) is existing energy
> backed nvdimm platforms.  I'm assuming those platforms will want to
> assert persistence guarantees in the absence of a pcommit-like
> instruction, and that we want to stop gating arch_has_pmem_api() on
> the presence of wmb_pmem() capability.  In that case
> arch_has_wmb_pmem() will be useful to have and that was the original
> intent for including it, that intent did not seem to comprehended in
> the changelog.

I think that we should only keep around functions that are actually used and
useful.  I agree that there could potentially be a use for a distinction like
the one you are talking about when we try and properly support ADR systems and
stop lumping them in with "non-PCOMMIT" systems like we are doing now.

Right now, though, arch_has_wmb_pmem() is just redundant.  If/when we add that
new support in, we'll have to properly update this code anyway - let's not
keep around unneeded code until then.



WARNING: multiple messages have this Message-ID (diff)
From: Ross Zwisler <ross.zwisler@linux.intel.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-nvdimm@lists.01.org" <linux-nvdimm@ml01.01.org>,
	Christoph Hellwig <hch@infradead.org>
Subject: Re: [PATCH 1/6] pmem: remove indirection layer arch_has_pmem_api()
Date: Fri, 07 Aug 2015 12:41:37 -0600	[thread overview]
Message-ID: <1438972897.2293.2.camel@linux.intel.com> (raw)
In-Reply-To: <CAPcyv4jun2dE9=24Wcf4Qh_JruNyf=5qoMckqdg3u0ROUgPZ8w@mail.gmail.com>

On Fri, 2015-08-07 at 09:14 -0700, Dan Williams wrote:
> On Thu, Aug 6, 2015 at 10:43 AM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > Prior to this change arch_has_wmb_pmem() was only called by
> > arch_has_pmem_api().  Both arch_has_wmb_pmem() and arch_has_pmem_api()
> > checked to make sure that CONFIG_ARCH_HAS_PMEM_API was enabled.
> >
> > Instead, remove one extra layer of indirection and the redundant
> > CONFIG_ARCH_HAS_PMEM_API check, and just have arch_has_pmem_api()
> > call __arch_has_wmb_pmem() directly.
> 
> So I think this patch takes us further away from where we want to go
> in the near term which is a finer grained pmem api.  The class of
> systems where (has_pmem_api() && !has_wmb_pmem()) is existing energy
> backed nvdimm platforms.  I'm assuming those platforms will want to
> assert persistence guarantees in the absence of a pcommit-like
> instruction, and that we want to stop gating arch_has_pmem_api() on
> the presence of wmb_pmem() capability.  In that case
> arch_has_wmb_pmem() will be useful to have and that was the original
> intent for including it, that intent did not seem to comprehended in
> the changelog.

I think that we should only keep around functions that are actually used and
useful.  I agree that there could potentially be a use for a distinction like
the one you are talking about when we try and properly support ADR systems and
stop lumping them in with "non-PCOMMIT" systems like we are doing now.

Right now, though, arch_has_wmb_pmem() is just redundant.  If/when we add that
new support in, we'll have to properly update this code anyway - let's not
keep around unneeded code until then.



  reply	other threads:[~2015-08-07 18:41 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-06 17:43 [PATCH 0/6] pmem, dax: I/O path enhancements Ross Zwisler
2015-08-06 17:43 ` Ross Zwisler
2015-08-06 17:43 ` [PATCH 1/6] pmem: remove indirection layer arch_has_pmem_api() Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-07  6:38   ` Christoph Hellwig
2015-08-07 14:07     ` Ross Zwisler
2015-08-07 16:14   ` Dan Williams
2015-08-07 16:14     ` Dan Williams
2015-08-07 18:41     ` Ross Zwisler [this message]
2015-08-07 18:41       ` Ross Zwisler
2015-08-07 20:01       ` Dan Williams
2015-08-07 20:01         ` Dan Williams
2015-08-06 17:43 ` [PATCH 2/6] x86: clean up conditional pmem includes Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-07  6:39   ` Christoph Hellwig
2015-08-07 14:08     ` Ross Zwisler
2015-08-06 17:43 ` [PATCH 3/6] x86: add clwb_cache_range() Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-06 17:43 ` [PATCH 4/6] pmem: Add wb_cache_pmem() and flush_cache_pmem() Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-06 17:43 ` [PATCH 5/6] nd_blk: add support for "read flush" DSM flag Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-06 17:43 ` [PATCH 6/6] dax: update I/O path to do proper PMEM flushing Ross Zwisler
2015-08-06 17:43   ` Ross Zwisler
2015-08-06 21:04   ` Dave Chinner
2015-08-06 21:04     ` Dave Chinner
2015-08-07 19:08     ` Ross Zwisler
2015-08-07 19:08       ` Ross Zwisler
2015-08-06 21:26   ` Dan Williams
2015-08-06 21:26     ` Dan Williams
2015-08-07 16:47 ` [PATCH 0/6] pmem, dax: I/O path enhancements Dan Williams
2015-08-07 16:47   ` Dan Williams
2015-08-07 19:06   ` Ross Zwisler
2015-08-07 19:06     ` Ross Zwisler

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=1438972897.2293.2.camel@linux.intel.com \
    --to=ross.zwisler@linux.intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=hch@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvdimm@lists.01.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.