On Mon, 26 Feb 2024 17:01:22 -0800
fan <nifan....@gmail.com> wrote:
> On Mon, Feb 26, 2024 at 06:04:17PM +0000, Jonathan Cameron wrote:
> > On Wed, 21 Feb 2024 10:16:01 -0800
> > nifan....@gmail.com wrote:
> >
> > > From: Fan Ni <fan...@samsung.com>
> > >
> > > Per CXL spec 3.1, two mailbox commands are implemented:
> > > Add Dynamic Capacity Response (Opcode 4802h) 8.2.9.9.9.3, and
> > > Release Dynamic Capacity (Opcode 4803h) 8.2.9.9.9.4.
> > >
> > > Signed-off-by: Fan Ni <fan...@samsung.com>
> >
> > Hi Fan,
> >
> > Comments on this are all about corner cases. If we can I think we need
> > to cover a few more. Linux won't hit them (I think) so it will be
> > a bit of a pain to test but maybe raw commands enabled and some
> > userspace code will let us exercise the corner cases?
> >
> > Jonathan
> >
> >
> >
> > > +
> > > +/*
> > > + * CXL r3.1 section 8.2.9.9.9.4: Release Dynamic Capacity (opcode 4803h)
> > > + */
> > > +static CXLRetCode cmd_dcd_release_dyn_cap(const struct cxl_cmd *cmd,
> > > + uint8_t *payload_in,
> > > + size_t len_in,
> > > + uint8_t *payload_out,
> > > + size_t *len_out,
> > > + CXLCCI *cci)
> > > +{
> > > + CXLUpdateDCExtentListInPl *in = (void *)payload_in;
> > > + CXLType3Dev *ct3d = CXL_TYPE3(cci->d);
> > > + CXLDCExtentList *extent_list = &ct3d->dc.extents;
> > > + CXLDCExtent *ent;
> > > + uint32_t i;
> > > + uint64_t dpa, len;
> > > + CXLRetCode ret;
> > > +
> > > + if (in->num_entries_updated == 0) {
> > > + return CXL_MBOX_INVALID_INPUT;
> > > + }
> > > +
> > > + ret = cxl_detect_malformed_extent_list(ct3d, in);
> > > + if (ret != CXL_MBOX_SUCCESS) {
> > > + return ret;
> > > + }
> > > +
> > > + for (i = 0; i < in->num_entries_updated; i++) {
> > > + bool found = false;
> > > +
> > > + dpa = in->updated_entries[i].start_dpa;
> > > + len = in->updated_entries[i].len;
> > > +
> > > + QTAILQ_FOREACH(ent, extent_list, node) {
> > > + if (ent->start_dpa <= dpa &&
> > > + dpa + len <= ent->start_dpa + ent->len) {
> > > + /*
> > > + * If an incoming extent covers a portion of an extent
> > > + * in the device extent list, remove only the overlapping
> > > + * portion, meaning
> > > + * 1. the portions that are not covered by the incoming
> > > + * extent at both end of the original extent will
> > > become
> > > + * new extents and inserted to the extent list; and
> > > + * 2. the original extent is removed from the extent
> > > list;
> > > + * 3. dc extent count is updated accordingly.
> > > + */
> > > + uint64_t ent_start_dpa = ent->start_dpa;
> > > + uint64_t ent_len = ent->len;
> > > + uint64_t len1 = dpa - ent_start_dpa;
> > > + uint64_t len2 = ent_start_dpa + ent_len - dpa - len;
> > > +
> > > + found = true;
> > > + cxl_remove_extent_from_extent_list(extent_list, ent);
> > > + ct3d->dc.total_extent_count -= 1;
> > > +
> > > + if (len1) {
> > > + cxl_insert_extent_to_extent_list(extent_list,
> > > + ent_start_dpa, len1,
> > > + NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> > > + }
> > > + if (len2) {
> > > + cxl_insert_extent_to_extent_list(extent_list, dpa +
> > > len,
> > > + len2, NULL, 0);
> > > + ct3d->dc.total_extent_count += 1;
> >
> > There is a non zero chance that we'll overflow however many extents we claim
> > to support. So we need to check that and fail the remove if it happens.
> > Could ignore this for now though as that value is (I think!) conservative
> > to allow for complex extent list tracking implementations. Succeeding
> > when a naive solution would fail due to running out of extents that it can
> > manage is not (I think) a bug.
>
> Yeah. spec r3.1 mentioned about the overflow issue that adding/releasing
> extent requests can raise. We should fail the operation if running out of
> extents and report resource exhausted.
>
> >
> > > + }
> > > + break;
> > > + /*Currently we reject the attempt to remove a superset*/
> > >
> >
> > Space after /* and before */
> >
> > I think we need to fix this. Linux isn't going to do it any time soon, but
> > I think it's allowed to allocate two extents next to each other then free
> > them
> > in one go. Isn't this case easy to do or are there awkward corners?
> > If it's sufficiently nasty (maybe because only part of extent provided
> > exists?)
> > then maybe we can leave it for now.
> >
> > I worry about something like
> >
> > | EXTENT TO FREE |
> > | Exists | gap | Exists |
> > Where we have to check for gap before removing anything?
> > Does the spec address this? Not that I can find.
> > I think the implication is we have to do a validation pass, then a free
> > pass after we know whole of requested extent is valid.
> > Nasty to test if nothing else :( Would look much like your check
> > on malformed extent lists.
> >
>
> I cannot find anything specific to this in the specification either.
> Since we have already detected the case where the extent range across
> multiple regions, the only case we need to capture here is one/multiple
> portions of an extents getting released and causing extent overflow.
> I think we can handle it after we introduce the bitmaps (PATCH 10) which
> indicates DPA ranges mapped by valid extents in the device.
>
> With that, The release workflow would be
>
> 1) detecting malformed extent lists; if passed
> 2) do cxl_detect_extent_overflow {
> delta = 0;
> make a copy of the bitmap as bitmap_copy;
> for each extent in the updated_extent_list; do
> if (extent range not fully set in the bitmap_copy)
> return error;
> else {
> if gap at the front based on the bitmap_copy:
> delta += 1;
> if gap at the end based on the bitmap_copy:
> delta += 1;
> delta -= 1;
> // NOTE: current_extent_count will not be updated in the
> // loop since delta will track the whole loop
> if (delta + current_extent_count > max_extent_count)
> return resource exhausted;
> update bitmap_copy to clear the range covered by the extent
> under consideration;
> }
> done
>
> }; if pass
> 3. do real release: in the pass, we will not need to detect extent
> errors;
>
> Does the above solution sound reasonable? If so, do we want to go this
> way? do we need to introduce the bitmap earlier in the series?
Yes, something along these lines should work nicely.
Jonathan
>
> Thanks,
> Fan
>
>
>
> >
> > > + } else if ((dpa < ent->start_dpa + ent->len &&
> > > + dpa + len > ent->start_dpa + ent->len) ||
> > > + (dpa < ent->start_dpa && dpa + len >
> > > ent->start_dpa)) {
> > > + return CXL_MBOX_INVALID_EXTENT_LIST;
> > > + }
> > > + }
> > > +
> > > + if (!found) {
> > > + /* Try to remove a non-existing extent */
> > > + return CXL_MBOX_INVALID_PA;
> > > + }
> > > + }
> > > +
> > > + return CXL_MBOX_SUCCESS;
> > > +}
> >
> >
