Re: [PATCH v4 10/10] ntb_perf: clear link_is_up flag when the link goes down.

[Jiang, Dave]

On Thu, 2016-06-16 at 14:17 -0600, Logan Gunthorpe wrote:
> When the link goes down, the link_is_up flag did not return to
> false. This could have caused some subtle corner case bugs
> when the link goes up and down quickly.
> 
> Once that was fixed, there was found to be a race if the link was
> brought down then immediately up. The link_cleanup work would
> occasionally be scheduled after the next link up event. This would
> cancel the link_work that was supposed to occur and leave ntb_perf
> in an unusable state.
> 
> To fix this we get rid of the link_cleanup work and put the actions
> directly in the link_down event.
> 
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>

Acked-by: Dave Jiang <dave.jiang@xxxxxxxxx>

Also patches 1-3 Acked.Â

> ---
> Âdrivers/ntb/test/ntb_perf.c | 28 +++++++++-------------------
> Â1 file changed, 9 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ntb/test/ntb_perf.c
> b/drivers/ntb/test/ntb_perf.c
> index f0784e5..6a50f20 100644
> --- a/drivers/ntb/test/ntb_perf.c
> +++ b/drivers/ntb/test/ntb_perf.c
> @@ -133,7 +133,6 @@ struct perf_ctx {
> Â	spinlock_t		db_lock;
> Â	struct perf_mw		mw;
> Â	bool			link_is_up;
> -	struct work_struct	link_cleanup;
> Â	struct delayed_work	link_work;
> Â	wait_queue_head_t	link_wq;
> Â	struct dentry		*debugfs_node_dir;
> @@ -158,10 +157,16 @@ static void perf_link_event(void *ctx)
> Â{
> Â	struct perf_ctx *perf = ctx;
> Â
> -	if (ntb_link_is_up(perf->ntb, NULL, NULL) == 1)
> +	if (ntb_link_is_up(perf->ntb, NULL, NULL) == 1) {
> Â		schedule_delayed_work(&perf->link_work, 2*HZ);
> -	else
> -		schedule_work(&perf->link_cleanup);
> +	} else {
> +		dev_dbg(&perf->ntb->pdev->dev, "link down\n");
> +
> +		if (!perf->link_is_up)
> +			cancel_delayed_work_sync(&perf->link_work);
> +
> +		perf->link_is_up = false;
> +	}
> Â}
> Â
> Âstatic void perf_db_event(void *ctx, int vec)
> @@ -547,18 +552,6 @@ out:
> Â				ÂÂÂÂÂÂmsecs_to_jiffies(PERF_LINK_DOW
> N_TIMEOUT));
> Â}
> Â
> -static void perf_link_cleanup(struct work_struct *work)
> -{
> -	struct perf_ctx *perf = container_of(work,
> -					ÂÂÂÂÂstruct perf_ctx,
> -					ÂÂÂÂÂlink_cleanup);
> -
> -	dev_dbg(&perf->ntb->pdev->dev, "%s called\n", __func__);
> -
> -	if (!perf->link_is_up)
> -		cancel_delayed_work_sync(&perf->link_work);
> -}
> -
> Âstatic int perf_setup_mw(struct ntb_dev *ntb, struct perf_ctx *perf)
> Â{
> Â	struct perf_mw *mw;
> @@ -787,7 +780,6 @@ static int perf_probe(struct ntb_client *client,
> struct ntb_dev *ntb)
> Â	perf_setup_mw(ntb, perf);
> Â	init_waitqueue_head(&perf->link_wq);
> Â	INIT_DELAYED_WORK(&perf->link_work, perf_link_work);
> -	INIT_WORK(&perf->link_cleanup, perf_link_cleanup);
> Â
> Â	rc = ntb_set_ctx(ntb, perf, &perf_ops);
> Â	if (rc)
> @@ -807,7 +799,6 @@ static int perf_probe(struct ntb_client *client,
> struct ntb_dev *ntb)
> Â
> Âerr_ctx:
> Â	cancel_delayed_work_sync(&perf->link_work);
> -	cancel_work_sync(&perf->link_cleanup);
> Â	kfree(perf);
> Âerr_perf:
> Â	return rc;
> @@ -823,7 +814,6 @@ static void perf_remove(struct ntb_client
> *client, struct ntb_dev *ntb)
> Â	mutex_lock(&perf->run_mutex);
> Â
> Â	cancel_delayed_work_sync(&perf->link_work);
> -	cancel_work_sync(&perf->link_cleanup);
> Â
> Â	ntb_clear_ctx(ntb);
> Â	ntb_link_disable(ntb);
> --Â
> 2.1.4
>