Re: [ndctl PATCH v4 0/6] Add partitioning support for CXL memdevs

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Feb 08, 2022 at 09:23:54AM -0800, Dan Williams wrote:
> On Mon, Feb 7, 2022 at 3:06 PM <alison.schofield@intel.com> wrote:
> >
> > From: Alison Schofield <alison.schofield@intel.com>
> >
> > Users may want to view and change partition layouts of CXL memory
> > devices that support partitioning. Provide userspace access to
> > the device partitioning capabilities as defined in the CXL 2.0
> > specification.
> 
> This is minor feedback if these end up being re-spun, but "Users may
> want..." is too passive for what this is, which is a critical building
> block in the provisioning model for PMEM over CXL. So, consider
> rewriting in active voice, and avoid underselling the importance of
> this capability.

Yes! I have some words you gave me in another commit I will draw upon.

> 
> > The first 4 patches add accessors for all the information needed
> > to formulate a new partition layout. This info is accessible via
> > the libcxl API and a new option in the cxl list command:
> >
> >     "partition_info":{
> >       "active_volatile_bytes":273535729664,
> >       "active_persistent_bytes":0,
> >       "next_volatile_bytes":268435456,
> >       "next_persistent_bytes":273267294208,
> >       "total_bytes":273535729664,
> >       "volatile_only_bytes":0,
> >       "persistent_only_bytes":0,
> >       "partition_alignment_bytes":268435456
> >     }
> 
> Is this stale? I.e. we discussed aligning the names to other
> 'size'-like values in 'ndctl list' and 'cxl list'.
> 

Yes - that is STALE. The cxl-list patch commit msg has it right.
Will fix here.

 "partition_info":{
      "active_volatile_size":273535729664,
      "active_persistent_size":0,
      "next_volatile_size":0,
      "next_persistent_size":0,
      "total_size":273535729664,
      "volatile_only_size":0,
      "persistent_only_size":0,
      "partition_alignment_size":268435456
    }



> >
> > Patch 5 introduces the libcxl interfaces for the SET_PARTITION_INFO
> > mailbox command and Patch 6 adds the new CXL command:
> >
> > Synopsis:
> > cxl set-partition <mem0> [<mem1>..<memN>] [<options>]
> >
> > -t, --type=<type>       'pmem' or 'volatile' (Default: 'pmem')
> > -s, --size=<size>       size in bytes (Default: all partitionable capacity)
> 
> Spell-check does not like "partitionable"
> 
> s/partitionable/available/

hmm... passes my spell check, but alas, it is overuse of the root word.
I like available. Will change.

> 
> > -a, --align             allow alignment correction
> 
> How about:
> 
> "Auto-align --size per device's requirement."
> 

So much better. Thanks.

> > -v, --verbose           turn on debug
> >
> > The CXL command does not offer the IMMEDIATE mode option defined
> 
> s/CXL/'cxl set-parition'/
> 
> This is a general problem caused by the tool 'cxl' being the same name
> as the specification CXL. When it is ambiguous, go ahead and spell out
> 'cxl <command>'.
> 

Got it.

snip...