Re: [PATCH v5 12/13] hw/mem/cxl_type3: Allow to release partial extent and extent superset in QMP interface

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon,  4 Mar 2024 11:34:07 -0800
nifan.cxl@gmail.com wrote:

> From: Fan Ni <fan.ni@samsung.com>
> 
> Before the change, the QMP interface used for add/release DC extents
> only allows to release extents that exist in either pending-to-add list
> or accepted list in the device, which means the DPA range of the extent must
> match exactly that of an extent in either list. Otherwise, the release
> request will be ignored.
> 
> With the change, we relax the constraints. As long as the DPA range of the
> extent to release is covered by extents in one of the two lists
> mentioned above, we allow the release.
> 
> Signed-off-by: Fan Ni <fan.ni@samsung.com>
Run out of time today,  so just took a very quick look at this.

Seemed fine but similar comments on exit conditions and retry gotos as
earlier patches.

> +/*
> + * Remove all extents whose DPA range has overlaps with  the DPA range
> + * [dpa, dpa + len) from the list, and delete the overlapped portion.
> + * Note:
> + * 1. If the removed extents is fully within the DPA range, delete the extent;
> + * 2. Otherwise, keep the portion that does not overlap, insert new extents to
> + * the list if needed for the un-coverlapped part.
> + */
> +static void cxl_delist_extent_by_dpa_range(CXLDCExtentList *list,
> +                                           uint64_t dpa, uint64_t len)
> +{
> +    CXLDCExtent *ent;
>  
> -    return NULL;
> +process_leftover:

As before can we turn this into a while loop so the exit conditions are 
more obvious?  Based on len I think.


> +    QTAILQ_FOREACH(ent, list, node) {
> +        if (ent->start_dpa <= dpa && dpa < ent->start_dpa + ent->len) {
> +            uint64_t ent_start_dpa = ent->start_dpa;
> +            uint64_t ent_len = ent->len;
> +            uint64_t len1 = dpa - ent_start_dpa;
> +
> +            cxl_remove_extent_from_extent_list(list, ent);
> +            if (len1) {
> +                cxl_insert_extent_to_extent_list(list, ent_start_dpa,
> +                                                 len1, NULL, 0);
> +            }
> +
> +            if (dpa + len <= ent_start_dpa + ent_len) {
> +                uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> +                if (len2) {
> +                    cxl_insert_extent_to_extent_list(list, dpa + len,
> +                                                     len2, NULL, 0);
> +                }
> +            } else {
> +                len = dpa + len - ent_start_dpa - ent_len;
> +                dpa = ent_start_dpa + ent_len;
> +                goto process_leftover;
> +            }
> +        }
> +    }
>  }
>  
>  /*
> @@ -1915,8 +1966,8 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
>      list = records;
>      extents = g_new0(CXLDCExtentRaw, num_extents);
>      while (list) {
> -        CXLDCExtent *ent;
>          bool skip_extent = false;
> +        CXLDCExtentList *extent_list;
>  
>          offset = list->value->offset;
>          len = list->value->len;
> @@ -1933,15 +1984,32 @@ static void qmp_cxl_process_dynamic_capacity(const char *path, CxlEventLog log,
>               * remove it from the pending extent list, so later when the add
>               * response for the extent arrives, the device can reject the
>               * extent as it is not in the pending list.
> +             * Now, we can handle the case where the extent covers the DPA

No need for Now. Anyone reading it is look at the cod here.

> +             * range of multiple extents in the pending_to_add list.
> +             * TODO: we do not allow the extent covers range of extents in
> +             * pending_to_add list and accepted list at the same time for now.
>               */
> -            ent = cxl_dc_extent_exists(&dcd->dc.extents_pending_to_add,
> -                        &extents[i]);
> -            if (ent) {
> -                QTAILQ_REMOVE(&dcd->dc.extents_pending_to_add, ent, node);
> -                g_free(ent);
> +            extent_list = &dcd->dc.extents_pending_to_add;
> +            if (cxl_test_dpa_range_covered_by_extents(extent_list,
> +                                                      extents[i].start_dpa,
> +                                                      extents[i].len)) {
> +                cxl_delist_extent_by_dpa_range(extent_list,
> +                                               extents[i].start_dpa,
> +                                               extents[i].len);
> +            } else if (!ct3_test_region_block_backed(dcd, extents[i].start_dpa,
> +                                                     extents[i].len)) {
> +                /*
> +                 * If the DPA range of the extent is not covered by extents
> +                 * in the accepted list, skip
> +                 */
>                  skip_extent = true;
> -            } else if (!cxl_dc_extent_exists(&dcd->dc.extents, &extents[i])) {
> -                /* If the exact extent is not in the accepted list, skip */
> +            }
> +        } else if (type == DC_EVENT_ADD_CAPACITY) {
> +            extent_list = &dcd->dc.extents;
> +            /* If the extent is ready pending to add, skip */
> +            if (cxl_test_dpa_range_covered_by_extents(extent_list,
> +                                                      extents[i].start_dpa,
> +                                                      extents[i].len)) {
>                  skip_extent = true;
>              }
>          }