Messages in this thread | | | Date | Tue, 5 Sep 2023 16:49:25 -0700 | From | Ira Weiny <> | Subject | Re: [PATCH RFC v2 10/18] cxl/mem: Handle DCD add and release capacity events. |
| |
Jonathan Cameron wrote: > On Mon, 28 Aug 2023 22:21:01 -0700 > Ira Weiny <ira.weiny@intel.com> 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@intel.com> > > Co-developed-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > 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; > > +} > > + > >
| |