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

Ravis OpenSrc
2024-10-18T06:39:41Z

>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;