Re: [PATCH v5 09/13] hw/cxl/events: Add qmp interfaces to add/release dynamic capacity extents

[fan]

On Thu, Mar 07, 2024 at 12:45:55PM +0000, Jonathan Cameron wrote:
> ...
> 
> > > > +    list = records;
> > > > +    extents = g_new0(CXLDCExtentRaw, num_extents);
> > > > +    while (list) {
> > > > +        CXLDCExtent *ent;
> > > > +        bool skip_extent = false;
> > > > +
> > > > +        offset = list->value->offset;
> > > > +        len = list->value->len;
> > > > +
> > > > +        extents[i].start_dpa = offset + dcd->dc.regions[rid].base;
> > > > +        extents[i].len = len;
> > > > +        memset(extents[i].tag, 0, 0x10);
> > > > +        extents[i].shared_seq = 0;
> > > > +
> > > > +        if (type == DC_EVENT_RELEASE_CAPACITY ||
> > > > +            type == DC_EVENT_FORCED_RELEASE_CAPACITY) {
> > > > +            /*
> > > > +             *  if the extent is still pending to be added to the 
> > > > host,  
> > > 
> > > Odd spacing.
> > >   
> > > > +             * 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.
> > > > +             */
> > > > +            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);
> > > > +                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 */
> > > > +                skip_extent = true;
> > > > +            }  
> > > I think we need to reject case of some extents skipped and others not.
> > > That's not supported yet so we need to complain if we get it at least. 
> > > Maybe we need
> > > to do two passes so we know this has happened early (or perhaps this is a 
> > > later
> > > patch in which case a todo here would help).  
> > 
> > Skip here does not mean the extent is invalid, it just means the extent
> > is still pending to add, so remove them from pending list would be
> > enough to reject the extent, no need to release further. That is based
> > on your feedback on v4.
> 
> Ah. I'd missunderstood.

Hi Jonathan,

I think we should not allow to release extents that are still pending to
add. 
If we allow it, there is a case that will not work.
Let's see the following case (time order):
1. Send request to add extent A to host; (A --> pending list)
2. Send request to release A from the host; (Delete A from pending list,
hoping the following add response for A will fail as there is not a matched
extent in the pending list).
3. Host send response to the device for the add request, however, for
some reason, it does not accept any of it, so updated list is empty,
spec allows it. Based on the spec, we need to drop the extent at the
head of the event log. Now we have problem. Since extent A is already
dropped from the list, we either cannot drop as the list is empty, which
is not the worst. If we have more extents in the list, we may drop the
one following A, which is for another request. If this happens, all the
following extents will be acked incorrectly as the order has been
shifted.
 
Does the above reasoning make sense to you?

Fan

> 
> > 
> > The loop here is only to collect the extents to sent to the event log. 
> > But as you said, we need one pass before updating pending list.
> > Actually if we do not allow the above case where extents to release is
> > still in the pending to add list, we can just return here with error, no
> > extra dry run needed. 
> > 
> > What do you think?
> 
> I think we need a way to back out extents from the pending to add list
> so we can create the race where they are offered to the OS and it takes
> forever to accept and by the time it does we've removed them.
> 
> > 
> > >   
> > > > +        
> > > > +
> > > > +        /* No duplicate or overlapped extents are allowed */
> > > > +        if (test_any_bits_set(blk_bitmap, offset / block_size,
> > > > +                              len / block_size)) {
> > > > +            error_setg(errp, "duplicate or overlapped extents are 
> > > > detected");
> > > > +            return;
> > > > +        }
> > > > +        bitmap_set(blk_bitmap, offset / block_size, len / block_size);
> > > > +
> > > > +        list = list->next;
> > > > +        if (!skip_extent) {
> > > > +            i++;  
> > > Problem is if we skip one in the middle the records will be wrong below.  
> > 
> > Why? Only extents passed the check will be stored in variable extents and
> > processed further and i be updated. 
> > For skipped ones, since i is not updated, they will be
> > overwritten by following valid ones.
> Ah. I'd missed the fact you store into the extent without a check on validity
> but only move the index on if they were valid. Then rely on not passing a 
> trailing
> entry at the end.
> If would be more readable I think if local variables were used for the 
> parameters
> until we've decided not to skip and the this ended with
> 
>         if (!skip_extent) {
>             extents[i] = (DCXLDCExtentRaw) {
>                 .start_dpa = ...
>               ...
>             };
>             i++
>         }
> We have local len already so probably just need
> uint64_t start_dpa = offset + dcd->dc.regions[rid].base;
> 
> Also maybe skip_extent_evlog or something like that to explain we are only
> skipping that part. 
> Helps people like me who read it completely wrong!
> 
> Jonathan
> 
>  
>