On Fri, 2016-06-10 at 16:54 -0600, Logan Gunthorpe wrote:
> This commit accomplishes a few things:
>
> 1) Properly prevent multiple sets of threads from running at once
> using
> a mutex. Lots of race issues existed with the thread_cleanup.
>
> 2) The mutex allows us to ensure that threads are finished before
> tearing down the device or module.
>
> 3) Don't use kthread_stop when the threads can exit by themselves, as
> this is counter-indicated by the kthread_create documentation.
> Threads
> now wait for kthread_stop to occur.
>
> 4) Writing to the run file now blocks until the threads are complete.
> The test can then be safely interrupted by a SIGINT.
>
> Also, while I was at it:
>
> 5) debugfs_run_write shouldn't return 0 in the early check cases as
> this
> could cause debugfs_run_write to loop undesirably.
>
> Signed-off-by: Logan Gunthorpe <log...@deltatee.com>
Acked-by: Dave Jiang <dave.ji...@intel.com>
> ---
> drivers/ntb/test/ntb_perf.c | 124 +++++++++++++++++++++++++++-------
> ----------
> 1 file changed, 76 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index 5008ccf..db4dc61 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -58,6 +58,7 @@
> #include <linux/delay.h>
> #include <linux/sizes.h>
> #include <linux/ntb.h>
> +#include <linux/mutex.h>
>
> #define DRIVER_NAME "ntb_perf"
> #define DRIVER_DESCRIPTION "PCIe NTB Performance Measurement
> Tool"
> @@ -121,6 +122,7 @@ struct pthr_ctx {
> int dma_prep_err;
> int src_idx;
> void *srcs[MAX_SRCS];
> + wait_queue_head_t *wq;
> };
>
> struct perf_ctx {
> @@ -134,9 +136,11 @@ struct perf_ctx {
> struct dentry *debugfs_run;
> struct dentry *debugfs_threads;
> u8 perf_threads;
> - bool run;
> + /* mutex ensures only one set of threads run at once */
> + struct mutex run_mutex;
> struct pthr_ctx pthr_ctx[MAX_THREADS];
> atomic_t tsync;
> + atomic_t tdone;
> };
>
> enum {
> @@ -295,12 +299,18 @@ static int perf_move_data(struct pthr_ctx
> *pctx, char __iomem *dst, char *src,
> set_current_state(TASK_INTERRUPTIBLE);
> schedule_timeout(1);
> }
> +
> + if (unlikely(kthread_should_stop()))
> + break;
> }
>
> if (use_dma) {
> pr_info("%s: All DMA descriptors submitted\n",
> current->comm);
> - while (atomic_read(&pctx->dma_sync) != 0)
> + while (atomic_read(&pctx->dma_sync) != 0) {
> + if (kthread_should_stop())
> + break;
> msleep(20);
> + }
> }
>
> kstop = ktime_get();
> @@ -393,7 +403,10 @@ static int ntb_perf_thread(void *data)
> pctx->srcs[i] = NULL;
> }
>
> - return 0;
> + atomic_inc(&perf->tdone);
> + wake_up(pctx->wq);
> + rc = 0;
> + goto done;
>
> err:
> for (i = 0; i < MAX_SRCS; i++) {
> @@ -406,6 +419,16 @@ err:
> pctx->dma_chan = NULL;
> }
>
> +done:
> + /* Wait until we are told to stop */
> + for (;;) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (kthread_should_stop())
> + break;
> + schedule();
> + }
> + __set_current_state(TASK_RUNNING);
> +
> return rc;
> }
>
> @@ -553,6 +576,7 @@ static ssize_t debugfs_run_read(struct file
> *filp, char __user *ubuf,
> struct perf_ctx *perf = filp->private_data;
> char *buf;
> ssize_t ret, out_offset;
> + int running;
>
> if (!perf)
> return 0;
> @@ -560,7 +584,9 @@ static ssize_t debugfs_run_read(struct file
> *filp, char __user *ubuf,
> buf = kmalloc(64, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
> - out_offset = snprintf(buf, 64, "%d\n", perf->run);
> +
> + running = mutex_is_locked(&perf->run_mutex);
> + out_offset = snprintf(buf, 64, "%d\n", running);
> ret = simple_read_from_buffer(ubuf, count, offp, buf,
> out_offset);
> kfree(buf);
>
> @@ -572,7 +598,6 @@ static void threads_cleanup(struct perf_ctx
> *perf)
> struct pthr_ctx *pctx;
> int i;
>
> - perf->run = false;
> for (i = 0; i < MAX_THREADS; i++) {
> pctx = &perf->pthr_ctx[i];
> if (pctx->thread) {
> @@ -587,65 +612,66 @@ static ssize_t debugfs_run_write(struct file
> *filp, const char __user *ubuf,
> {
> struct perf_ctx *perf = filp->private_data;
> int node, i;
> + DECLARE_WAIT_QUEUE_HEAD(wq);
>
> if (!perf->link_is_up)
> - return 0;
> + return -ENOLINK;
>
> if (perf->perf_threads == 0)
> - return 0;
> + return -EINVAL;
>
> - if (atomic_read(&perf->tsync) == 0)
> - perf->run = false;
> + if (!mutex_trylock(&perf->run_mutex))
> + return -EBUSY;
>
> - if (perf->run)
> - threads_cleanup(perf);
> - else {
> - perf->run = true;
> + if (perf->perf_threads > MAX_THREADS) {
> + perf->perf_threads = MAX_THREADS;
> + pr_info("Reset total threads to: %u\n",
> MAX_THREADS);
> + }
>
> - if (perf->perf_threads > MAX_THREADS) {
> - perf->perf_threads = MAX_THREADS;
> - pr_info("Reset total threads to: %u\n",
> MAX_THREADS);
> - }
> + /* no greater than 1M */
> + if (seg_order > MAX_SEG_ORDER) {
> + seg_order = MAX_SEG_ORDER;
> + pr_info("Fix seg_order to %u\n", seg_order);
> + }
>
> - /* no greater than 1M */
> - if (seg_order > MAX_SEG_ORDER) {
> - seg_order = MAX_SEG_ORDER;
> - pr_info("Fix seg_order to %u\n", seg_order);
> - }
> + if (run_order < seg_order) {
> + run_order = seg_order;
> + pr_info("Fix run_order to %u\n", run_order);
> + }
>
> - if (run_order < seg_order) {
> - run_order = seg_order;
> - pr_info("Fix run_order to %u\n", run_order);
> - }
> + node = dev_to_node(&perf->ntb->pdev->dev);
> + atomic_set(&perf->tdone, 0);
>
> - node = dev_to_node(&perf->ntb->pdev->dev);
> - /* launch kernel thread */
> - for (i = 0; i < perf->perf_threads; i++) {
> - struct pthr_ctx *pctx;
> -
> - pctx = &perf->pthr_ctx[i];
> - atomic_set(&pctx->dma_sync, 0);
> - pctx->perf = perf;
> - pctx->thread =
> - kthread_create_on_node(ntb_perf_thre
> ad,
> - (void *)pctx,
> - node,
> "ntb_perf %d", i);
> - if (IS_ERR(pctx->thread)) {
> - pctx->thread = NULL;
> - goto err;
> - } else
> - wake_up_process(pctx->thread);
> -
> - if (perf->run == false)
> - return -ENXIO;
> - }
> + /* launch kernel thread */
> + for (i = 0; i < perf->perf_threads; i++) {
> + struct pthr_ctx *pctx;
>
> + pctx = &perf->pthr_ctx[i];
> + atomic_set(&pctx->dma_sync, 0);
> + pctx->perf = perf;
> + pctx->wq = &wq;
> + pctx->thread =
> + kthread_create_on_node(ntb_perf_thread,
> + (void *)pctx,
> + node, "ntb_perf %d",
> i);
> + if (IS_ERR(pctx->thread)) {
> + pctx->thread = NULL;
> + goto err;
> + } else {
> + wake_up_process(pctx->thread);
> + }
> }
>
> + wait_event_interruptible(wq,
> + atomic_read(&perf->tdone) == perf->perf_threads);
> +
> + threads_cleanup(perf);
> + mutex_unlock(&perf->run_mutex);
> return count;
>
> err:
> threads_cleanup(perf);
> + mutex_unlock(&perf->run_mutex);
> return -ENXIO;
> }
>
> @@ -713,7 +739,7 @@ static int perf_probe(struct ntb_client *client,
> struct ntb_dev *ntb)
> perf->ntb = ntb;
> perf->perf_threads = 1;
> atomic_set(&perf->tsync, 0);
> - perf->run = false;
> + mutex_init(&perf->run_mutex);
> spin_lock_init(&perf->db_lock);
> perf_setup_mw(ntb, perf);
> INIT_DELAYED_WORK(&perf->link_work, perf_link_work);
> @@ -748,6 +774,8 @@ static void perf_remove(struct ntb_client
> *client, struct ntb_dev *ntb)
>
> dev_dbg(&perf->ntb->dev, "%s called\n", __func__);
>
> + mutex_lock(&perf->run_mutex);
> +
> cancel_delayed_work_sync(&perf->link_work);
> cancel_work_sync(&perf->link_cleanup);
>
> --
> 2.1.4
>
