Re: [RFC PATCH v2 3/4] cxl: Abort background operation in case of timeout

[Ravis OpenSrc <Ravis.OpenSrc@micron.com>]

>On Thu, 17 Oct 2024 09:06:00 +0530
>Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>> 
>> On Wed, 16 Oct 2024 05:00:00 +0000
>> Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:
>> 
>>> Adding support for aborting timed out background operations
>>>
>>> CXL r3.1 8.2.9.1.5 Request Abort Background Operation.
>>>
>>> If the status of a mailbox command is identified as timedout, an abort
>>> background operation request is sent to the device.
>>>
>>> Link:
>>>
>>https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
>>> .kernel.org%2Flinux-cxl%2F66035c2e8ba17_770232948b%40dwillia2-
>>xfh.jf.i
>>>
>>ntel.com.notmuch%2F&data=05%7C02%7Cajayjoshi%40micron.com%7C0d
>>d742041c
>>>
>>ba47ec0dbd08dceec16a02%7Cf38a5ecd28134862b11bac1d563c806f%7C0
>>%7C0%7C63
>>>
>>8647761722049771%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
>>MDAiLCJQIjoiV
>>>
>>2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=1vVK
>>PaqWSp6qT
>>> FmwsYF1z%2F7%2FdfEeVFD9cA0g1VVo00c%3D&reserved=0
>>>
>>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>>> Signed-off-by: Ajay Joshi <ajay.opensrc@micron.com>
>>> Signed-off-by: Ravi Shankar <ravis.opensrc@micron.com>
>>
>I was kind of expecting that we'd do this more on an ondemand basis. 

By saying on demand, do you envision a different implementation for
abort like a sysfs interface or module param?

> So if nothing is going on, the command can have ages.

I did not quite get the comment about the ages.

>If the kernel wants to access the device and it has had a reasonable amount of
>time we abort.
>I'm not against this simpler approach though if it turns out to be good enough
>for likely use cases.
> 
>My worry is error paths crossing with a slow command where we want to find
>out what went wrong as quick as possible Is 5 seconds ok for that?  Maybe
> not.
Do you feel we need to check for "aborted" status along with background percentage,
to avoid waiting for 5 seconds for cases when the command has already aborted.

Would something like below make sense, if not let us know your thoughts.
+static bool cxl_mbox_background_aborted(struct cxl_dev_state *cxlds)
+{
+       u64 reg;
+       u16 ret_code;
+
+       reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
+       ret_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK, reg);
+       if (ret_code == CXL_MBOX_CMD_RC_ABORT)
+               return true;
+
+       return false;
+}
+
 static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
 {
        u64 reg;
@@ -316,11 +329,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
                timeout = mbox_cmd->poll_interval_ms;
                for (i = 0; i < mbox_cmd->poll_count; i++) {
                        if (rcuwait_wait_event_timeout(&mds->mbox_wait,
-                                      cxl_mbox_background_complete(cxlds),
+                                      (cxl_mbox_background_complete(cxlds) |
+                                          cxl_mbox_background_aborted(cxlds)),
                                       TASK_UNINTERRUPTIBLE,
                                       msecs_to_jiffies(timeout)) > 0)
                                break;
                }
+               if (!cxl_mbox_background_aborted(cxlds)) {
+                       dev_err(dev, "aborted waiting for background (%d ms) by device\n",
+                               timeout * mbox_cmd->poll_count);
+                       return -ECANCELED;
+               }
> 
>Jonathan
>> 
>>> ---
>>>  drivers/cxl/cxlmem.h |  1 +
>>>  drivers/cxl/pci.c    | 11 +++++++++++
>>>  2 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h index
>>> d8c0894797ac..808fb8712145 100644
>>> --- a/drivers/cxl/cxlmem.h
>>> +++ b/drivers/cxl/cxlmem.h
>>> @@ -516,6 +516,7 @@ to_cxl_memdev_state(struct cxl_dev_state *cxlds)
>>> enum cxl_opcode {
>>>          CXL_MBOX_OP_INVALID             = 0x0000,
>>>          CXL_MBOX_OP_RAW                 = CXL_MBOX_OP_INVALID,
>>> +       CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION    = 0x0005,
>>>          CXL_MBOX_OP_GET_EVENT_RECORD    = 0x0100,
>>>          CXL_MBOX_OP_CLEAR_EVENT_RECORD  = 0x0101,
>>>          CXL_MBOX_OP_GET_EVT_INT_POLICY  = 0x0102, diff --git
>>> a/drivers/cxl/pci.c b/drivers/cxl/pci.c index
>>> d5d6142f6aa3..95c1f329bca2 100644
>>> --- a/drivers/cxl/pci.c
>>> +++ b/drivers/cxl/pci.c
>>> @@ -394,6 +394,17 @@ static int cxl_pci_mbox_send(struct cxl_mailbox
>>> *cxl_mbox,
>>>
>>>          mutex_lock_io(&cxl_mbox->mbox_mutex);
>>>          rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
>>> +       if (rc == -ETIMEDOUT &&
>>> +               cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
>>
>>align cmd just after ( - that is c is under the r of rc After fixing the tabs >ting.
>>
>>> +               struct cxl_mbox_cmd abort_cmd = {
>>> +                       .opcode =
>>CXL_MBOX_OP_REQ_ABRT_BACKGROUND_OPERATION
>>> +               };
>>> +
>>> +               rc = __cxl_pci_mbox_send_cmd(cxl_mbox, &abort_cmd);
>>> +               if (!rc)
>>> +                       rc = -ECANCELED;
>>> +       }
>>> +
>>>          mutex_unlock(&cxl_mbox->mbox_mutex);
>>>
>>>          return rc;