Re: [PATCH RFC v2 10/18] cxl/mem: Handle DCD add and release capacity events.

[Ira Weiny]

Jonathan Cameron wrote:
> On Mon, 28 Aug 2023 22:21:01 -0700
> Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
> 
> > A Dynamic Capacity Device (DCD) utilizes events to signal the host about
> > the changes to the allocation of Dynamic Capacity (DC) extents. The
> > device communicates the state of DC extents through an extent list that
> > describes the starting DPA, length, and meta data of the blocks the host
> > can access.
> > 
> > Process the dynamic capacity add and release events.  The addition or
> > removal of extents can occur at any time.  Adding asynchronous memory is
> > straight forward.  Also remember the host is under no obligation to
> > respond to a release event until it is done with the memory.  Introduce
> > extent kref's to handle the delay of extent release.
> > 
> > In the case of a force removal, access to the memory will fail and may
> > cause a crash.  However, the extent tracking object is preserved for the
> > region to safely tear down as long as the memory is not accessed.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> > Co-developed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Minor stuff inline.
> 
> 
> > +static int cxl_prepare_ext_list(struct cxl_mbox_dc_response **res,
> > +				int *n, struct range *extent)
> > +{
> > +	struct cxl_mbox_dc_response *dc_res;
> > +	unsigned int size;
> > +
> > +	if (!extent)
> > +		size = struct_size(dc_res, extent_list, 0);
> 
> This is confusing as if you did have *n > 0 I'd kind of expect
> this to just not extend the list rather than shortening it.
> Now I guess that never happens, but locally it looks odd.
> 
> Maybe just handle that case in a separate function as it doesn't
> share much code with the case where there is an extent and I would
> assume we always know at the caller which one we want.

Yea I forget why I left this alone.  I did not care for it during internal
review and I think I got so busy with the other code that this just got
left behind.

Frankly this is a candidate for the __free() magic as well.  But in a
helper function which handles sending the response...

This needs some refactoring for sure...  :-/

> 
> 
> > +	else
> > +		size = struct_size(dc_res, extent_list, *n + 1);
> 
> Might be clearer with a local variable for the number of extents.
> 
> extents_count = *n;
> 
> if (extent)
> 	extents_count++;
> 
> size = struct_size(dc_res, extent_list, extents_count);
> 
> Though I'm not sure that really helps.  Maybe this will just need
> to be a little confusing :)

Actually no.  IIRC the original idea was to have a running response data
structure realloc'ed as events were processed from the log and then to
send out a final large response...  But in my refactoring I did not do
that.  The refactoring processes each event (extent) before going on to
the next event.  I suppose this may be an issue later if large numbers
of extents are added to the logs rapidly and the processing is not fast
enough and the logs overflow.

But I don't think the complexity is warranted at this time.  Especially
because under that condition the size of the response needs to be
contained within mds->payload_size.  So there is quite a bit more
complexity there that I don't think was accounted for initially.

I think cxl_send_dc_cap_response() should handle this allocation (using
__free() magic) and then do the send all in 1 function.

I'll refactor and see how it goes.

> 
> > +
> > +	dc_res = krealloc(*res, size, GFP_KERNEL);
> > +	if (!dc_res)
> > +		return -ENOMEM;
> > +
> > +	if (extent) {
> > +		dc_res->extent_list[*n].dpa_start = cpu_to_le64(extent->start);
> > +		memset(dc_res->extent_list[*n].reserved, 0, 8);
> > +		dc_res->extent_list[*n].length = cpu_to_le64(range_len(extent));
> > +		(*n)++;
> > +	}
> > +
> > +	*res = dc_res;
> > +	return 0;
> > +}
> 
> > +
> > +/* Returns 0 if the event was handled successfully. */
> > +static int cxl_handle_dcd_event_records(struct cxl_memdev_state *mds,
> > +					struct cxl_event_record_raw *rec)
> > +{
> > +	struct dcd_event_dyn_cap *record = (struct dcd_event_dyn_cap *)rec;
> > +	uuid_t *id = &rec->hdr.id;
> > +	int rc;
> > +
> > +	if (!uuid_equal(id, &dc_event_uuid))
> > +		return -EINVAL;
> > +
> > +	switch (record->data.event_type) {
> > +	case DCD_ADD_CAPACITY:
> > +		rc = cxl_handle_dcd_add_event(mds, &record->data.extent);
> > +		break;
> 
> I guess it might not be consistent with local style...
> 		return cxl_handle_dcd_add_event()  etc

Sure.  That is cleaner.  Done.

Ira

> 
> > +	case DCD_RELEASE_CAPACITY:
> > +        case DCD_FORCED_CAPACITY_RELEASE:
> > +		rc = cxl_handle_dcd_release_event(mds, &record->data.extent);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return rc;
> > +}
> > +
> 
>