| Message ID | 00a0aa82ae12450a8c690e11440c4101@micron.com |
|---|---|
| State | New |
| Headers | show |
| Series | cxl: Support for mailbox background abort operation | expand |
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://lore.kernel.org/linux-cxl/66035c2e8ba17_770232948b@dwillia2-xfh.jf.intel.com.notmuch/ > > 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. So if nothing is going on, the command can have 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. 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 thing. > + 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;
>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;
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;
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) { + 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;