Received-SPF: pass (google.com: domain of linux-kernel+bounces-166924-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 2 May 2024 14:12:26 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Ira Weiny <ira.weiny@intel.com>, Jonathan Cameron
	<Jonathan.Cameron@huawei.com>
CC: Dave Jiang <dave.jiang@intel.com>, Fan Ni <fan.ni@samsung.com>, "Navneet
 Singh" <navneet.singh@intel.com>, 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>,
	<linux-btrfs@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 16/26] cxl/extent: Realize extent devices
Message-ID: <663401ba2e237_1384629438@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240324-dcd-type2-upstream-v1-16-b7b00d623625@intel.com>
 <20240404173241.00003a6f@Huawei.com>
 <6630641e5238e_e1f5829431@iweiny-mobl.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6630641e5238e_e1f5829431@iweiny-mobl.notmuch>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?LC254O9NN5Noc/1aDi1BMpSvn899fgwTEuH34VtgHxwCd6Oywl4Dn6TW5Xka?=
 =?us-ascii?Q?BmL1CO+K0iVRdBeoS/p02bhuCNxaqOPPXddcQf0RnTJX/dBmXlOX0r6GY1pO?=
 =?us-ascii?Q?SF3A/aVewrjgj/wVgfznDzGhnz5Ql/Qap40Lz3IceP4tMYJHCPTRy7pIbQV4?=
 =?us-ascii?Q?EG/bBASCzwREkpuNddH5qi/rI9EpjdT8aQORu53evrbfr63W4payQ1MQDTUA?=
 =?us-ascii?Q?1E7DWGhhuTocQIePRmFA8inBKqbbeGg5LEdfHl/fb8CNH4y2A5lf/u1oCUZ2?=
 =?us-ascii?Q?TgupYbT0dCv2QmG24raE2UjgW7gPcn8Gr4WIhfpAOji0pjSocAP95GdTEK/5?=
 =?us-ascii?Q?omGB/yeMahkNkC0g5q5QepujoqJeegCg8vulJ+FTcLAqDpZjVeTN9zBboPsa?=
 =?us-ascii?Q?MGPJs2OCV/tBJg3z6yrme1DKleDSwG+eFJ7VK0ym366ufUqVPFwB5eoaUB5n?=
 =?us-ascii?Q?AsBbmayJGsxrFVqKiL5FMgNza5+TothlIC28/l4e+rjDBMe40PpSWkgV8vXm?=
 =?us-ascii?Q?Fx+z1Q3F7lWRQ/bqzvGv0Xylak26QaL1zLRs6lgOGjI3cfgVY11QMRf3U5mZ?=
 =?us-ascii?Q?5YVTwPd+W0AILR9JPWE52AHB26b8RkoZeiPtgcR3LrkkLGSgYOfAkpaFzjoE?=
 =?us-ascii?Q?0EPOy6P13qeSedi8ENi5BhCNVXeDAI+TjPSJXAmyRVUjSVSfBepHmlg2CR0b?=
 =?us-ascii?Q?+RW7V9YrrAo/gPtjkOL//qrjHTve+WiPaMaWn64zO8J62LkZ/GhszBmVpL6X?=
 =?us-ascii?Q?PO7yjKJ71GUH+NZUTO2fGoQsDXHN9xBpPkBRy1tuCdLFC2rTnjJPrEHnpCaw?=
 =?us-ascii?Q?og7gGomiMq2KbVhIOFdMHi+8EuBarI4tHXhEmumq0dSKFsHRWkrM7OdW+tji?=
 =?us-ascii?Q?5xHa240P1hdvvR+tdEpewpRh+sZzBcudB39qN2W7jT14il/ySu4hUbdUN/vs?=
 =?us-ascii?Q?SRF/OE28QmMA0ufEDKCdffkoctrgz19qyKEtxLfohC5/9YiGwFij09O0Y12h?=
 =?us-ascii?Q?q98ohbTy3zIZ6ozoxaZlTJIBQUwDruWpoZEmT4PbD998deYbXhgpXVmYCbTB?=
 =?us-ascii?Q?TrbIrBYMc+f2rJKFznX4fwWW58QC3gr90l36+fyUXRFUX67aWLA5CKIxze6l?=
 =?us-ascii?Q?MJXWAUXjU/ag05Ti+yUR1P2zNV6CvffdbrZUCJ1fY3aBeHzEqo0YgfMW23Xr?=
 =?us-ascii?Q?I9lSi1zNwBj/YwHl5yQvM+PS1Z9F6VpWCgVorpFGnmax5iQ1LQG9nRfsLvxQ?=
 =?us-ascii?Q?LleFSYV8oG49B76YOxcKy5IPYsNYXg8dwvOcVBfaTrhPWbQGiHTIV5e+d4yQ?=
 =?us-ascii?Q?mURN8/RC4AzTmVU/DVF5rgWufbFz4jWAm1w/tM91Vg6pymaEm9piZhwiR+yy?=
 =?us-ascii?Q?kgGOJh4qw+2mTioSChhLBBzZJ90DZWyDlqpTjkv+kavtXFt/zX/H/EfIqup5?=
 =?us-ascii?Q?9gR/C0soPsKIMqPxEn/eqnT/V1xTsDO4l+mFon0BkMRYbRZmF5fYwvCIIGz5?=
 =?us-ascii?Q?fwP+ukMOJenKO7Xt5mGvl5idrEAdDSS9V4o1ifYCiWpOi8z0HLRFkGbVO5o6?=
 =?us-ascii?Q?WU5O+YiFnQLRLk/75Vr2xoVBKC9rMFj10is/IdYOSUAFnsxZFNn+V2a7xTwP?=
 =?us-ascii?Q?Fg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f7e6e7c9-3071-4796-80d6-08dc6aec92b8
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 May 2024 21:12:28.7808
 (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: PFlNIz1rUFV8WPykuYJCHwffDn7gh/PVOe2S0WjLSHkqX/V30XRNTEbWDg5Xe05pgL/2uWaMPR6Uw4cbVz7VmInmI6z2Yv6ZhFW1egipAP0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB8022
X-OriginatorOrg: intel.com

Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Sun, 24 Mar 2024 16:18:19 -0700
> > ira.weiny@intel.com wrote:
> > 
> > > From: Navneet Singh <navneet.singh@intel.com>
> > > 
> > > Once all extents of an interleave set are present a region must
> > > surface an extent to the region.
> > > 
> > > Without interleaving; endpoint decoder and region extents have a 1:1
> > > relationship.  Future support for IW > 1 will maintain a N:1
> > > relationship between the device extents and region extents.
> > > 
> > > Create a region extent device for every device extent found.  Release of
> > > the extent device triggers a response to the underlying hardware extent.
> > > 
> > > There is no strong use case to support the addition of extents which
> > > overlap previously accepted extent ranges.  Reject such new extents
> > > until such time as a good use case emerges.
> > > 
> > > Expose the necessary details of region extents by creating the following
> > > sysfs entries.
> > > 
> > > 	/sys/bus/cxl/devices/dax_regionX/extentY
> > > 	/sys/bus/cxl/devices/dax_regionX/extentY/offset
> > > 	/sys/bus/cxl/devices/dax_regionX/extentY/length
> > > 	/sys/bus/cxl/devices/dax_regionX/extentY/label
> > 
> > Docs?
> 
> That is a good idea.
> 
> > The label in particular worries me a little as I'm not sure what
> > is in it.
> 
> I envisioned a pass through of the tag.
> 
> >
> > If it's the tag one possible format is a uuid (not a coincidence
> > that it is the same length) and interpreting that as characters isn't
> > going to get us far.  I wonder if we have to treat it as a binary attr
> > given we have no idea what it is.
> 
> In thinking about this more (and running some experiments): none of these are
> strictly necessary in this initial implementation.  No code currently uses
> them directly.
> 
> I questioned these in the past and I've done so again over the weekend.
> 
> I was about to rip them out entirely when I remembered Gregory Price's
> comments on Discord.  There he indicating a desire to very carefully place
> dax devices.  Without at least the offset and length above (and to a
> lesser extent the label) this can't be done.

