Received-SPF: pass (google.com: domain of linux-kernel+bounces-169301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sun, 5 May 2024 21:24:14 -0700
From: Ira Weiny <ira.weiny@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>, Ira Weiny
	<ira.weiny@intel.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>, Chris Mason <clm@fb.com>, Josef Bacik
	<josef@toxicpanda.com>, David Sterba <dsterba@suse.com>
Subject: Re: [PATCH 00/26] DCD: Add support for Dynamic Capacity Devices (DCD)
Message-ID: <66385b6eb5f54_25842129416@iweiny-mobl.notmuch>
References: <20240324-dcd-type2-upstream-v1-0-b7b00d623625@intel.com>
 <20240404184901.00002104@Huawei.com>
 <6632d503f3ae5_e1f58294df@iweiny-mobl.notmuch>
 <20240503102051.00004a99@Huawei.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240503102051.00004a99@Huawei.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8BbD99qZdjdu7r51wipocZcD3Ytm06gox3u6sd6KvmLekPg6pA3JWTVgc8G5?=
 =?us-ascii?Q?14bOqZLMvGdx2WTVmZzetC+V41OvIbRwqDD9fq6dRomVPQmyuAMKxJeTC6cH?=
 =?us-ascii?Q?Ko9DCwbZ3NYeqeLAxpPI7z7ULV3ZWpHvVNd+OuMlEh2DJbRvgIoNqFzNHNy8?=
 =?us-ascii?Q?9ay/GndO08pR6Q3RkangrI15rr/TKyH0y4Qv4Vfaj8pbLz2OiTu+YT91nr30?=
 =?us-ascii?Q?oLfaqq7P8uti9LEGoryYTwKm9OKLg85+8LqZ+xNUSt8c85XU7lywgGr6zfF7?=
 =?us-ascii?Q?harhW0sqmV6ILKklBNk49Kwn0PMjOaGel2hNw5SwrzJRVVLjJQ5CWjKA32E8?=
 =?us-ascii?Q?vz++buYPviaJMa0QVmf7DqbHVvaMcyhywkRjmLd1K/pttf2fHPo2Cybl5+3a?=
 =?us-ascii?Q?Qiww5ln4dgAAmrTVyeRT5Q+/SK0d3wPADypqoIqNo3W2HColFyuWNyI8JuvU?=
 =?us-ascii?Q?9VTbFTRFI93Wid1/o/lQP/cP/HuH8R5vRrDcijTcAXCM0j//6HUQU6EHnYPg?=
 =?us-ascii?Q?MfvUIQK4D6nza2k7iBpZLX0zLKS9WMYD622Dybnxy9UpzeEpkr1h3yT0IZZg?=
 =?us-ascii?Q?VYOg9HHVeygd0HOQc0o4kh8GCcGDa4CIKWIPR8wcSI/zDlPe0UAFY4rE0LX2?=
 =?us-ascii?Q?QcgCtCd4ipzs3IDQGm6r4aj4nfXYAQML747YbH2tAqOW5nni5Au2yjG5VD0M?=
 =?us-ascii?Q?7RBPAGqmsm77icN4WhHkc89V3wDpSJSktll1bXcyeIfcfsSYwWs0FHBnRpht?=
 =?us-ascii?Q?eh3p5gtsK3BygTDeJyBVsl05jn3lcl2IXzAp9FsaQQZhNiChA/GM0RdI1bRt?=
 =?us-ascii?Q?Iay+FHWrQXzKC785S6IslAMe52E/ICofy91J7QrNXQMT1RDcJDcVgev6uxFQ?=
 =?us-ascii?Q?olhPSt4ZkfOkIRXTMGiRCCabpQ0pXSnnrJ1flbsyfU9Bs3zSaMFUHWKWd+nh?=
 =?us-ascii?Q?2c8OySlf6MGLN+oZqXh/TzFBW+cTg/9TEns3hc1onOJiM4L0yxPuOCHtKNGF?=
 =?us-ascii?Q?OC1DLRsxm5UGqsgc/i0sxwUf12zLyOmXH2OsOAOS0fLK8jcF2lX5BLr/cjE4?=
 =?us-ascii?Q?EdZaYHUTwZAuyspf3EZULRtFzpav9RcmL0sb09cmTxD16iEwMV467SgJuFoH?=
 =?us-ascii?Q?Gcx1fraoyerctdEYFCwH6qiU9xNJIctwGcHxMIvVz6bkclbZZzA5XSF0DOse?=
 =?us-ascii?Q?+kWgsu4XFt563N/4HnV859IOnFTtdignvtxfC07lKulzmE2Y7B7hQ7ozN0b4?=
 =?us-ascii?Q?vH8bPixgfQuVxnyL7UIJtNCgV5tJgGcdFRQTfPPknpwVkvci2rFD0OHvFww2?=
 =?us-ascii?Q?dA5MwgR2Y9Az7gdXTc9sdRpH4cmAxJKru472cU52CNaWmMDHTFx5Ud1nvsft?=
 =?us-ascii?Q?nKlbbQwpFgtFxV9lvAM9HqoVBBIUUdmx5L8uD73LyFiLYKzFXeedLgqjVx9G?=
 =?us-ascii?Q?u237DjS1wec9YzgV75ofqNMlTmhlIp2puPXUwsxmdE1nwyAOyyQfw2wyib+e?=
 =?us-ascii?Q?n0Ca+0MF8V6cTRPVevOgLBIhtNmmnZojH9pRzkxVUd/NxRs8VTTa8P0D/9Xp?=
 =?us-ascii?Q?/X6WIbeNNlS8XXAB4Hl3Ur27T4Def0VumfmtdSnV?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8e793fce-039a-4ba5-83d0-08dc6d8464f6
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 May 2024 04:24:17.8277
 (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: 6BzEYhmKUdbb/fYMz5vOHsnjMwKLDVWqcc3ip/BdZoFKmz2OpDtne7apAiLncVJ0bmB535a/pZTt2Iajw8lhIA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6531
X-OriginatorOrg: intel.com

Jonathan Cameron wrote:
> On Wed, 1 May 2024 16:49:24 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > Jonathan Cameron wrote:
> > >   
> > > > 
> > > > Fan Ni's latest v5 of Qemu DCD was used for testing.[2]  
> > > Hi Ira, Navneet.  
> > > > 
> > > > Remaining work:
> > > > 
> > > > 	1) Integrate the QoS work from Dave Jiang
> > > > 	2) Interleave support  
> > > 
> > > 
> > > More flag.  This one I think is potentially important and don't
> > > see any handling in here.  
> > 
> > Nope I admit I missed the spec requirement.
> > 
> > > 
> > > Whilst an FM could in theory be careful to avoid sending a
> > > sparse set of extents, if the device is managing the memory range
> > > (which is possible all it supports) and the FM issues an Initiate Dynamic
> > > Capacity Add with Free (again may be all device supports) then we
> > > can't stop the device issuing a bunch of sparse extents.
> > > 
> > > Now it won't be broken as such without this, but every time we
> > > accept the first extent that will implicitly reject the rest.
> > > That will look very ugly to an FM which has to poke potentially many
> > > times to successfully allocate memory to a host.  
> > 
> > This helps me to see see why the more bit is useful.
> > 
> > > 
> > > I also don't think it will be that hard to support, but maybe I'm
> > > missing something?   
> > 
> > Just a bunch of code and refactoring busy work.  ;-)  It's not rocket
> > science but does fundamentally change the arch again.
> > 
> > > 
> > > My first thought is it's just a loop in cxl_handle_dcd_add_extent()
> > > over a list of extents passed in then slightly more complex response
> > > generation.  
> > 
> > Not exactly 'just a loop'.  No matter how I work this out there is the
> > possibility that some extents get surfaced and then the kernel tries to
> > remove them because it should not have.
> 
> Lets consider why it might need to back out.
> 1) Device sends an invalid set of extents - so maybe one in a later message
>    overlaps with an already allocated extent.   Device bug, handling can
>    be extremely inelegant - up to crashing the kernel.  Worst that happens
>    due to race is probably a poison storm / machine check fun?  Not our
>    responsibility to deal with something that broken (in my view!) Best effort
>    only.
> 
> 2) Host can't handle the extent for some reason and didn't know that until
>    later - can just reject the ones it can't handle. 

