Re: [PATCH RFC v2 11/18] cxl/region: Expose DC extents on region driver load

From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Ira Weiny <ira.weiny@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	Navneet Singh <navneet.singh@intel.com>,
	Fan Ni <fan.ni@samsung.com>, Davidlohr Bueso <dave@stgolabs.net>,
	Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	Vishal Verma <vishal.l.verma@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH RFC v2 11/18] cxl/region: Expose DC extents on region driver load
Date: Tue, 29 Aug 2023 17:20:48 +0100	[thread overview]
Message-ID: <20230829172048.000006fb@Huawei.com> (raw)
In-Reply-To: <20230604-dcd-type2-upstream-v2-11-f740c47e7916@intel.com>

On Mon, 28 Aug 2023 22:21:02 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> Ultimately user space must associate Dynamic Capacity (DC) extents with
> DAX devices.  Remember also that DCD extents may have been accepted
> previous to regions being created and must have references held until
> all higher level regions and DAX devices are done with the memory.
> 
> On CXL region driver load scan existing device extents and create CXL
> DAX region extents as needed.
> 
> Create abstractions for the extents to be used in DAX region.  This
> includes a generic interface to take proper references on the lower
> level CXL region extents.
> 
> Also maintain separate objects for the DAX region extent device vs the
> DAX region extent.  The DAX region extent device has a shorter life span
> which corresponds to the removal of an extent while a DAX device is
> still using it.  In this case an extent continues to exist whilst the
> ability to create new DAX devices on that extent is prevented.
> 
> NOTE: Without interleaving; the device, CXL region, and DAX region
> extents have a 1:1:1 relationship.  Future support for interleaving will
> maintain a 1:N relationship between CXL region extents and the hardware
> extents.
> 
> While the ability to create DAX devices on an extent exists; expose the
> necessary details of DAX region extents by creating a device with the
> following sysfs entries.
> 
> /sys/bus/cxl/devices/dax_regionX/extentY
> /sys/bus/cxl/devices/dax_regionX/extentY/length
> /sys/bus/cxl/devices/dax_regionX/extentY/label
> 
> Label is a rough analogy to the DC extent tag.  As such the DC extent
> tag is used to initially populate the label.  However, the label is made
> writeable so that it can be adjusted in the future when forming a DAX
> device.
> 
> Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> Co-developed-by: Navneet Singh <navneet.singh@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 

Trivial stuff inline.

> diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> index 27cf2daaaa79..4dab52496c3f 100644
> --- a/drivers/dax/dax-private.h
> +++ b/drivers/dax/dax-private.h
> @@ -5,6 +5,7 @@
>  #ifndef __DAX_PRIVATE_H__
>  #define __DAX_PRIVATE_H__
>  
> +#include <linux/pgtable.h>
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/idr.h>
> @@ -40,6 +41,58 @@ struct dax_region {
>  	struct device *youngest;
>  };
>  
> +/*
/**

as it's valid kernel doc so no disadvantage really.

> + * struct dax_region_extent - extent data defined by the low level region
> + * driver.
> + * @private_data: lower level region driver data
> + * @ref: track number of dax devices which are using this extent
> + * @get: get reference to low level data
> + * @put: put reference to low level data

I'd like to understand when these are optional - perhaps comment on that?

> + */
> +struct dax_region_extent {
> +	void *private_data;
> +	struct kref ref;
> +	void (*get)(struct dax_region_extent *dr_extent);
> +	void (*put)(struct dax_region_extent *dr_extent);
> +};
> +
> +static inline void dr_extent_get(struct dax_region_extent *dr_extent)
> +{
> +	if (dr_extent->get)
> +		dr_extent->get(dr_extent);
> +}
> +
> +static inline void dr_extent_put(struct dax_region_extent *dr_extent)
> +{
> +	if (dr_extent->put)
> +		dr_extent->put(dr_extent);
> +}
> +
> +#define DAX_EXTENT_LABEL_LEN 64

blank line here.

> +/**
> + * struct dax_reg_ext_dev - Device object to expose extent information
> + * @dev: device representing this extent
> + * @dr_extent: reference back to private extent data
> + * @offset: offset of this extent
> + * @length: size of this extent
> + * @label: identifier to group extents
> + */
> +struct dax_reg_ext_dev {
> +	struct device dev;
> +	struct dax_region_extent *dr_extent;
> +	resource_size_t offset;
> +	resource_size_t length;
> +	char label[DAX_EXTENT_LABEL_LEN];
> +};