| Message ID | 20201204174256.62xfcvudndt7oufl@linutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2] IB/iser: Remove in_interrupt() usage. | expand |
On Fri, Dec 04, 2020 at 06:42:56PM +0100, Sebastian Andrzej Siewior wrote: > iser_initialize_task_headers() uses in_interrupt() to find out if it is > safe to acquire a mutex. > > in_interrupt() is deprecated as it is ill defined and does not provide what > it suggests. Aside of that it covers only parts of the contexts in which > a mutex may not be acquired. > > The following callchains exist: > > iscsi_queuecommand() *locks* iscsi_session::frwd_lock > -> iscsi_prep_scsi_cmd_pdu() > -> session->tt->init_task() (iscsi_iser_task_init()) > -> iser_initialize_task_headers() > -> iscsi_iser_task_xmit() (iscsi_transport::xmit_task) > -> iscsi_iser_task_xmit_unsol_data() > -> iser_send_data_out() > -> iser_initialize_task_headers() > > iscsi_data_xmit() *locks* iscsi_session::frwd_lock > -> iscsi_prep_mgmt_task() > -> session->tt->init_task() (iscsi_iser_task_init()) > -> iser_initialize_task_headers() > -> iscsi_prep_scsi_cmd_pdu() > -> session->tt->init_task() (iscsi_iser_task_init()) > -> iser_initialize_task_headers() > > __iscsi_conn_send_pdu() caller has iscsi_session::frwd_lock > -> iscsi_prep_mgmt_task() > -> session->tt->init_task() (iscsi_iser_task_init()) > -> iser_initialize_task_headers() > -> session->tt->xmit_task() ( > > The only callchain that is close to be invoked in preemptible context: > iscsi_xmitworker() worker > -> iscsi_data_xmit() > -> iscsi_xmit_task() > -> conn->session->tt->xmit_task() (iscsi_iser_task_xmit() > > In iscsi_iser_task_xmit() there is this check: > if (!task->sc) > return iscsi_iser_mtask_xmit(conn, task); > > so it does end up in iser_initialize_task_headers() and > iser_initialize_task_headers() relies on iscsi_task::sc == NULL. > > Remove conditional locking of iser_conn::state_mutex because there is no > call chain to do so. Remove the goto label and return early now that > there is no clean up needed. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Sagi Grimberg <sagi@grimberg.me> > Cc: Max Gurtovoy <maxg@nvidia.com> > Cc: Doug Ledford <dledford@redhat.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: linux-rdma@vger.kernel.org > --- Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 3690e28cc7ea2..72fcccb459872 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -187,23 +187,14 @@ iser_initialize_task_headers(struct iscsi_task *task, struct iser_device *device = iser_conn->ib_conn.device; struct iscsi_iser_task *iser_task = task->dd_data; u64 dma_addr; - const bool mgmt_task = !task->sc && !in_interrupt(); - int ret = 0; - if (unlikely(mgmt_task)) - mutex_lock(&iser_conn->state_mutex); - - if (unlikely(iser_conn->state != ISER_CONN_UP)) { - ret = -ENODEV; - goto out; - } + if (unlikely(iser_conn->state != ISER_CONN_UP)) + return -ENODEV; dma_addr = ib_dma_map_single(device->ib_device, (void *)tx_desc, ISER_HEADERS_LEN, DMA_TO_DEVICE); - if (ib_dma_mapping_error(device->ib_device, dma_addr)) { - ret = -ENOMEM; - goto out; - } + if (ib_dma_mapping_error(device->ib_device, dma_addr)) + return -ENOMEM; tx_desc->inv_wr.next = NULL; tx_desc->reg_wr.wr.next = NULL; @@ -214,11 +205,8 @@ iser_initialize_task_headers(struct iscsi_task *task, tx_desc->tx_sg[0].lkey = device->pd->local_dma_lkey; iser_task->iser_conn = iser_conn; -out: - if (unlikely(mgmt_task)) - mutex_unlock(&iser_conn->state_mutex); - return ret; + return 0; } /**
iser_initialize_task_headers() uses in_interrupt() to find out if it is safe to acquire a mutex. in_interrupt() is deprecated as it is ill defined and does not provide what it suggests. Aside of that it covers only parts of the contexts in which a mutex may not be acquired. The following callchains exist: iscsi_queuecommand() *locks* iscsi_session::frwd_lock -> iscsi_prep_scsi_cmd_pdu() -> session->tt->init_task() (iscsi_iser_task_init()) -> iser_initialize_task_headers() -> iscsi_iser_task_xmit() (iscsi_transport::xmit_task) -> iscsi_iser_task_xmit_unsol_data() -> iser_send_data_out() -> iser_initialize_task_headers() iscsi_data_xmit() *locks* iscsi_session::frwd_lock -> iscsi_prep_mgmt_task() -> session->tt->init_task() (iscsi_iser_task_init()) -> iser_initialize_task_headers() -> iscsi_prep_scsi_cmd_pdu() -> session->tt->init_task() (iscsi_iser_task_init()) -> iser_initialize_task_headers() __iscsi_conn_send_pdu() caller has iscsi_session::frwd_lock -> iscsi_prep_mgmt_task() -> session->tt->init_task() (iscsi_iser_task_init()) -> iser_initialize_task_headers() -> session->tt->xmit_task() ( The only callchain that is close to be invoked in preemptible context: iscsi_xmitworker() worker -> iscsi_data_xmit() -> iscsi_xmit_task() -> conn->session->tt->xmit_task() (iscsi_iser_task_xmit() In iscsi_iser_task_xmit() there is this check: if (!task->sc) return iscsi_iser_mtask_xmit(conn, task); so it does end up in iser_initialize_task_headers() and iser_initialize_task_headers() relies on iscsi_task::sc == NULL. Remove conditional locking of iser_conn::state_mutex because there is no call chain to do so. Remove the goto label and return early now that there is no clean up needed. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Sagi Grimberg <sagi@grimberg.me> Cc: Max Gurtovoy <maxg@nvidia.com> Cc: Doug Ledford <dledford@redhat.com> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: linux-rdma@vger.kernel.org --- v1…v2: Remove the goto label and return early. Suggested by Sagi Grimberg. drivers/infiniband/ulp/iser/iscsi_iser.c | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-)