mbox series

[ndctl,v4,0/6] Add partitioning support for CXL memdevs

Message ID cover.1644271559.git.alison.schofield@intel.com (mailing list archive)
Headers show
Series Add partitioning support for CXL memdevs | expand

Message

Alison Schofield Feb. 7, 2022, 11:10 p.m. UTC 
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.

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
    }

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)
-a, --align             allow alignment correction
-v, --verbose           turn on debug

The CXL command does not offer the IMMEDIATE mode option defined
in the CXL 2.0 spec because the CXL kernel driver does not support
immediate mode yet. Since userspace could use the libcxl API to
send a SET_PARTITION_INFO mailbox command with immediate mode
selected, a kernel patch that rejects such requests is in review
for the CXL driver.

Changes in v4: (from Dan's review)
- cxl set-partition command: add type (pmem | volatile),
  add defaults for type and size, and add an align option.
- Replace macros with return statements with functions.
- Add cxl_set_partition_set_mode() to the libcxl API.
- Add API documentation to Documentation/cxl/lib/libcxl.txt.
- Use log_err/info mechanism.
- Display memdev JSON info upon completion of set-partition command.
- Remove unneeded casts when accessing command payloads.
- Name changes - like drop _info suffix, use _size instead of _bytes.

Changes in v3:
- Back out the API name change to the partition align accessor.
  The API was already released in v72.
- Man page: collapse into one .txt file. 
- Man page: add to Documentation/meson.build 

Changes in v2:
- Rebase onto https://github.com/pmem/ndctl.git djbw/meson.
- Clarify that capacities are reported in bytes. (Ira)
  This led to renaming accessors like *_capacity to  *_bytes and 
  a spattering of changes in the man pages and commit messages.
- Add missing cxl_cmd_unref() when creating json objects. (Ira)
- Change the cxl list option to: -I --partition (Dan) 
- Use OPT_STRING for the --volatile_size parameter (Dan, Vishal)
- Drop extra _get_ in accessor names. (Vishal)
- Add an accessor for the SET_PARTITION_INFO mbox cmd flag.
- Display usage_with_options if size parameter is missing.
- Drop the OPT64 define patch. Use OPT_STRING instead.

Alison Schofield (6):
  libcxl: add GET_PARTITION_INFO mailbox command and accessors
  libcxl: add accessors for capacity fields of the IDENTIFY command
  libcxl: return the partition alignment field in bytes
  cxl: add memdev partition information to cxl-list
  libcxl: add interfaces for SET_PARTITION_INFO mailbox command
  cxl: add command set-partition

 Documentation/cxl/cxl-list.txt          |  23 +++
 Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
 Documentation/cxl/lib/libcxl.txt        |   5 +
 Documentation/cxl/meson.build           |   1 +
 cxl/builtin.h                           |   1 +
 cxl/cxl.c                               |   1 +
 cxl/filter.c                            |   2 +
 cxl/filter.h                            |   1 +
 cxl/json.c                              | 113 ++++++++++++++
 cxl/lib/libcxl.c                        | 123 ++++++++++++++-
 cxl/lib/libcxl.sym                      |  10 ++
 cxl/lib/private.h                       |  18 +++
 cxl/libcxl.h                            |  18 +++
 cxl/list.c                              |   2 +
 cxl/memdev.c                            | 196 ++++++++++++++++++++++++
 util/json.h                             |   1 +
 util/size.h                             |   1 +
 17 files changed, 575 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/cxl/cxl-set-partition.txt

Comments

Dan Williams Feb. 8, 2022, 5:23 p.m. UTC | #1 
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.

> 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'.

>
> 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/

> -a, --align             allow alignment correction

How about:

"Auto-align --size per device's requirement."

> -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>'.

> in the CXL 2.0 spec because the CXL kernel driver does not support
> immediate mode yet. Since userspace could use the libcxl API to
> send a SET_PARTITION_INFO mailbox command with immediate mode
> selected, a kernel patch that rejects such requests is in review
> for the CXL driver.
>
> Changes in v4: (from Dan's review)
> - cxl set-partition command: add type (pmem | volatile),
>   add defaults for type and size, and add an align option.
> - Replace macros with return statements with functions.
> - Add cxl_set_partition_set_mode() to the libcxl API.
> - Add API documentation to Documentation/cxl/lib/libcxl.txt.
> - Use log_err/info mechanism.
> - Display memdev JSON info upon completion of set-partition command.
> - Remove unneeded casts when accessing command payloads.
> - Name changes - like drop _info suffix, use _size instead of _bytes.
>
> Changes in v3:
> - Back out the API name change to the partition align accessor.
>   The API was already released in v72.
> - Man page: collapse into one .txt file.
> - Man page: add to Documentation/meson.build
>
> Changes in v2:
> - Rebase onto https://github.com/pmem/ndctl.git djbw/meson.
> - Clarify that capacities are reported in bytes. (Ira)
>   This led to renaming accessors like *_capacity to  *_bytes and
>   a spattering of changes in the man pages and commit messages.
> - Add missing cxl_cmd_unref() when creating json objects. (Ira)
> - Change the cxl list option to: -I --partition (Dan)
> - Use OPT_STRING for the --volatile_size parameter (Dan, Vishal)
> - Drop extra _get_ in accessor names. (Vishal)
> - Add an accessor for the SET_PARTITION_INFO mbox cmd flag.
> - Display usage_with_options if size parameter is missing.
> - Drop the OPT64 define patch. Use OPT_STRING instead.
>
> Alison Schofield (6):
>   libcxl: add GET_PARTITION_INFO mailbox command and accessors
>   libcxl: add accessors for capacity fields of the IDENTIFY command
>   libcxl: return the partition alignment field in bytes
>   cxl: add memdev partition information to cxl-list
>   libcxl: add interfaces for SET_PARTITION_INFO mailbox command
>   cxl: add command set-partition
>
>  Documentation/cxl/cxl-list.txt          |  23 +++
>  Documentation/cxl/cxl-set-partition.txt |  60 ++++++++
>  Documentation/cxl/lib/libcxl.txt        |   5 +
>  Documentation/cxl/meson.build           |   1 +
>  cxl/builtin.h                           |   1 +
>  cxl/cxl.c                               |   1 +
>  cxl/filter.c                            |   2 +
>  cxl/filter.h                            |   1 +
>  cxl/json.c                              | 113 ++++++++++++++
>  cxl/lib/libcxl.c                        | 123 ++++++++++++++-
>  cxl/lib/libcxl.sym                      |  10 ++
>  cxl/lib/private.h                       |  18 +++
>  cxl/libcxl.h                            |  18 +++
>  cxl/list.c                              |   2 +
>  cxl/memdev.c                            | 196 ++++++++++++++++++++++++
>  util/json.h                             |   1 +
>  util/size.h                             |   1 +
>  17 files changed, 575 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/cxl/cxl-set-partition.txt
>
> --
> 2.31.1
>
Alison Schofield Feb. 8, 2022, 6 p.m. UTC | #2 
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...