Re: [PATCH] IB/iser: Remove in_interrupt() usage.

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Nov 26, 2020 at 09:27:20PM +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.

AFAIK, there is no way to get into a hard IRQ from
drivers/infiniband/ulp/*

The closest it gets to real HW is a soft IRQ from the CQ handler,
starting at these functions:

drivers/infiniband/ulp/iser/iser_initiator.c:   tx_desc->cqe.done = iser_cmd_comp;
drivers/infiniband/ulp/iser/iser_initiator.c:   tx_desc->cqe.done = iser_dataout_comp;
drivers/infiniband/ulp/iser/iser_initiator.c:   mdesc->cqe.done = iser_ctrl_comp;
drivers/infiniband/ulp/iser/iser_verbs.c:       desc->cqe.done = iser_login_rsp;
drivers/infiniband/ulp/iser/iser_verbs.c:               rx_desc->cqe.done = iser_task_rsp;

So, I can't see any way in_interrupt() was ever detecting actual
interrupts. I wonder if it is was some hacky way to detect
non-preemption from a softirq or something?

> 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>
> ---
>  drivers/infiniband/ulp/iser/iscsi_iser.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
> index 3690e28cc7ea2..b34a1881c4cad 100644
> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c
> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
> @@ -187,12 +187,8 @@ 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;

Why do you think the task->sc doesn't matter?

> -	if (unlikely(mgmt_task))
> -		mutex_lock(&iser_conn->state_mutex);
> -
>  	if (unlikely(iser_conn->state != ISER_CONN_UP)) {
>  		ret = -ENODEV;
>  		goto out;
> @@ -215,9 +211,6 @@ iser_initialize_task_headers(struct iscsi_task *task,
>  
>  	iser_task->iser_conn = iser_conn;
>  out:
> -	if (unlikely(mgmt_task))
> -		mutex_unlock(&iser_conn->state_mutex);
> -
>  	return ret;
>  }

Sagi, you added this code, any rememberance of what it is for?

commit 7414dde0a6c3a958e26141991bf5c75dc58d28b2
Author: Sagi Grimberg <sagig@mellanox.com>
Date:   Sun Dec 7 16:09:59 2014 +0200

    IB/iser: Fix race between iser connection teardown and scsi TMFs
    
    In certain scenarios (target kill with live IO) scsi TMFs may race
    with iser RDMA teardown, which might cause NULL dereference on iser IB
    device handle (which might have been freed). In this case we take a
    conditional lock for TMFs and check the connection state (avoid
    introducing lock contention in the IO path). This is indeed best
    effort approach, but sufficient to survive multi targets sudden death
    while heavy IO is inflight.
    
    While we are on it, add a nice kernel-doc style documentation.

Max, can you do a test with this patch and we might luck into a
lockdep splat that will be informative?

Jason