3) Something in the host fails like ENOMEM on a later extent surface which
   requires the host to back out of all of them.

3 should be rare and I'm working toward it.  But it is possible this will
happen.

If you have a 'prepare' notify it should avoid most of these because the
extents will be mostly formed.  But there are some error paths on the actual
surface code path.

> 
> > 
> > To be most safe the cxl core is going to have to make 2 round trips to the
> > cxl region layer for each extent.  The first determines if the extent is
> > valid and creates the extent as much as possible.  The second actually
> > surfaces the extents.  However, if the surface fails then you might not
> > get the extents back.  So now we are in an invalid state.  :-/  WARN and
> > continue I guess?!??!
> 
> Yes. Orchestrator can decide how to handle - probably reboot server in as
> gentle a fashion as possible.
> 

Ok

> 
> > 
> > I think the safest way to handle this is add a new kernel notify event
> > called 'extent create' which stops short of surfacing the extent.  [I'm
> > not 100% sure how this is going to affect interleave.]
> > 
> > I think the safest logic for add is something like:
> > 
> > 	cxl_handle_dcd_add_event()
> > 		add_extent(squirl_list, extent);
> > 
> > 		if (more bit) /* wait for more */
> > 			return;
> > 
> > 		/* Create extents to hedge the bets against failure */
> > 		for_each(squirl_list)
> > 			if (notify 'extent create' != ok)
> > 				send_response(fail);
> > 				return;
> > 		
> > 		for_each(squirl_list)
> > 			if (notify 'surface' != ok)
> > 				/*
> > 				 * If the more bit was set, some extents
> > 				 * have been surfaced and now need to be
> > 				 * removed...
> > 				 *
> > 				 * Try to remove them and hope...
> > 				 */
> 
> If we failed to surface them all another option is just tell the device
> that.   Responds with the extents that successfully surfaced and reject
> all others (or all after the one that failed?)  So for the lower layers
> send the device a response that says "thanks but I only took these ones"
> and for the upper layers pretend "I was only offered these ones"
> 

