[PATCH 0/3] iscsi-target: Misc thread_set improvements

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

Hi folks,

Here is a small set of miscellaneous improvements to the iscsi-target
thread_set code to reduce latency for handling 100's of simultaneous
logins, that goes along with iscsi-target login multi-plexing logic
in-flight for v3.12-rc1.

The first patch fixes a long-standing race between rx/tx pre handlers
use of flush_signals(), and iscsi_deallocate_extra_thread_sets() setting
of ISCSI_THREAD_SET_DIE before calling kthread_stop().

The second patch removes the iscsi_thread_set->[rx,tx]_post_start_comp
that was originally used synchronize startup between rx and tx threads
within a single thread_set.

And the third patch removes an unnecessary wait_for_completion() within
iscsi_get_thread_set(), that originally would wait an extra second when
no thread_set pair was available.

Thanks,

--nab

Nicholas Bellinger (3):
iscsi-target: Fix race with thread_pre_handler flush_signals +
ISCSI_THREAD_SET_DIE
iscsi-target: Remove iscsi_thread_set->[rx,tx]_post_start_comp
iscsi-target: Remove unnecessary wait_for_completion in
iscsi_get_thread_set

drivers/target/iscsi/iscsi_target_tq.c | 63 +++++++++++---------------------
drivers/target/iscsi/iscsi_target_tq.h | 4 --
2 files changed, 22 insertions(+), 45 deletions(-)

--
1.7.2.5

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch addresses an long standing race in iscsi_[rx,tx]_thread_pre_handler()
use of flush_signals(), and between iscsi_deallocate_extra_thread_sets() setting
ISCSI_THREAD_SET_DIE before calling kthread_stop().

It addresses the issue by both holding ts_state_lock before calling send_sig()
in iscsi_deallocate_extra_thread_sets(), as well as only calling flush_signals()
when ts->status != ISCSI_THREAD_SET_DIE within iscsi_[rx,tx]_thread_pre_handler()
code.

Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---
drivers/target/iscsi/iscsi_target_tq.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_tq.c b/drivers/target/iscsi/iscsi_target_tq.c
index 8128952..8e9e4d8 100644
--- a/drivers/target/iscsi/iscsi_target_tq.c
+++ b/drivers/target/iscsi/iscsi_target_tq.c
@@ -189,16 +189,20 @@ static void iscsi_deallocate_extra_thread_sets(void)

spin_lock_bh(&ts->ts_state_lock);
ts->status = ISCSI_THREAD_SET_DIE;
- spin_unlock_bh(&ts->ts_state_lock);

if (ts->rx_thread) {
send_sig(SIGINT, ts->rx_thread, 1);
+ spin_unlock_bh(&ts->ts_state_lock);
kthread_stop(ts->rx_thread);
+ spin_lock_bh(&ts->ts_state_lock);
}
if (ts->tx_thread) {
send_sig(SIGINT, ts->tx_thread, 1);
+ spin_unlock_bh(&ts->ts_state_lock);
kthread_stop(ts->tx_thread);
+ spin_lock_bh(&ts->ts_state_lock);
}
+ spin_unlock_bh(&ts->ts_state_lock);
/*
* Release this thread_id in the thread_set_bitmap
*/
@@ -419,7 +423,8 @@ struct iscsi_conn *iscsi_rx_thread_pre_handler(struct iscsi_thread_set *ts)
goto sleep;
}

- flush_signals(current);
+ if (ts->status != ISCSI_THREAD_SET_DIE)
+ flush_signals(current);

if (ts->delay_inactive && (--ts->thread_count == 0)) {
spin_unlock_bh(&ts->ts_state_lock);
@@ -472,7 +477,8 @@ struct iscsi_conn *iscsi_tx_thread_pre_handler(struct iscsi_thread_set *ts)
goto sleep;
}

- flush_signals(current);
+ if (ts->status != ISCSI_THREAD_SET_DIE)
+ flush_signals(current);

