Received-SPF: pass (google.com: domain of linux-kernel+bounces-170676-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 6 May 2024 22:04:23 -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 18/26] cxl/mem: Handle DCD add & release capacity events.
Message-ID: <6639b657da437_2f63a29410@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-18-b7b00d623625@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240324-dcd-type2-upstream-v1-18-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?G4AKbG4czHcHRFCOXTdxEgSML1etujCmtq7zWb+y3ZY7c19mYFUeI5k57vpv?=
 =?us-ascii?Q?mKafpgSjLJ9tHkJykWPTvioN+fAjZQP3FDx7qvEcHpjhVM5OwnV7YDkzX2Tf?=
 =?us-ascii?Q?7dBxfNxpR8lUJCO0XkBIEf91DPItX28kU1ch5fhU8SxjqjRX5PjztGNO75Z2?=
 =?us-ascii?Q?Zya2VZZVuW7owA2agMJ8v6pMQbB3GFIok4+fEFyEbl0i+6bH5sk7a9+uM8vN?=
 =?us-ascii?Q?SVz3jSt0Z+LdrYrrN7FSbz7T3358yvLr0FBY32MRN1uYgOUEmW7D6u8B/3ft?=
 =?us-ascii?Q?cmhDry5iNJ1CDKD6wmK9yQ37b6onDL+Xv5AFuhmJgX+Nrg9803Wu0N6Bkmih?=
 =?us-ascii?Q?q5iFpi6bZKYyhUPTPymhgRhRYjUbWEHVdFKZYXFql+DM+HOdejEYq8A7zc7D?=
 =?us-ascii?Q?gtG83s1eKZW+dbebGfAjM3tLXS7kAoFZimMyloLJXCeaMD0rgAUDbu4NPUIx?=
 =?us-ascii?Q?mRvnJgN6Qxq3Q58oxESL8EmyPWm3pWO+n+tBylsIl9whfQAPEhvHx5hCRZE9?=
 =?us-ascii?Q?fzCCFueSyX2vWRuF0nXtr1a1dlaB7swVlpISn+yJ4vHw9w85EsDGqPrUxOut?=
 =?us-ascii?Q?Zu3bbiR0GwNl2X3llXFQi2v7lJLIx0G9Vnbby+YS9at7twsA6Tid0UKfWaHl?=
 =?us-ascii?Q?Ss6MdLkx5eARkYgLVpZnOCVje1l0RXOyaitIZ/YFWoxpaZlYSSKS0lPbhEtH?=
 =?us-ascii?Q?dy+rJxrBQsVRmqptCJQc3klLLTec5bfo7Y75i56eDXzMp/t41Oi5qHd4LuCQ?=
 =?us-ascii?Q?hCI4ZISHXliL1zUhjGbFoVNg6XOuzFRh9Dbdl6L7HAlLfNnZ68MULexjCbi4?=
 =?us-ascii?Q?X8Ocf8H8hknUVuDvdBmcEoZ9kT6a9ge0eoO0wS+z8HfeMfell8NUcgpkC/Jd?=
 =?us-ascii?Q?oHyzgJajTa7kDCGnPVBQwRAKv9UzN1NUSIyMUi03cyaGQECnbxXiB02/NAFK?=
 =?us-ascii?Q?mlu3aKgrZxOL6KCdn3sxBcHHvuikx8P8FRWb7kQemvwZQlfVpPIE+GJH9R+U?=
 =?us-ascii?Q?jomLVui1/g/aO7yrkO5eOebzpqySrEP8ocvm/UIpXtnqfFbYRW+G1SndzTBi?=
 =?us-ascii?Q?TqOzzbHTSyGyjDmmUQG3gdgxpczxXjKcwie7QljLs/mJlQtLhfCsut5nQvxd?=
 =?us-ascii?Q?iSc+QAQx1q9NEUsD3Lkpb8SmTrzq3Zs5LIGzXqWfTfT8njscn+k5Iq7nuXQZ?=
 =?us-ascii?Q?BhjG/sBL3I5AgfHE6Eeh7Cq+hs0SABUUaUecW7n35iN7lCZ9V4zYJqkLtUli?=
 =?us-ascii?Q?QsdZeb7hDk2QXC6QIwToR9M2wBAoeH6hufiUMZvshwlt0XocXLcPkDJY+REW?=
 =?us-ascii?Q?Z47+wHECenWZaE6CO0tPF5AvxN5cLSmuDqc+mYWeND/sVHkG7O9eLsiTkmPY?=
 =?us-ascii?Q?kDMeHDDDVkQf73FxPJO4BY6gpz3kqmGhKz4arnrVxUi5nsupUTpQREg2mJEH?=
 =?us-ascii?Q?PdbT/nBfrycLSonw/EGOv4MK0cfWbYUrTkZ0XiUVkmUsZFwPLioYuRrzTaDn?=
 =?us-ascii?Q?b2GRRogxkLyPeYbiQ0onFezkQU4rv8s48oOwXYC2c7dW1YgzzwOGn6HCHDy1?=
 =?us-ascii?Q?W5J9cJ+oHYsrHdLwotpq2OxAIyJ5ddmLu/d04u+v29ejPoYzFbwhUSFyPWuF?=
 =?us-ascii?Q?jQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 55bd929c-734c-4f40-4ab0-08dc6e532b49
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 05:04:26.9092
 (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: ecNnTZB8KrNeN/B1h43KSHt/AdBoHVhSmy1k9OX5T5Ikp0lLkyuAEtSQJM10RM33nRr9yAKjPO8/+qnEfJDUU8VEbyqqoTPiTeBm1kY0E2w=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6935
X-OriginatorOrg: intel.com

ira.weiny@ wrote:
> From: Navneet Singh <navneet.singh@intel.com>
> 
> A dynamic capacity devices (DCD) send events to signal the host about
> changes in the availability of Dynamic Capacity (DC) memory.  These
> events contain extents, the addition or removal of which may occur at
> any time.
> 
> Adding memory is straight forward.  If no region exists the extent is
> rejected.  If a region does exist, a region extent is formed and
> surfaced.
> 
> Removing memory requires checking if the memory is currently in use.
> Memory use tracking is added in a subsequent patch so here the memory is
> never in use and the removal occurs immediately.
> 
> Most often extents will be offered to and accepted by the host in well
> defined chunks.  However, part of an extent may be requested for
> release.  Simplify extent tracking by signaling removal of any extent
> which overlaps the requested release range.
> 
> Force removal is intended as a mechanism between the FM and the device
> and intended only when the host is unresponsive or otherwise broken.
> Purposely ignore force removal events.
> 
> Process DCD extents.
> 
> Recall that all devices of an interleave set must offer a corresponding
> extent for the region extent to be realized.  This patch limits
> interleave to 1.  Thus the 1:1 mapping between device extent and DAX
> region extent allows immediate surfacing.
> 
> 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 all xarrays]
> [iweiny: entirely new architecture]
> ---
>  drivers/cxl/core/extent.c |   4 ++
>  drivers/cxl/core/mbox.c   | 142 +++++++++++++++++++++++++++++++++++++++++++---
>  drivers/cxl/core/region.c | 139 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/cxl/cxl.h         |  34 +++++++++++
>  drivers/cxl/cxlmem.h      |  21 +++----
>  drivers/cxl/mem.c         |  45 +++++++++++++++
>  drivers/dax/cxl.c         |  22 +++++++
>  include/linux/cxl-event.h |  31 ++++++++++
>  8 files changed, 405 insertions(+), 33 deletions(-)
> 
[..]
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 7635ff109578..a07d95136f0d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[..]
> @@ -1502,18 +1552,7 @@ int cxl_ed_add_one_extent(struct cxl_endpoint_decoder *cxled,
>  	dev_dbg(dev, "Adding DC extent DPA %#llx - %#llx\n",
>  		ext_dpa_range.start, ext_dpa_range.end);
>  
> -	/*
> -	 * Without interleave...
> -	 * HPA offset == DPA offset
> -	 * ... but do the math anyway
> -	 */
> -	dpa_offset = ext_dpa_range.start - cxled->dpa_res->start;
> -	hpa = cxled->cxld.hpa_range.start + dpa_offset;
> -
> -	ext_hpa_range = (struct range) {
> -		.start = hpa - cxlr->cxlr_dax->hpa_range.start,
> -		.end = ext_hpa_range.start + range_len(&ext_dpa_range) - 1,
> -	};

Please don't refactor code that just got added in the same series. Upon
seeing that this wants a common helper in this patch, go back to the
original patch and put it in a helper from the beginning.

[..]
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 5379ad7f5852..156d7c9a8de5 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
[..]
> @@ -891,10 +900,18 @@ bool is_cxl_region(struct device *dev);
>  
>  extern struct bus_type cxl_bus_type;

I skipped ahead here in the review since notification organization feels
wrong.

> +/* Driver Notifier Data */
> +struct cxl_drv_nd {

I never would have guessed that cxl_drv_nd meant cxl driver notifier
data, it might be able to be jettisoned.

> +	enum dc_event event;
> +	struct cxl_dc_extent *dc_extent;
> +	struct region_extent *reg_ext;
> +};
> +
>  struct cxl_driver {
>  	const char *name;
>  	int (*probe)(struct device *dev);
>  	void (*remove)(struct device *dev);
> +	int (*notify)(struct device *dev, struct cxl_drv_nd *nd);

First, this feels an overly DCD specific mechanism to inflict on the core
generic 'struct cxl_driver'. Most 'struct cxl_driver' instances do not
need any 'notify' callback and 'struct cxl_drv_nd' makes this even less
relevant to the core 'struct cxl_driver' definition.

Second, it leads to 2 anonymous ->notify() callbacks wht too deep of a
stack. It feels as if the resulting code is being actively evasive.

Given that the event handling code already knows how to lookup a 'struct
cxl_region', as Alison demonstrated in her DPA->HPA series, it should be
straightforward to lookup a 'struct cxl_dax_region' without a notifying
the cxl_mem driver.

So my expectation is just enough DCD event parsing to determine when the
payload applies to given cxl_dax_region. Then define a:

struct cxl_dax_region_driver {
        struct cxl_driver driver;
        void (*notify)(struct cxl_dax_region *cxlr_dax, ...);
};

..to send the payload over for further processing. If a cxl_dax_region
device instance cannot be found, just drop the event record.