Re: [PATCH v2 4/5] cxl: Make cxl_dpa_alloc() DPA partition number agnostic

[Dan Williams <dan.j.williams@intel.com>]

Ira Weiny wrote:
> Dan Williams wrote:
> > cxl_dpa_alloc() is a hard coded nest of assumptions around PMEM
> > allocations being distinct from RAM allocations in specific ways when in
> > practice the allocation rules are only relative to DPA partition index.
> > 
> > The rules for cxl_dpa_alloc() are:
> > 
> > - allocations can only come from 1 partition
> > 
> > - if allocating at partition-index-N, all free space in partitions less
> >   than partition-index-N must be skipped over
> 
> I think this is a bit deeper.  The partition index must also correspond to
> the DPA order.  The DCD code verifies the partition index's are in DPA
> order when reading them from the device.  Therefore, that code will add
> them to cxl_dpa_info in order.  But general device driver writers may miss
> this point.

We could save them from themselves with some paranoia in
cxl_dpa_setup(), but as Alejandro said accelerators are typically
single-static-RAM-partition devices. The risk is low that someone builds
a multi-partition accelerator *and* builds a driver that messes that up,
but I would not say no to a comment that notes that expectation.

> [snip]
> 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 3f8a54ca4624..591aeb26c9e1 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -223,6 +223,31 @@ void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_dpa_debug, "CXL");
> >  
> > +/* See request_skip() kernel-doc */
> > +static void release_skip(struct cxl_dev_state *cxlds,
> > +			 const resource_size_t skip_base,
> > +			 const resource_size_t skip_len)
> > +{
> > +	resource_size_t skip_start = skip_base, skip_rem = skip_len;
> > +
> > +	for (int i = 0; i < cxlds->nr_partitions; i++) {
> > +		const struct resource *part_res = &cxlds->part[i].res;
> > +		resource_size_t skip_end, skip_size;
> > +
> > +		if (skip_start < part_res->start || skip_start > part_res->end)
> > +			continue;
> > +
> > +		skip_end = min(part_res->end, skip_start + skip_rem - 1);
> > +		skip_size = skip_end - skip_start + 1;
> > +		__release_region(&cxlds->dpa_res, skip_start, skip_size);
> > +		skip_start += skip_size;
> > +		skip_rem -= skip_size;
> > +
> > +		if (!skip_rem)
> > +			break;
> > +	}
> > +}
> > +
> >  /*
> >   * Must be called in a context that synchronizes against this decoder's
> >   * port ->remove() callback (like an endpoint decoder sysfs attribute)
> > @@ -241,7 +266,7 @@ static void __cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> >  	skip_start = res->start - cxled->skip;
> >  	__release_region(&cxlds->dpa_res, res->start, resource_size(res));
> >  	if (cxled->skip)
> > -		__release_region(&cxlds->dpa_res, skip_start, cxled->skip);
> > +		release_skip(cxlds, skip_start, cxled->skip);
> >  	cxled->skip = 0;
> >  	cxled->dpa_res = NULL;
> >  	put_device(&cxled->cxld.dev);
> > @@ -268,6 +293,79 @@ static void devm_cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
> >  	__cxl_dpa_release(cxled);
> >  }
> >  
> > +/**
> > + * request_skip() - Track DPA 'skip' in @cxlds->dpa_res resource tree
> > + * @cxlds: CXL.mem device context that parents @cxled
> > + * @cxled: Endpoint decoder establishing new allocation that skips lower DPA
> > + * @skip_base: DPA < start of new DPA allocation (DPAnew)
> > + * @skip_len: @skip_base + @skip_len == DPAnew
> > + *
> > + * DPA 'skip' arises from out-of-sequence DPA allocation events relative
> > + * to free capacity across multiple partitions. It is a wasteful event
> > + * as usable DPA gets thrown away, but if a deployment has, for example,
> > + * a dual RAM+PMEM device, wants to use PMEM, and has unallocated RAM
> > + * DPA, the free RAM DPA must be sacrificed to start allocating PMEM.
> > + * See third "Implementation Note" in CXL 3.1 8.2.4.19.13 "Decoder
> > + * Protection" for more details.
> 
> I think this is a great comment here.

Appreciate that, never know how these things are going to translate.

> 
> > + *
> > + * A 'skip' always covers the last allocated DPA in a previous partition
> > + * to the start of the current partition to allocate.  Allocations never
> > + * start in the middle of a partition, and allocations are always
> > + * de-allocated in reverse order (see cxl_dpa_free(), or natural devm
> > + * unwind order from forced in-order allocation).
> > + *
> > + * If @cxlds->nr_partitions was guaranteed to be <= 2 then the 'skip'
> > + * would always be contained to a single partition. Given
> > + * @cxlds->nr_partitions may be > 2 it results in cases where the 'skip'
> > + * might span "tail capacity of partition[0], all of partition[1], ...,
> > + * all of partition[N-1]" to support allocating from partition[N]. That
> > + * in turn interacts with the partition 'struct resource' boundaries
> > + * within @cxlds->dpa_res whereby 'skip' requests need to be divided by
> > + * partition. I.e. this is a quirk of using a 'struct resource' tree to
> > + * detect range conflicts while also tracking partition boundaries in
> > + * @cxlds->dpa_res.
> 
> Another great comment but it does not actually cover the DCD case.  This
> is because in DCD the partitions might also have skips between them.

I think that "just works". The allocation will be bound by the
partition, and the skip is calculated from the "end of last allocation
in a previous partition". So, the distance between "end of last" and
"allocation start" will naturally include inter-partition holes, right?

> That said the update should come with DCD or if type 2 devices may have
> the same loosening of device partitions.
> 
> This is a good clean up though,
> 
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>

Appreciate the quick turnaround... I will endeavor to do the same with
the next DCD posting.