From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754656AbcFPU5Y (ORCPT ); Thu, 16 Jun 2016 16:57:24 -0400 Received: from mga01.intel.com ([192.55.52.88]:28283 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752655AbcFPU5W (ORCPT ); Thu, 16 Jun 2016 16:57:22 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,481,1459839600"; d="scan'208";a="720293039" From: "Jiang, Dave" To: "Allen.Hubbe@emc.com" , "logang@deltatee.com" , "jdmason@kudzu.us" CC: "linux-kernel@vger.kernel.org" , "shuahkh@osg.samsung.com" , "sudipm.mukherjee@gmail.com" , "linux-kselftest@vger.kernel.org" , "arnd@arndb.de" , "linux-ntb@googlegroups.com" Subject: Re: [PATCH v4 10/10] ntb_perf: clear link_is_up flag when the link goes down. Thread-Topic: [PATCH v4 10/10] ntb_perf: clear link_is_up flag when the link goes down. Thread-Index: AQHRyAw3nEVpq6LoU0S6G2VQpOhUZJ/tCISA Date: Thu, 16 Jun 2016 20:57:20 +0000 Message-ID: <1466110623.16234.279.camel@intel.com> References: <53d495a5f97211289302ddb60d8508edeaab20ed.1466098407.git.logang@deltatee.com> In-Reply-To: <53d495a5f97211289302ddb60d8508edeaab20ed.1466098407.git.logang@deltatee.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [143.182.137.38] Content-Type: text/plain; charset="utf-8" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id u5GKvSxo004208 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 Acked-by: Dave Jiang 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 >