But doesn't that basically break the more bit?  I'm willing to do that as it is
easier for the host.

> > 				WARN_ON('surface extents failed');
> > 				for_each(squirl_list)
> > 					notify 'remove without response'
> > 				send_response(fail);
> > 				return;
> > 
> > 		send_response(squirl_list, accept);
> > 
> > The logic for remove is not changed AFAICS because the device must allow
> > for memory to be released at any time so the host is free to release each
> > of the extents individually despite the 'more' bit???
> 
> Yes, but only after it accepted them - which needs to be done in one go.
> So you can't just send releases before that (the device will return an
> error and keep them in the pending list I think...)

:-(  OK so this more bit is really more...  no pun intended.  Because this
breaks the entire model I have if I have to treat these as a huge atomic unit.

Let me think on that a bit more.  Obviously it is just tagging an iterating the
extents to find those associated with a more bit on accept.  But it will take
some time to code up.

> 
> > 
> > > 
> > > I don't want this to block getting initial DCD support in but it
> > > will be a bit ugly if we quickly support the more flag and then end
> > > up with just one kernel that an FM has to be careful with...  
> > 
> > I'm not sure which is worse.  Given your use case above it seems like the
> > more bit may be more important for 'dumb' devices which want to add
> > extents in blocks before responding to the FM.  Thus complicating the FM.
> > 
> > It seems 'smarter' devices which could figure this out (not requiring the
> > more bit) are the ones which will be developed later.  So it seems the use
> > case time line is the opposite of what we need right now.
> 
> Once we hit shareable capacity (which the smarter devices will use) then
> this become the dominant approach to non contiguous allocations because
> you can't add extents with a given tag in multiple goes.

Why not?  Sharing is going to require some synchronization with the
orchestrator and can't the user app just report it did not get all it's memory
and wait for more?  With the same tag?

> 
> So I'd expect the more flag to be more common not less over time.
> > 
> > For that reason I'm inclined to try and get this in.
> > 
> 
> Great - but I'd not worry too much about bad effects if you get invalid
> lists from the device.  If the only option is shout and panic, then fine
> though I'd imagine we can do slightly better than that, so maybe warn
> extensively and don't let the region be used.

It is not just about invalid lists.  It is that setting up the extent devices
may fail and waiting for the devices to be set up means that they are user
visible.  So that is the chicken and the egg...

This is unlikely and perhaps the partials should just be surfaced and accept
whatever works.  Then let it all tear down later if it does not all go.

But I was trying to honor the accept 'all or nothing' as that is what has been
stated as the requirement of the more bit.

But it seems that it does not __have__ to be atomic.  Or at least the partials
can be cleaned up and all tried again.

Ira

> 
> Jonathan
> 
> > Ira
> > 
>