if (ts->delay_inactive && (--ts->thread_count == 0)) {
spin_unlock_bh(&ts->ts_state_lock);
--
1.7.2.5

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch removes the iscsi_thread_set->[rx,tx]_post_start_comp that

was originally used synchronize startup between rx and tx threads within
a single thread_set.

It changes the logic to simply call complete(&ts->[rx,tx]_start_comp)
from iscsi_activate_thread_set() to wake up both sleeping processes in
iscsi_[rx,tx]_thread_pre_handler(), and set ISCSI_THREAD_SET_ACTIVE
before returning.

This means that iscsi_activate_thread_set() will no longer sleep while
waiting for iscsi_[rx,tx]_thread_pre_handler() to awake.

Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---

drivers/target/iscsi/iscsi_target_tq.c | 32 ++++++++++----------------------
drivers/target/iscsi/iscsi_target_tq.h | 4 ----
2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_tq.c b/drivers/target/iscsi/iscsi_target_tq.c
index 8e9e4d8..fe99ef6 100644
--- a/drivers/target/iscsi/iscsi_target_tq.c
+++ b/drivers/target/iscsi/iscsi_target_tq.c
@@ -105,8 +105,6 @@ int iscsi_allocate_thread_sets(u32 thread_pair_count)
ts->status = ISCSI_THREAD_SET_FREE;
INIT_LIST_HEAD(&ts->ts_list);
spin_lock_init(&ts->ts_state_lock);
- init_completion(&ts->rx_post_start_comp);
- init_completion(&ts->tx_post_start_comp);
init_completion(&ts->rx_restart_comp);
init_completion(&ts->tx_restart_comp);
init_completion(&ts->rx_start_comp);
@@ -229,13 +227,13 @@ void iscsi_activate_thread_set(struct iscsi_conn *conn, struct iscsi_thread_set
conn->thread_set = ts;
ts->conn = conn;
spin_unlock_bh(&ts->ts_state_lock);
- /*
- * Start up the RX thread and wait on rx_post_start_comp. The RX
- * Thread will then do the same for the TX Thread in
- * iscsi_rx_thread_pre_handler().
- */
+
complete(&ts->rx_start_comp);
- wait_for_completion(&ts->rx_post_start_comp);
+ complete(&ts->tx_start_comp);
+
+ spin_lock_bh(&ts->ts_state_lock);
+ ts->status = ISCSI_THREAD_SET_ACTIVE;
+ spin_unlock_bh(&ts->ts_state_lock);
}

struct iscsi_thread_set *iscsi_get_thread_set(void)
@@ -457,12 +455,10 @@ sleep:
goto sleep;
}
iscsi_check_to_add_additional_sets();
- /*
- * The RX Thread starts up the TX Thread and sleeps.
- */
+
+ spin_lock_bh(&ts->ts_state_lock);
ts->thread_clear |= ISCSI_CLEAR_RX_THREAD;
- complete(&ts->tx_start_comp);
- wait_for_completion(&ts->tx_post_start_comp);
+ spin_unlock_bh(&ts->ts_state_lock);

return ts->conn;
}
@@ -512,17 +508,9 @@ sleep:
}

iscsi_check_to_add_additional_sets();
- /*
- * From the TX thread, up the tx_post_start_comp that the RX Thread is
- * sleeping on in iscsi_rx_thread_pre_handler(), then up the
- * rx_post_start_comp that iscsi_activate_thread_set() is sleeping on.
- */
- ts->thread_clear |= ISCSI_CLEAR_TX_THREAD;
- complete(&ts->tx_post_start_comp);
- complete(&ts->rx_post_start_comp);

spin_lock_bh(&ts->ts_state_lock);
- ts->status = ISCSI_THREAD_SET_ACTIVE;
+ ts->thread_clear |= ISCSI_CLEAR_TX_THREAD;
spin_unlock_bh(&ts->ts_state_lock);

return ts->conn;
diff --git a/drivers/target/iscsi/iscsi_target_tq.h b/drivers/target/iscsi/iscsi_target_tq.h
index 547d118..3770396 100644
--- a/drivers/target/iscsi/iscsi_target_tq.h
+++ b/drivers/target/iscsi/iscsi_target_tq.h
@@ -64,10 +64,6 @@ struct iscsi_thread_set {
struct iscsi_conn *conn;
/* used for controlling ts state accesses */
spinlock_t ts_state_lock;
- /* Used for rx side post startup */
- struct completion rx_post_start_comp;
- /* Used for tx side post startup */
- struct completion tx_post_start_comp;
/* used for restarting thread queue */
struct completion rx_restart_comp;
/* used for restarting thread queue */
--
1.7.2.5

[Nicholas A. Bellinger]

From: Nicholas Bellinger <n...@linux-iscsi.org>

This patch removes an unnecessary wait_for_completion within
iscsi_get_thread_set(), that would wait for 1 second before
trying to obtain an inactive struct iscsi_thread_set from
iscsi_get_ts_from_inactive_list().

Since iscsi_allocate_thread_sets() will already be adding the
newly allocated iscsi_thread_set to the inactive list directly,
there is no need to wait here.

Signed-off-by: Nicholas Bellinger <n...@linux-iscsi.org>
---

drivers/target/iscsi/iscsi_target_tq.c | 19 +++----------------
1 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_tq.c b/drivers/target/iscsi/iscsi_target_tq.c
index fe99ef6..39c7dfa 100644
--- a/drivers/target/iscsi/iscsi_target_tq.c
+++ b/drivers/target/iscsi/iscsi_target_tq.c
@@ -238,25 +238,12 @@ void iscsi_activate_thread_set(struct iscsi_conn *conn, struct iscsi_thread_set

struct iscsi_thread_set *iscsi_get_thread_set(void)
{
- int allocate_ts = 0;
- struct completion comp;
- struct iscsi_thread_set *ts = NULL;
- /*
- * If no inactive thread set is available on the first call to
- * iscsi_get_ts_from_inactive_list(), sleep for a second and
- * try again. If still none are available after two attempts,
- * allocate a set ourselves.
- */
+ struct iscsi_thread_set *ts;
+
get_set:
ts = iscsi_get_ts_from_inactive_list();
if (!ts) {
- if (allocate_ts == 2)
- iscsi_allocate_thread_sets(1);
-
- init_completion(&comp);
- wait_for_completion_timeout(&comp, 1 * HZ);
-
- allocate_ts++;
+ iscsi_allocate_thread_sets(1);
goto get_set;
}

--
1.7.2.5