Re: [ndctl PATCH 4/6] cxl/region: Add creation of Dynamic capacity regions

[Fan Ni <nifan.cxl@gmail.com>]

On Thu, Oct 31, 2024 at 05:11:04PM -0500, Ira Weiny wrote:
> Fan Ni wrote:
> > On Wed, Oct 30, 2024 at 04:54:47PM -0500, ira.weiny@intel.com wrote:
> > > From: Navneet Singh <navneet.singh@intel.com>
> > > 
> > > CXL Dynamic Capacity Devices (DCDs) optionally support dynamic capacity
> > > with up to eight partitions (Regions) (dc0-dc7).  CXL regions can now be
> > > spare and defined as dynamic capacity (dc).
> > > 
> > > Add support for DCD devices.  Query for DCD capabilities.  Add the
> > > ability to add DC partitions to a CXL DC region.
> > > 
> > > Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> > > Co-authored-by: Sushant1 Kumar <sushant1.kumar@intel.com>
> > > Signed-off-by: Sushant1 Kumar <sushant1.kumar@intel.com>
> > > Co-authored-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > ---
> > > Changes:
> > > [iweiny: adjust to new sysfs interface.]
> > > [iweiny: Rebase to latest pending]
> > > [iweiny: Adjust DCD region code to new upstream sysfs entries]
> > > [iweiny: Ensure backwards compatibility for non-DC kernels]
> > > [iweiny: fixup help message to show DC type]
> > > [iweiny: don't double declare decoder mode is dc]
> > > [iweiny: simplify __reserve_dpa() with decoder mode to index]
> > > [iweiny: Adjust to the new region mode]
> > > ---
> > >  cxl/json.c         | 26 +++++++++++++++
> > >  cxl/lib/libcxl.c   | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > >  cxl/lib/libcxl.sym |  3 ++
> > >  cxl/lib/private.h  |  6 +++-
> > >  cxl/libcxl.h       | 55 +++++++++++++++++++++++++++++--
> > >  cxl/memdev.c       |  7 +++-
> > >  cxl/region.c       | 49 ++++++++++++++++++++++++++--
> > >  7 files changed, 234 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/cxl/json.c b/cxl/json.c
> > > index dcd3cc28393faf7e8adf299a857531ecdeaac50a..4276b9678d7e03eaf2aec581a08450f2a0b857f2 100644
> > > --- a/cxl/json.c
> > > +++ b/cxl/json.c
> > > @@ -754,10 +754,12 @@ err_free:
> > >  	return jpoison;
> > >  }
> > >  
> > > +#define DC_SIZE_NAME_LEN 64
> > >  struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
> > >  		unsigned long flags)
> > >  {
> > >  	const char *devname = cxl_memdev_get_devname(memdev);
> > > +	char size_name[DC_SIZE_NAME_LEN];
> > >  	struct json_object *jdev, *jobj;
> > >  	unsigned long long serial, size;
> > >  	const char *fw_version;
> > > @@ -800,6 +802,17 @@ struct json_object *util_cxl_memdev_to_json(struct cxl_memdev *memdev,
> > >  		}
> > >  	}
> > >  
> > > +	for (int index; index < MAX_NUM_DC_REGIONS; index++) {
> > 
> > index is not initialized.
> > Should be index = 0;
> 
> Thanks for the review!
> 
> Good catch.  I'll fix up.
> 
> > 
> > Also, the "cxl list" looks like below, the size of each DC region is
> > attached to each DCD device, that seems not quite aligned with what
> > "_size" means for pmem/ram. Should we have a separate option for "cxl
> > list" to show DC region info??
> 
> I'm not sure I follow.  The pmem/ram sizes show the size of the partitions on
> the memdev.  This is the same for each DC partition.
> 
> Are you looking for the available size after some extents are available?
> 
> In that case I think you are looking for the dax information details which
> comes after creating a region and using the -X option.
> 
> 17:02:42 > ./build/cxl/cxl list -r 8 -X
> [
>   {
>     "region":"region8",
>     "resource":1031597457408,
>     "size":536870912,
>     "type":"dc",
>     "interleave_ways":1,
>     "interleave_granularity":256,
>     "decode_state":"commit",
>     "daxregion":{
>       "id":8,
>       "size":536870912,
>       "available_size":134217728,
>       "align":2097152
>     }
>   }
> ]
> 
> 
> This shows an available size which can further be dissected with the new
> --extents (-N) option added in patch 5/6.
> 
> 17:04:32 > ./build/cxl/cxl list -r 8 -X -N
> [
>   {
>     "region":"region8",
>     "resource":1031597457408,
>     "size":536870912,
>     "type":"dc",
>     "interleave_ways":1,
>     "interleave_granularity":256,
>     "decode_state":"commit",
>     "daxregion":{
>       "id":8,
>       "size":536870912,
>       "available_size":134217728,
>       "align":2097152
>     },
>     "extents":[
>       {
>         "offset":268435456,
>         "length":67108864,
>         "tag":"00000000-0000-0000-0000-000000000000"
>       },
>       {
>         "offset":134217728,
>         "length":67108864,
>         "tag":"00000000-0000-0000-0000-000000000000"
>       }
>     ]
>   }
> ]
> 
> 
> Does this give you the information you are looking for?  Or am I missing
> something in your question?
> 
> Ira
I was looking for something like the "-N" option provides, so I think we
are good.

Fan
> 
> > 
> > Fan
> > 
> > ----------
> >   {
> >         "memdev":"mem1",
> >         "dc0_size":"2.00 GiB (2.15 GB)",
> >         "dc1_size":"2.00 GiB (2.15 GB)",
> >         "serial":"0xf02",
> >         "host":"0000:11:00.0",
> >         "firmware_version":"BWFW VERSION 00"
> >       },
> >       {
> >         "memdev":"mem3",
> >         "dc0_size":"2.00 GiB (2.15 GB)",
> >         "dc1_size":"2.00 GiB (2.15 GB)",
> >         "serial":"0xf03",
> >         "host":"0000:12:00.0",
> >         "firmware_version":"BWFW VERSION 00"
> >       },
> > ----------
> > 
> 
> [snip]

-- 
Fan Ni