lkml.org 
[lkml]   [2016]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH 2/8] ntb_perf: Improve thread handling to increase robustness
Date
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 <logang@deltatee.com>
Acked-by: Dave Jiang <dave.jiang@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
>
\
 
 \ /
  Last update: 2016-06-13 21:01    [W:0.215 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site