Careful placement of dax-devices physically requires an entirely new
allocation ABI. There is the mapping_store() interface that was added
for a specific kexec / VMM fast restore use case, but that never
envisioned the sparse region case. So I do think it is worthwhile to
punt on that question to a later add-on feature.
 
[..]
> I don't think this is exactly what the user is going to expect.  This can
> be resolved by by looking at the dax device mappings though.[0]  So I'm going
> to leave this for now.  But I expect some additional porcelain is going to
> be required to fully meet Gregory's requirements.

Not sure what the exact requirement is, but if it's the typical, "I want
to allocate by tag", then I think there is another potential coarse
grained solution that probably covers most cases. Allow multiple
dax_regions per cxl_dcd_regions, where each dax_region manages an
exclusive set of tags.

The host negotiates a dax_region tag layout with the orchestrator and
can then trust that all of the extents that show up in a given dax_region
belong to a given tag or set of tags.

This is not something that needs to be considered in the initial
enabling, but is potentially a way to avoid bolting-on a new fine grained
allocation api after the fact.


> [0]
> 	/sys/bus/dax/devices/daxX.Y/mapping[0..N]/start
> 	/sys/bus/dax/devices/daxX.Y/mapping[0..N]/end
> 
> Back to the label field:  It is currently just the 'tag' of the individual
> extent (because no interleaving).  My vision for the interleave case would
> be for the kernel to assemble device extents into a region extent only if
> the tags match and export that.
> 
> Thinking on it more though we should leave label out for now.  This is the
> second time it has been questioned.

I don't understand the issue. That is a critical piece of information
and it is at the cxl device level

/sys/bus/cxl/devices/dax_regionX/extentY/label

..now I would just call that "tag" and UUID format it (to Jonathan's
point), but I see no rationale to hide what is most likely the most
useful information about an extent.