On Mon, Sep 17, 2018 at 02:35:42PM -0700, Bart Van Assche wrote:
> A session must only be released after all code that accesses the session
> structure has finished. Make sure that this is the case by introducing a
> new command counter per session that is only decremented after the
> .release_cmd() callback has finished.
Can you explain what problems we are running into right now due to the
lack of a refcount?
> @@ -2897,6 +2907,8 @@ void target_sess_cmd_list_set_waiting(struct se_session *se_sess)
> spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> se_sess->sess_tearing_down = 1;
> spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
> +
> + percpu_ref_kill_and_confirm(&se_sess->cmd_count, NULL);
This is equivalent ot a plain percpu_ref_kill call.
> @@ -2911,17 +2923,14 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
>
> WARN_ON_ONCE(!se_sess->sess_tearing_down);
>
> - spin_lock_irq(&se_sess->sess_cmd_lock);
> do {
> - ret = wait_event_interruptible_lock_irq_timeout(
> - se_sess->cmd_list_wq,
> - list_empty(&se_sess->sess_cmd_list),
> - se_sess->sess_cmd_lock, 180 * HZ);
> + ret = wait_event_timeout(se_sess->cmd_list_wq,
> + percpu_ref_is_zero(&se_sess->cmd_count),
> + 180 * HZ);
So this is basically the big change - check for a zero reference instead
of the list_empty.
I fail to see how this makes a difference, and also why we even need a
percpu ref as the get/pull calls are under, or right next to
sess_cmd_lock critical sections.
