From 1b3d5b60eb51f3772b167702ab5317de9c277e9b Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Sun, 3 Jan 2021 02:59:31 +0000 Subject: [PATCH] scst: Rework the mechanism for suspending activity Use a percpu_ref instead of one atomic counter per CPU. This patch eliminates two atomic instructions from the command processing path. This patch decreases performance for the following configurations because the backported version of percpu_ref uses a single global counter: * RHEL 6.x / CentOS 6.x and before (maintenance ended on 2020-11-30). * Upstream kernel version v3.10 and before. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@9321 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 4 +- scst/src/scst_lib.c | 14 +++--- scst/src/scst_main.c | 99 +++++++++++++++++++++++----------------- scst/src/scst_priv.h | 105 +++++++++++++++++++++---------------------- scst/src/scst_targ.c | 63 +++++++++++--------------- 5 files changed, 145 insertions(+), 140 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 153b8a9e9..30d0e2d34 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -2081,7 +2081,7 @@ struct scst_cmd { struct scst_session *sess; /* corresponding session */ - atomic_t *cpu_cmd_counter; + bool counted; atomic_t cmd_ref; @@ -2571,7 +2571,7 @@ struct scst_mgmt_cmd { struct scst_session *sess; - atomic_t *cpu_cmd_counter; + bool counted; /* Mgmt cmd state, one of SCST_MCMD_STATE_* constants */ int state; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 23a4f0acd..61eb4e789 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -5824,7 +5824,7 @@ struct scst_cmd *__scst_create_prepare_internal_cmd(const uint8_t *cdb, } scst_sess_get(res->sess); - res->cpu_cmd_counter = scst_get(); + scst_get_icmd(res); TRACE(TRACE_SCSI, "New internal cmd %p (op %s)", res, scst_get_opcode_name(res)); @@ -7534,10 +7534,8 @@ static void scst_destroy_cmd(struct scst_cmd *cmd) scst_sess_put(cmd->sess); - if (likely(cmd->cpu_cmd_counter)) { - scst_put(cmd->cpu_cmd_counter); - cmd->cpu_cmd_counter = NULL; - } + if (likely(cmd->counted)) + scst_put_cmd(cmd); EXTRACHECKS_BUG_ON(cmd->pre_alloced && cmd->internal); @@ -7777,10 +7775,8 @@ void scst_free_mgmt_cmd(struct scst_mgmt_cmd *mcmd) scst_sess_put(mcmd->sess); - if (mcmd->cpu_cmd_counter) { - scst_put(mcmd->cpu_cmd_counter); - mcmd->cpu_cmd_counter = NULL; - } + if (mcmd->counted) + scst_put_mcmd(mcmd); mempool_free(mcmd, scst_mgmt_mempool); diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index 6fa785d15..f19889273 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -112,8 +112,6 @@ struct kmem_cache *scst_cmd_cachep; unsigned long scst_trace_flag; #endif -unsigned long scst_flags; - #if LINUX_VERSION_CODE >= KERNEL_VERSION(3, 9, 0) unsigned long scst_poll_ns = SCST_DEF_POLL_NS; #endif @@ -122,6 +120,9 @@ int scst_max_tasklet_cmd = SCST_DEF_MAX_TASKLET_CMD; struct scst_cmd_threads scst_main_cmd_threads; +static bool percpu_ref_killed; +struct percpu_ref scst_cmd_count; +struct percpu_ref scst_mcmd_count; struct scst_percpu_info scst_percpu_infos[NR_CPUS]; spinlock_t scst_mcmd_lock; @@ -796,13 +797,15 @@ static void __printf(2, 3) scst_to_syslog(void *arg, const char *fmt, ...) return; } +/* + * Number of SCST non-management commands, management commands and activities + * that are in progress. Must only be called if both scst_cmd_count and + * scst_mcmd_count are in atomic mode. + */ int scst_get_cmd_counter(void) { - int i, res = 0; - - for (i = 0; i < ARRAY_SIZE(scst_percpu_infos); i++) - res += atomic_read(&scst_percpu_infos[i].cpu_cmd_count); - return res; + return percpu_ref_read(&scst_cmd_count) + + percpu_ref_read(&scst_mcmd_count); } static int scst_susp_wait(unsigned long timeout) @@ -820,7 +823,7 @@ static int scst_susp_wait(unsigned long timeout) t = min(timeout, SCST_SUSP_WAIT_REPORT_TIMEOUT); res = wait_event_interruptible_timeout(scst_dev_cmd_waitQ, - (scst_get_cmd_counter() == 0), t); + percpu_ref_killed, t); if (res > 0) { res = 0; goto out; @@ -836,13 +839,13 @@ static int scst_susp_wait(unsigned long timeout) if (timeout != SCST_SUSPEND_TIMEOUT_UNLIMITED) { res = wait_event_interruptible_timeout(scst_dev_cmd_waitQ, - (scst_get_cmd_counter() == 0), timeout - t); + percpu_ref_killed, timeout - t); if (res == 0) res = -EBUSY; else if (res > 0) res = 0; } else { - wait_event(scst_dev_cmd_waitQ, scst_get_cmd_counter() == 0); + wait_event(scst_dev_cmd_waitQ, percpu_ref_killed); res = 0; } @@ -855,17 +858,18 @@ out: } /* - * scst_suspend_activity() - globally suspend any activity + * scst_suspend_activity() - globally suspend activity * * Description: - * Globally suspends any activity and doesn't return, until there are any - * active commands (state after SCST_CMD_STATE_INIT). Timeout parameter sets - * max time this function will wait for suspending or interrupted by a - * signal with the corresponding error status < 0. If timeout is - * SCST_SUSPEND_TIMEOUT_UNLIMITED, then it will wait virtually forever. - * On success returns 0. + * Globally suspends SCSI command and SCSI management command processing and + * waits until all active commands have finished (state after + * SCST_CMD_STATE_INIT). The timeout parameter defines the maximum time this + * function will wait until activity has been suspended. If this function is + * interrupted by a signal, it returns a negative value. If the timeout value + * is SCST_SUSPEND_TIMEOUT_UNLIMITED, then it will wait virtually forever. + * Returns 0 upon success. * - * New arriving commands stay in the suspended state until + * Newly arriving commands remain in the suspended state until * scst_resume_activity() is called. */ int scst_suspend_activity(unsigned long timeout) @@ -892,14 +896,9 @@ int scst_suspend_activity(unsigned long timeout) if (suspend_count > 1) goto out_up; - set_bit(SCST_FLAG_SUSPENDING, &scst_flags); - set_bit(SCST_FLAG_SUSPENDED, &scst_flags); - /* - * Assignment of SCST_FLAG_SUSPENDING and SCST_FLAG_SUSPENDED must be - * ordered with cpu_cmd_count in scst_get(). Otherwise, lockless logic - * of scst_get() users won't work. - */ - smp_mb__after_set_bit(); + /* Cause scst_get_cmd() to fail. */ + percpu_ref_killed = false; + percpu_ref_kill(&scst_cmd_count); /* * See comment in scst_user.c::dev_user_task_mgmt_fn() for more @@ -921,12 +920,13 @@ int scst_suspend_activity(unsigned long timeout) } res = scst_susp_wait(timeout); - if (res != 0) - goto out_clear; - clear_bit(SCST_FLAG_SUSPENDING, &scst_flags); - /* See comment about smp_mb() above */ - smp_mb__after_clear_bit(); + /* Cause scst_get_mcmd() to fail. */ + percpu_ref_killed = false; + percpu_ref_kill(&scst_mcmd_count); + + if (res != 0) + goto out_resume; if (scst_get_cmd_counter() != 0) TRACE_MGMT_DBG("Waiting for %d active commands finally to " @@ -964,11 +964,6 @@ out: TRACE_EXIT_RES(res); return res; -out_clear: - clear_bit(SCST_FLAG_SUSPENDING, &scst_flags); - /* See comment about smp_mb() above */ - smp_mb__after_clear_bit(); - out_resume: __scst_resume_activity(); EXTRACHECKS_BUG_ON(suspend_count != 0); @@ -994,7 +989,8 @@ static void __scst_resume_activity(void) if (suspend_count > 0) goto out; - clear_bit(SCST_FLAG_SUSPENDED, &scst_flags); + percpu_ref_resurrect(&scst_mcmd_count); + percpu_ref_resurrect(&scst_cmd_count); mutex_lock(&scst_cmd_threads_mutex); list_for_each_entry(l, &scst_cmd_threads_list, lists_list_entry) { @@ -1004,7 +1000,7 @@ static void __scst_resume_activity(void) /* * Wait until scst_init_thread() either is waiting or has reexamined - * scst_flags. + * scst_cmd_count. */ spin_lock_irq(&scst_init_lock); spin_unlock_irq(&scst_init_lock); @@ -2318,6 +2314,13 @@ static void __init scst_print_config(void) PRINT_INFO("%s", buf); } +static void scst_suspended(struct percpu_ref *ref) +{ + WARN_ON_ONCE(ref != &scst_cmd_count && ref != &scst_mcmd_count); + percpu_ref_killed = true; + wake_up_all(&scst_dev_cmd_waitQ); +} + static int __init init_scst(void) { int res, i; @@ -2526,8 +2529,17 @@ static int __init init_scst(void) if (res != 0) goto out_destroy_sgv_pool; + res = percpu_ref_init(&scst_cmd_count, scst_suspended, + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL); + if (res != 0) + goto out_unreg_interface; + + res = percpu_ref_init(&scst_mcmd_count, scst_suspended, + PERCPU_REF_ALLOW_REINIT, GFP_KERNEL); + if (res != 0) + goto out_cmd_count; + for (i = 0; i < ARRAY_SIZE(scst_percpu_infos); i++) { - atomic_set(&scst_percpu_infos[i].cpu_cmd_count, 0); spin_lock_init(&scst_percpu_infos[i].tasklet_lock); INIT_LIST_HEAD(&scst_percpu_infos[i].tasklet_cmd_list); tasklet_init(&scst_percpu_infos[i].tasklet, @@ -2565,10 +2577,14 @@ out: out_thread_free: scst_stop_global_threads(); + percpu_ref_exit(&scst_mcmd_count); +out_cmd_count: + percpu_ref_exit(&scst_cmd_count); + +out_unreg_interface: scsi_unregister_interface(&scst_interface); - out_destroy_sgv_pool: scst_sgv_pools_deinit(); scst_tg_cleanup(); @@ -2651,6 +2667,9 @@ static void __exit exit_scst(void) scst_deinit_threads(&scst_main_cmd_threads); + percpu_ref_exit(&scst_mcmd_count); + percpu_ref_exit(&scst_cmd_count); + scsi_unregister_interface(&scst_interface); diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index 21061c7a1..9f3437c3b 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -74,20 +74,6 @@ extern unsigned long scst_trace_flag; #endif -/** - ** Bits for scst_flags - **/ - -/* - * Set if new commands initialization is being suspended for a while. - * Used to let TM commands execute while preparing the suspend, since - * RESET or ABORT could be necessary to free SCSI commands. - */ -#define SCST_FLAG_SUSPENDING 0 - -/* Set if new commands initialization is suspended for a while */ -#define SCST_FLAG_SUSPENDED 1 - extern spinlock_t scst_measure_latency_lock; extern atomic_t scst_measure_latency; void scst_update_latency_stats(struct scst_cmd *cmd, int new_state); @@ -184,7 +170,6 @@ extern struct kmem_cache *scst_tgt_cachep; extern struct kmem_cache *scst_tgtd_cachep; extern struct kmem_cache *scst_acgd_cachep; -extern unsigned long scst_flags; extern struct list_head scst_template_list; extern struct list_head scst_dev_list; extern struct list_head scst_dev_type_list; @@ -217,8 +202,9 @@ extern struct list_head scst_active_mgmt_cmd_list; extern struct list_head scst_delayed_mgmt_cmd_list; extern wait_queue_head_t scst_mgmt_cmd_list_waitQ; +extern struct percpu_ref scst_cmd_count; +extern struct percpu_ref scst_mcmd_count; struct scst_percpu_info { - atomic_t cpu_cmd_count; spinlock_t tasklet_lock; struct list_head tasklet_cmd_list; struct tasklet_struct tasklet; @@ -671,51 +657,64 @@ void scst_ext_blocking_done(struct scst_device *dev); int scst_get_suspend_count(void); /* - * Increases global SCST ref counters which prevent from entering into suspended - * activities stage, so protects from any global management operations. + * Increase the global command count if it has not been 'killed'. Use this + * function to protect regular commands. */ -static inline atomic_t *scst_get(void) +static inline bool __must_check scst_get_cmd(struct scst_cmd *cmd) { - atomic_t *a; - - /* - * Avoid that a high I/O load prevents activity to be suspended. See - * also http://sourceforge.net/p/scst/mailman/message/34074831/. - */ - if (unlikely(test_bit(SCST_FLAG_SUSPENDING, &scst_flags))) - mdelay(100); - - /* - * We don't mind if we because of preemption inc counter from another - * CPU as soon in the majority cases we will the correct one. - */ - a = &scst_percpu_infos[raw_smp_processor_id()].cpu_cmd_count; - atomic_inc(a); - TRACE_DBG("Incrementing cpu_cmd_count %p (new value %d)", - a, atomic_read(a)); - /* See comment about smp_mb() in scst_suspend_activity() */ - smp_mb__after_atomic_inc(); - - return a; + if (!percpu_ref_tryget_live(&scst_cmd_count)) + return false; + cmd->counted = true; + return true; } /* - * Decreases global SCST ref counters which prevent from entering into suspended - * activities stage, so protects from any global management operations. On - * all them zero, if suspending activities is waiting, it will be proceed. + * Increase the global management command count if it is not zero. Use this + * function to protect management commands. */ -static inline void scst_put(atomic_t *a) +static inline bool __must_check scst_get_mcmd(struct scst_mgmt_cmd *mcmd) { - int f; + if (!percpu_ref_tryget_live(&scst_mcmd_count)) + return false; + mcmd->counted = true; + return true; +} - f = atomic_dec_and_test(a); - /* See comment about smp_mb() in scst_suspend_activity() */ - if (unlikely(test_bit(SCST_FLAG_SUSPENDED, &scst_flags)) && f) { - TRACE_MGMT_DBG("%s", "Waking up scst_dev_cmd_waitQ"); - wake_up_all(&scst_dev_cmd_waitQ); - } - TRACE_DBG("Decrementing cpu_cmd_count %p (new value %d)", - a, atomic_read(a)); +/* + * Increase the global command count. Use this function to protect internal + * commands. + */ +static inline void scst_get_icmd(struct scst_cmd *cmd) +{ + percpu_ref_get(&scst_cmd_count); + cmd->counted = true; +} + +/* Decrease the global SCST refcount which prevents suspending activity. */ +static inline void scst_put_cmd(struct scst_cmd *cmd) +{ + WARN_ON_ONCE(!cmd->counted); + cmd->counted = false; + percpu_ref_put(&scst_cmd_count); +} + +static inline void scst_put_mcmd(struct scst_mgmt_cmd *mcmd) +{ + WARN_ON_ONCE(!mcmd->counted); + mcmd->counted = false; + percpu_ref_put(&scst_mcmd_count); +} + +/* Whether or not activities are being suspended or have been suspended. */ +static inline bool scst_activity_suspended(void) +{ + return percpu_ref_is_dying(&scst_cmd_count); +} + +/* Returns true if and only if regular commands have already been suspended. */ +static inline bool scst_mcmd_suspended(void) +{ + return percpu_ref_is_dying(&scst_mcmd_count); } int scst_get_cmd_counter(void); diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 494f6163d..a3ef29bd1 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -574,7 +574,7 @@ static void __scst_rx_cmd(struct scst_cmd *cmd, struct scst_session *sess, { TRACE_ENTRY(); - WARN_ON_ONCE(cmd->cpu_cmd_counter); + WARN_ON_ONCE(cmd->counted); cmd->sess = sess; scst_sess_get(sess); @@ -4159,7 +4159,7 @@ struct scst_tgt_dev *scst_lookup_tgt_dev(struct scst_session *sess, u64 lun) * scst_translate_lun() - Translate @cmd->lun into a tgt_dev pointer. * @cmd: SCSI command for which to translate the LUN number. * - * Initialize the following @cmd members: cpu_cmd_counter, cmd_threads, + * Initialize the following @cmd members: counted, cmd_threads, * tgt_dev, cur_order_data, dev and devt. * * The caller must not hold any locks. May be called from IRQ context. The data @@ -4177,9 +4177,7 @@ static int scst_translate_lun(struct scst_cmd *cmd) TRACE_ENTRY(); - cmd->cpu_cmd_counter = scst_get(); - - if (likely(!test_bit(SCST_FLAG_SUSPENDED, &scst_flags))) { + if (likely(scst_get_cmd(cmd))) { TRACE_DBG("Finding tgt_dev for cmd %p (lun %lld)", cmd, (unsigned long long)cmd->lun); res = -1; @@ -4222,12 +4220,9 @@ static int scst_translate_lun(struct scst_cmd *cmd) cmd->sess->initiator_name, cmd->tgt->tgt_name); scst_event_queue_lun_not_found(cmd); } - scst_put(cmd->cpu_cmd_counter); - cmd->cpu_cmd_counter = NULL; + scst_put_cmd(cmd); } } else { - scst_put(cmd->cpu_cmd_counter); - cmd->cpu_cmd_counter = NULL; TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); res = 1; } @@ -4436,7 +4431,8 @@ restart: * There is no need for read barrier here, because we don't care where * this check will be done. */ - susp = test_bit(SCST_FLAG_SUSPENDED, &scst_flags); + susp = scst_activity_suspended(); + if (scst_init_poll_cnt > 0) scst_init_poll_cnt--; @@ -4497,13 +4493,13 @@ restart: return; } -static inline int test_init_cmd_list(void) +/* Whether or not scst_init_thread() should stop waiting. */ +static inline bool test_init_cmd_list(void) { - int res = (!list_empty(&scst_init_cmd_list) && - !test_bit(SCST_FLAG_SUSPENDED, &scst_flags)) || - unlikely(kthread_should_stop()) || - (scst_init_poll_cnt > 0); - return res; + return (!list_empty(&scst_init_cmd_list) && + !scst_activity_suspended()) || + unlikely(kthread_should_stop()) || + (scst_init_poll_cnt > 0); } int scst_init_thread(void *arg) @@ -5024,9 +5020,8 @@ void scst_cmd_tasklet(long p) } /* - * Returns 0 on success, or > 0 if SCST_FLAG_SUSPENDED set and - * SCST_FLAG_SUSPENDING - not. No locks, protection is done by the - * suspended activity. + * Returns 0 on success, or > 0 upon failure. No locks, protection is done by + * suspending activity. */ static int scst_get_mgmt(struct scst_mgmt_cmd *mcmd) { @@ -5034,12 +5029,7 @@ static int scst_get_mgmt(struct scst_mgmt_cmd *mcmd) TRACE_ENTRY(); - mcmd->cpu_cmd_counter = scst_get(); - - if (unlikely(test_bit(SCST_FLAG_SUSPENDED, &scst_flags) && - !test_bit(SCST_FLAG_SUSPENDING, &scst_flags))) { - scst_put(mcmd->cpu_cmd_counter); - mcmd->cpu_cmd_counter = NULL; + if (unlikely(!scst_get_mcmd(mcmd))) { TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); res = 1; goto out; @@ -5051,9 +5041,8 @@ out: } /* - * Returns 0 on success, < 0 if there is no device handler or - * > 0 if SCST_FLAG_SUSPENDED set and SCST_FLAG_SUSPENDING - not. - * No locks, protection is done by the suspended activity. + * Returns 0 on success, < 0 if there is no device handler or > 0 if activity + * has been suspended. No locks, protection is done by the suspended activity. */ static int scst_mgmt_translate_lun(struct scst_mgmt_cmd *mcmd) { @@ -5078,8 +5067,7 @@ static int scst_mgmt_translate_lun(struct scst_mgmt_cmd *mcmd) mcmd->mcmd_tgt_dev = tgt_dev; res = 0; } else { - scst_put(mcmd->cpu_cmd_counter); - mcmd->cpu_cmd_counter = NULL; + scst_put_mcmd(mcmd); res = -1; } @@ -5911,10 +5899,14 @@ static int scst_mgmt_cmd_init(struct scst_mgmt_cmd *mcmd) res = scst_set_mcmd_next_state(mcmd); goto out; } - __scst_cmd_get(cmd); tgt_dev = cmd->tgt_dev; - if (tgt_dev != NULL) - mcmd->cpu_cmd_counter = scst_get(); + if (tgt_dev && !scst_get_mcmd(mcmd)) { + TRACE_MGMT_DBG("Suspended; skipping mcmd"); + spin_unlock_irq(&sess->sess_list_lock); + res = 1; + goto ret; + } + __scst_cmd_get(cmd); spin_unlock_irq(&sess->sess_list_lock); TRACE_DBG("Cmd to abort %p for tag %llu found (tgt_dev %p)", cmd, (unsigned long long)mcmd->tag, tgt_dev); @@ -5972,6 +5964,7 @@ static int scst_mgmt_cmd_init(struct scst_mgmt_cmd *mcmd) out: scst_event_queue_tm_fn_received(mcmd); +ret: TRACE_EXIT_RES(res); return res; } @@ -6851,9 +6844,7 @@ int scst_tm_thread(void *arg) rc = scst_process_mgmt_cmd(mcmd); spin_lock_irq(&scst_mcmd_lock); if (rc > 0) { - if (test_bit(SCST_FLAG_SUSPENDED, &scst_flags) && - !test_bit(SCST_FLAG_SUSPENDING, - &scst_flags)) { + if (scst_mcmd_suspended()) { TRACE_MGMT_DBG("Adding mgmt cmd %p to " "head of delayed mgmt cmd list", mcmd);