Received-SPF: pass (google.com: domain of linux-kernel+bounces-170586-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 6 May 2024 19:31:32 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: <ira.weiny@intel.com>, Dave Jiang <dave.jiang@intel.com>, Fan Ni
	<fan.ni@samsung.com>, Jonathan Cameron <Jonathan.Cameron@huawei.com>, Navneet
 Singh <navneet.singh@intel.com>
CC: Dan Williams <dan.j.williams@intel.com>, Davidlohr Bueso
	<dave@stgolabs.net>, Alison Schofield <alison.schofield@intel.com>, "Vishal
 Verma" <vishal.l.verma@intel.com>, Ira Weiny <ira.weiny@intel.com>,
	<linux-btrfs@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 17/26] dax/region: Create extent resources on DAX region
 driver load
Message-ID: <6639928463d58_2f63a29424@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-17-b7b00d623625@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240324-dcd-type2-upstream-v1-17-b7b00d623625@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?BdV+an+zeUgZrTQSd9qDIj3irzIDoys5xv0w/H+z+hSLM0moMbGFxCpPOuGI?=
 =?us-ascii?Q?o9Di9uszccDyJH9Yo9Qef0zLgkZO1JR+lnoVQ2w9t4vMlxEsM854+anE1/6A?=
 =?us-ascii?Q?Jc51j9YGjqd9Z6vDHf25DioPFpecu3MOWDeNlVXlqIxr4rVsTV/vX4Oa6g0L?=
 =?us-ascii?Q?GMXpHPpJoibDcqAnsaFmklsD7z1LZx6BZeuKeHcxLdLskNwb0QCz1A9f+yDX?=
 =?us-ascii?Q?BTcQ+YCAjtDeUBjlWy4zGok53iFd3zgQhg8CoUSaQy4mGleBu6RSEXuZc3g1?=
 =?us-ascii?Q?X7qT/APTP+PBCPK2Z07hLOCDGyQhE233Dc040LZ+OyXJQZogHjhQSc6vq/IP?=
 =?us-ascii?Q?xha0U81iUxoKwfKAR7xIv9vVN8eYZVxnPurAvbXfPsea5MzjvS8gLwOo80c6?=
 =?us-ascii?Q?c8FwPOHygOftwdD+r6px0AuM8qw1pxLyrty1svg4K4ETyfEzLdEpY4cIwIBM?=
 =?us-ascii?Q?15ymRCQFgQ9s+vT/mLKfW/JygbEPOoJ1rUY7kE6HYlV+yOLBU+FVQNjvLkZn?=
 =?us-ascii?Q?1j6JdQ8ur7Ogs7vo0B4jDWgHkT6H85GoIz6CgSZLiB+xhrBs1HAQ+ADuzmru?=
 =?us-ascii?Q?806K2sC476FEncEY6KSHd4g91cEtOLDWR2+pD7eGyqH7xEBfxZg8wtA2rKCO?=
 =?us-ascii?Q?TFcUoO0oKK2N8elU/wLJMLsjGK9pxSnsl/feuUF/myW3kbfQXkQg9WMK3F4Q?=
 =?us-ascii?Q?24U8UQX+QZ7I9XEGopL6JOB8o7uCel4a1uDwL+Xo3skQBTC1TcqTyZALg7x+?=
 =?us-ascii?Q?GdC4Wc6UQnasJU65p4yQ+L/Ck3p2v4Q5K9YbNeHN8yFa4ck6XzSQI4lvMnEC?=
 =?us-ascii?Q?UMh3z30KOAHEajfVWScaoq5saSCQWKJvTgwCxQlhanung6JiVqLsfdtWGVlz?=
 =?us-ascii?Q?+7/YqZgmGoK3ZNldiyfE1fY5oTUzTDCNzzrEdEyvYs3p1opDGnC79LoWCdAQ?=
 =?us-ascii?Q?75ZxlrW8hdLg9omjI4LjspuTCNLT3+CS3qlUTmWWffVTN0+ZnoW/Jco8mtAm?=
 =?us-ascii?Q?05llSMOb4CcMxXSKuB+PofbBwTaQ5ZeQr5h4grFWPe7w4EFj8zvRxZz3Sdus?=
 =?us-ascii?Q?jyfytbMeLL3hzoK55MOmnd/S7s5FICYUfxjcmLwma/8HEvJ+/sPbsxY/6Zla?=
 =?us-ascii?Q?H0BRqRVqUAbzlOHsl01hfkeCmbxd3Pjo2SjhXs33dsyQyWv3ZHh6hS+JHqDO?=
 =?us-ascii?Q?gORI9VjrfkLhe8BZj8P78SNmJSxRsl1cWvWRnzff9KOCZBSrVCaPBFvAkEL4?=
 =?us-ascii?Q?/CJVHDyxGr3yI6bDCOYCfLvX2RFSWK6CRbWEZX8JB3pAZ9ID+3f77TvME+TN?=
 =?us-ascii?Q?EE3GxVc3HdAY5rUV/I8J7tD8BrRZ8p9R9H3JfFgjcMlpnig/yhWnI4eeX7xb?=
 =?us-ascii?Q?a1kU+UYlHWQvysrDYvnKPYRM/S2wHinUnC7GfaRWUqJiMho+NWYOELDoABwN?=
 =?us-ascii?Q?ZqgEs8zvVUd5OQ0GXVS/yMl00UEnovrKqqL07BY+mhGccbFEiWHJix1vwibI?=
 =?us-ascii?Q?WmGdANyNtfEcU/spOJJ2u+rw+vF7hTKJANo9Rwf50W9J7o7VmN8TBavcFABj?=
 =?us-ascii?Q?LGid+CZitu44i08pwEm1oNByHyyPUh91kxmX74h6A7DaQ9Xp0uMmDO0aw4/p?=
 =?us-ascii?Q?ow=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e6199e9e-63aa-4a9a-40d1-08dc6e3dd07a
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 02:31:35.1427
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9jukrWt0upE1h36KS43To209LvoJhLTCCmu6flk+CtN7aRbryQeQhy9mnlYhIuOGmLnGwh1KI+2Mi43hXFTPIdvIGFlfmuTLBnK+/cOT0l4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB5967
X-OriginatorOrg: intel.com

