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

Jonathan Cameron
2024-10-18T16:14:20Z

On Fri, 18 Oct 2024 06:39:34 +0000
Ravis OpenSrc <Ravis.OpenSrc@micron.com> wrote:

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

No. Abort by the kernel when it wants to do something rather than
on a timeout. It might also make sense ultimately to have a userspace
abort path (if it issued the command) but that is a different isseu.

>
> > So if nothing is going on, the command can have ages.
>
> I did not quite get the comment about the ages.

Long time. If no one wants to access the device interfaces, we don't mind if
the command takes much longer than 5 seconds.

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

At the moment we don't have anything issuing the abort early than the 5
seconds so I'm not sure how we'd hit this.

May need it in future though.
>
> 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;

return ret_code == CXL_MBOX_CMD_RC_ABORT;

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