diff --git a/scst/src/dev_handlers/kunit/user/suites/scst_user_cleanup_test.c b/scst/src/dev_handlers/kunit/user/suites/scst_user_cleanup_test.c index b9e6f4d3b..706a8879c 100644 --- a/scst/src/dev_handlers/kunit/user/suites/scst_user_cleanup_test.c +++ b/scst/src/dev_handlers/kunit/user/suites/scst_user_cleanup_test.c @@ -73,7 +73,9 @@ static int user_cleanup_init(struct kunit *test) u->dev = dev; u->state = UCMD_STATE_ATTACH_SESS; u->cmpl = &f->reply; - atomic_set(&u->ucmd_ref, 2); /* Fixture reference plus outstanding reply. */ + /* Fixture reference plus outstanding reply. */ + kref_init(&u->ucmd_ref); + kref_get(&u->ucmd_ref); INIT_LIST_HEAD(&u->hash_list_entry); INIT_LIST_HEAD(&u->ready_cmd_list_entry); } @@ -144,7 +146,7 @@ static void user_cleanup_shared_test(struct kunit *test) KUNIT_ASSERT_EQ(test, dev_user_process_cleanup(&f->devices[1]), 1); KUNIT_EXPECT_TRUE(test, completion_done(&f->reply)); KUNIT_EXPECT_EQ(test, f->pending[1].result, -EFAULT); - KUNIT_EXPECT_EQ(test, atomic_read(&f->pending[1].ucmd_ref), 1); + KUNIT_EXPECT_EQ(test, kref_read(&f->pending[1].ucmd_ref), 1); KUNIT_EXPECT_EQ(test, dev_user_process_cleanup(&f->devices[0]), 0); KUNIT_EXPECT_EQ(test, dev_user_process_cleanup(&f->devices[1]), 0); for (i = 0; i < 2; i++) @@ -203,16 +205,55 @@ static void user_reply_test(struct kunit *test) spin_unlock_irq(lock); KUNIT_EXPECT_EQ(test, completion_done(&f->reply), !p->no_completion); KUNIT_EXPECT_EQ(test, u->result, p->detach ? 0 : -EFAULT); - KUNIT_EXPECT_EQ(test, atomic_read(&u->ucmd_ref), 1); + KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 1); KUNIT_EXPECT_TRUE(test, !!u->jammed); KUNIT_EXPECT_TRUE(test, !!u->this_state_unjammed); KUNIT_EXPECT_FALSE(test, !!u->sent_to_user); } +static void user_defer_release(struct kref *ref) +{ + /* Model the destructor waiting for cmd_list_lock before hash removal. */ +} + +static void user_get_live_command(struct kunit *test) +{ + struct user_cleanup_fixture *f = test->priv; + struct scst_user_cmd *u = f->pending; + spinlock_t *lock = &f->devices[0].udev_cmd_threads.cmd_list_lock; + unsigned long flags; + bool skip; + + spin_lock_irqsave(lock, flags); + skip = ucmd_get_check(u); + spin_unlock_irqrestore(lock, flags); + KUNIT_EXPECT_FALSE(test, skip); + KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 3U); +} + +static void user_get_destroying_command(struct kunit *test) +{ + struct user_cleanup_fixture *f = test->priv; + struct scst_user_cmd *u = f->pending; + spinlock_t *lock = &f->devices[0].udev_cmd_threads.cmd_list_lock; + unsigned long flags; + bool skip; + + kref_put(&u->ucmd_ref, user_defer_release); + kref_put(&u->ucmd_ref, user_defer_release); + spin_lock_irqsave(lock, flags); + skip = ucmd_get_check(u); + spin_unlock_irqrestore(lock, flags); + KUNIT_EXPECT_TRUE(test, skip); + KUNIT_EXPECT_EQ(test, kref_read(&u->ucmd_ref), 0U); +} + static struct kunit_case user_cleanup_cases_kunit[] = { KUNIT_CASE_PARAM(user_cleanup_test, user_cleanup_gen_params), KUNIT_CASE(user_cleanup_shared_test), KUNIT_CASE_PARAM(user_reply_test, user_reply_gen_params), + KUNIT_CASE(user_get_live_command), + KUNIT_CASE(user_get_destroying_command), {} }; diff --git a/scst/src/dev_handlers/scst_user_main.c b/scst/src/dev_handlers/scst_user_main.c index a9e0e4b4b..404ca8150 100644 --- a/scst/src/dev_handlers/scst_user_main.c +++ b/scst/src/dev_handlers/scst_user_main.c @@ -133,47 +133,42 @@ static struct task_struct *cleanup_thread; * Skip this command if result is not 0. Must be called under * udev_cmd_threads.cmd_list_lock and IRQ off. */ -static inline bool ucmd_get_check(struct scst_user_cmd *ucmd) +SCST_KUNIT_VISIBLE_INLINE +bool ucmd_get_check(struct scst_user_cmd *ucmd) { - int r = atomic_inc_return(&ucmd->ucmd_ref); - int res; - - if (unlikely(r == 1)) { + /* Keep successful gets fully ordered, as atomic_inc_return() was. */ + smp_mb(); + if (unlikely(!kref_get_unless_zero(&ucmd->ucmd_ref))) { TRACE_DBG("ucmd %p is being destroyed", ucmd); - atomic_dec(&ucmd->ucmd_ref); - res = true; - /* - * Necessary code is serialized by cmd_list_lock in - * cmd_remove_hash() - */ - } else { - TRACE_DBG("ucmd %p, new ref_cnt %d", ucmd, - atomic_read(&ucmd->ucmd_ref)); - res = false; + return true; } - return res; + + smp_mb__after_atomic_inc(); + TRACE_DBG("ucmd %p, new ref_cnt %u", ucmd, kref_read(&ucmd->ucmd_ref)); + return false; } static inline void ucmd_get(struct scst_user_cmd *ucmd) { - TRACE_DBG("ucmd %p, ucmd_ref %d", ucmd, atomic_read(&ucmd->ucmd_ref)); - atomic_inc(&ucmd->ucmd_ref); - /* - * For the same reason as in kref_get(). Let's be safe and - * always do it. - */ + TRACE_DBG("ucmd %p, ucmd_ref %u", ucmd, kref_read(&ucmd->ucmd_ref)); + kref_get(&ucmd->ucmd_ref); + /* Preserve the ordering of SCST's command-reference acquisition. */ smp_mb__after_atomic_inc(); } +static void ucmd_release(struct kref *ref) +{ + dev_user_free_ucmd(container_of(ref, struct scst_user_cmd, ucmd_ref)); +} + /* Must not be called under cmd_list_lock!! */ static inline void ucmd_put(struct scst_user_cmd *ucmd) { - TRACE_DBG("ucmd %p, ucmd_ref %d", ucmd, atomic_read(&ucmd->ucmd_ref)); + TRACE_DBG("ucmd %p, ucmd_ref %u", ucmd, kref_read(&ucmd->ucmd_ref)); - EXTRACHECKS_BUG_ON(atomic_read(&ucmd->ucmd_ref) == 0); + EXTRACHECKS_BUG_ON(kref_read(&ucmd->ucmd_ref) == 0); - if (atomic_dec_and_test(&ucmd->ucmd_ref)) - dev_user_free_ucmd(ucmd); + kref_put(&ucmd->ucmd_ref, ucmd_release); } static inline int calc_num_pg(unsigned long buf, int len) @@ -598,7 +593,7 @@ static struct scst_user_cmd *dev_user_alloc_ucmd(struct scst_user_dev *dev, gfp_ goto out; } ucmd->dev = dev; - atomic_set(&ucmd->ucmd_ref, 1); + kref_init(&ucmd->ucmd_ref); cmd_insert_hash(ucmd); @@ -3796,8 +3791,8 @@ static void dev_user_check_lost_ucmds(struct scst_user_dev *dev) struct scst_user_cmd *ucmd2, *tmp; list_for_each_entry_safe(ucmd2, tmp, head, hash_list_entry) { - PRINT_ERROR("Lost ucmd %p (state %x, ref %d)", - ucmd2, ucmd2->state, atomic_read(&ucmd2->ucmd_ref)); + PRINT_ERROR("Lost ucmd %p (state %x, ref %u)", + ucmd2, ucmd2->state, kref_read(&ucmd2->ucmd_ref)); ucmd_put(ucmd2); } } @@ -3880,9 +3875,9 @@ static ssize_t dev_user_sysfs_commands_show(struct kobject *kobj, struct kobj_at list_for_each_entry(ucmd, head, hash_list_entry) { pos = ret; ret += sysfs_emit_at(buf, ret, - "ucmd %p (state %x, ref %d), sent_to_user %d, seen_by_user %d, aborted %d, jammed %d, scst_cmd %p\n", + "ucmd %p (state %x, ref %u), sent_to_user %d, seen_by_user %d, aborted %d, jammed %d, scst_cmd %p\n", ucmd, ucmd->state, - atomic_read(&ucmd->ucmd_ref), + kref_read(&ucmd->ucmd_ref), ucmd->sent_to_user, ucmd->seen_by_user, ucmd->aborted, ucmd->jammed, ucmd->cmd); if (ret >= SCST_SYSFS_BLOCK_SIZE - 1) { diff --git a/scst/src/dev_handlers/scst_user_priv.h b/scst/src/dev_handlers/scst_user_priv.h index 707244405..de994745d 100644 --- a/scst/src/dev_handlers/scst_user_priv.h +++ b/scst/src/dev_handlers/scst_user_priv.h @@ -78,7 +78,7 @@ struct scst_user_cmd { struct scst_user_cmd *buf_ucmd; - atomic_t ucmd_ref; + struct kref ucmd_ref; int cur_data_page; int num_data_pages; diff --git a/scst/src/dev_handlers/scst_user_test.h b/scst/src/dev_handlers/scst_user_test.h index f1a6e3021..37ec119d7 100644 --- a/scst/src/dev_handlers/scst_user_test.h +++ b/scst/src/dev_handlers/scst_user_test.h @@ -6,6 +6,7 @@ #ifdef SCST_KUNIT_ENABLED +bool ucmd_get_check(struct scst_user_cmd *ucmd); void dev_user_unjam_cmd(struct scst_user_cmd *ucmd, int busy, unsigned long *flags); void dev_user_flush_pool(struct sgv_pool *pool); int dev_user_process_cleanup(struct scst_user_dev *dev);