ira.weiny@ wrote:
> From: Navneet Singh <navneet.singh@intel.com>
> 
> DAX regions mapping dynamic capacity partitions introduce a requirement
> for the memory backing the region to come and go as required.  This
> results in a DAX region with sparse areas of memory backing.  To track
> the sparseness of the region, DAX extent objects need to track
> sub-resource information as a new layer between the DAX region resource
> and DAX device range resources.
> 
> Recall that DCD extents may be accepted when a region is first created.
> Extend this support on region driver load.  Scan existing extents and
> create DAX extent resources as a first step to DAX extent realization.
> 
> The lifetime of a DAX extent is tricky to manage because the extent life
> may end in one of two ways.  First, the device may request the extent be
> released.  Second, the region may release the extent when it is
> destroyed without hardware involvement.  Support extent release without
> hardware involvement first.  Subsequent patches will provide for
> hardware to request extent removal.
> 
> Signed-off-by: Navneet Singh <navneet.singh@intel.com>
> Co-developed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes for v1
> [iweiny: remove xarrays]
> [iweiny: remove as much of extra reference stuff as possible]
> [iweiny: Move extent resource handling to core DAX code]
> ---
>  drivers/dax/bus.c         | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/dax/cxl.c         | 43 ++++++++++++++++++++++++++++++++++--
>  drivers/dax/dax-private.h | 12 +++++++++++
>  3 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 903566aff5eb..4d5ed7ab6537 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -186,6 +186,61 @@ static bool is_sparse(struct dax_region *dax_region)
>  	return (dax_region->res.flags & IORESOURCE_DAX_SPARSE_CAP) != 0;
>  }
>  
> +static int dax_region_add_resource(struct dax_region *dax_region,
> +				   struct dax_extent *dax_ext,
> +				   resource_size_t start,
> +				   resource_size_t length)
> +{
> +	struct resource *ext_res;
> +
> +	dev_dbg(dax_region->dev, "DAX region resource %pr\n", &dax_region->res);
> +	ext_res = __request_region(&dax_region->res, start, length, "extent", 0);
> +	if (!ext_res) {
> +		dev_err(dax_region->dev, "Failed to add region s:%pa l:%pa\n",
> +			&start, &length);
> +		return -ENOSPC;
> +	}
> +
> +	dax_ext->region = dax_region;
> +	dax_ext->res = ext_res;
> +	dev_dbg(dax_region->dev, "Extent add resource %pr\n", ext_res);

dax_ext is never used, it feels like this these helpers are in the wrong
patch. Like consumer side of dax_ext infrastructure lands *before* the
producer side.

Because the justification for this producer-side patch is after the case
for 'struct dax_extent' has been made.

> +int dax_region_add_extent(struct dax_region *dax_region, struct device *ext_dev,
> +			  resource_size_t start, resource_size_t length)
> +{
> +	int rc;
> +
> +	struct dax_extent *dax_ext __free(kfree) = kzalloc(sizeof(*dax_ext),
> +							   GFP_KERNEL);
> +	if (!dax_ext)
> +		return -ENOMEM;
> +
> +	guard(rwsem_write)(&dax_region_rwsem);
> +	rc = dax_region_add_resource(dax_region, dax_ext, start, length);
> +	if (rc)
> +		return rc;
> +
> +	return devm_add_action_or_reset(ext_dev, dax_region_release_extent,
> +					no_free_ptr(dax_ext));

This looks like an awkward rewrite of __devm_request_region(), but
likely that is because dax_ext is vestigial in this patch.

> +static void cxl_dax_region_add_extents(struct cxl_dax_region *cxlr_dax,
> +				      struct dax_region *dax_region)
> +{
> +	dev_dbg(&cxlr_dax->dev, "Adding extents\n");
> +	device_for_each_child(&cxlr_dax->dev, dax_region, cxl_dax_region_add_extent);

Per the comment on the last patch to move extent device creation to
cxl_dax_region_probe() that can get rid of looping over those devices
another time.