Re: [PATCH] cxl/port: Prevent out-of-order decoder allocation

[Zijun Hu <zijun_hu@icloud.com>]

On 2024/10/15 09:51, Dan Williams wrote:
> Zijun Hu wrote:
>> On 2024/10/15 08:06, Dan Williams wrote:
>>> With the recent change to allow out-of-order decoder de-commit it
>>> highlights a need to strengthen the in-order decoder commit guarantees.
>>> As it stands match_free_decoder() ensures that if 2 regions are racing
>>> decoder allocations the one that wins the race will get the lower id
>>> decoder, but that still leaves the race to *commit* the decoder.
>>>
>>> Rather than have this complicated case of "reserved in-order, but may
>>> still commit out-of-order", just arrange for the reservation order to
>>> match the commit-order. In other words, prevent subsequent allocations
>>> until the last reservation is committed.
>>>
>>> This precludes overlapping region creation events and requires the
>>> previous regionN to either move forward to the decoder commit stage or
>>> drop its reservation before regionN+1 can move forward. That is,
>>> provided that regionN and regionN+1 decode through the same switch port.
>>>
>>> As a side effect this allows match_free_decoder() to drop its dependency
>>> on needing write access to the device_find_child() @data parameter [1].
>>>
>>> Reported-by: Zijun Hu <zijun_hu@icloud.com>
>>> Closes: http://lore.kernel.org/20240905-const_dfc_prepare-v4-0-4180e1d5a244@quicinc.com [1]
>>> Cc: Davidlohr Bueso <dave@stgolabs.net>
>>> Cc: Vishal Verma <vishal.l.verma@intel.com>
>>> Cc: Alison Schofield <alison.schofield@intel.com>
>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>> ---
>>> This patch is incremental to "cxl: Initialization and shutdown fixes"
>>>
>>> [2]: http://lore.kernel.org/172862483180.2150669.5564474284074502692.stgit@dwillia2-xfh.jf.intel.com
>>>
>>>  drivers/cxl/core/region.c |   43 +++++++++++++++++++++++++++++++++----------
>>>  1 file changed, 33 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>>> index 3478d2058303..dff618c708dc 100644
>>> --- a/drivers/cxl/core/region.c
>>> +++ b/drivers/cxl/core/region.c
> [..]
>>
>> Above checking seems meaningless since there are only one CLDX with ID
>> port->commit_end + 1.
>>
>> logical of below my proposal maybe what you want.
>> https://lore.kernel.org/all/4239bfd4-d5fe-4ac8-a087-9e1584765e61@icloud.com/
> 
> No. Consider that port->commit_end + 1 may walk off the end of available
> decoders.

really ?  could you like to take a example ?

let me paste the solution of below link here as well

1) it is  simpler.

2) it does need to introduce a new API device_for_each_child_reverse_from()

3) it have the minimal iterating count.

https://lore.kernel.org/all/4239bfd4-d5fe-4ac8-a087-9e1584765e61@icloud.com/

--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -796,8 +796,9 @@ static size_t show_targetN(struct cxl_region *cxlr,
char *buf, int pos)

 static int match_free_decoder(struct device *dev, void *data)
 {
+       struct cxl_port *port = to_cxl_port(dev->parent);
        struct cxl_decoder *cxld;
-       int *id = data;
+       struct device **target_dev = data;

        if (!is_switch_decoder(dev))
                return 0;
@@ -805,15 +806,19 @@ static int match_free_decoder(struct device *dev,
void *data)
        cxld = to_cxl_decoder(dev);

        /* enforce ordered allocation */
-       if (cxld->id != *id)
-               return 0;
-
-       if (!cxld->region)
-               return 1;
-
-       (*id)++;
-
-       return 0;
+       if (cxld->id == port->commit_end + 1) {
+               if (!cxld->region) {
+                       *target_dev = dev;
+                       return 1;
+               } else {
+                       dev_dbg(dev->parent,
+                               "next decoder to commit is already
reserved\n",
+                               dev_name(dev));
+                       return -ENODEV;
+               }
+       } else {
+               return cxld->flags & CXL_DECODER_F_ENABLE ? 0 : -EBUSY;
+       }
 }

 static int match_auto_decoder(struct device *dev, void *data)
@@ -839,7 +844,7 @@ cxl_region_find_decoder(struct cxl_port *port,
                        struct cxl_endpoint_decoder *cxled,
                        struct cxl_region *cxlr)
 {
-       struct device *dev;
+       struct device *dev = NULL;
        int id = 0;

        if (port == cxled_to_port(cxled))
@@ -848,8 +853,8 @@ cxl_region_find_decoder(struct cxl_port *port,
        if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
                dev = device_find_child(&port->dev, &cxlr->params,
                                        match_auto_decoder);
-       else
-               dev = device_find_child(&port->dev, &id,
match_free_decoder);
+       else if (device_for_each_child(&port->dev, &dev,
match_free_decoder) > 0)
+               get_device(dev);
        if (!dev)
                return NULL;
        /*