All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/3] qed: Fix and enhance slowpath workqueue mechanism.
@ 2020-03-24 14:13 Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag Yuval Basson
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Yuval Basson @ 2020-03-24 14:13 UTC (permalink / raw)
  To: davem, netdev

The next patch series enhances slowpath workqueue mechanism and fixes
a race between scheduled tasks and destroy_workqueue.

Yuval Basson (3):
  qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag
  qed: Add a flag for rescheduling the slowpath task
  qed: Fix race condition between scheduling and destroying the slowpath
    workqueue

 drivers/net/ethernet/qlogic/qed/qed.h      |  3 ++-
 drivers/net/ethernet/qlogic/qed/qed_main.c | 37 +++++++++++++-----------------
 2 files changed, 18 insertions(+), 22 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag
  2020-03-24 14:13 [PATCH net-next 0/3] qed: Fix and enhance slowpath workqueue mechanism Yuval Basson
@ 2020-03-24 14:13 ` Yuval Basson
  2020-03-24 23:36   ` David Miller
  2020-03-24 14:13 ` [PATCH net-next 2/3] qed: Add a flag for rescheduling the slowpath task Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 3/3] qed: Fix race condition between scheduling and destroying the slowpath workqueue Yuval Basson
  2 siblings, 1 reply; 6+ messages in thread
From: Yuval Basson @ 2020-03-24 14:13 UTC (permalink / raw)
  To: davem, netdev; +Cc: Yuval Basson, Denis Bolotin

The atomic opertaion might prevent a potential race condition.

Signed-off-by: Yuval Basson <ybason@marvell.com>
Signed-off-by: Denis Bolotin <dbolotin@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed.h      | 2 +-
 drivers/net/ethernet/qlogic/qed/qed_main.c | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index fa41bf0..ca866c2 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -565,6 +565,7 @@ struct qed_simd_fp_handler {
 enum qed_slowpath_wq_flag {
 	QED_SLOWPATH_MFW_TLV_REQ,
 	QED_SLOWPATH_PERIODIC_DB_REC,
+	QED_SLOWPATH_ACTIVE,
 };
 
 struct qed_hwfn {
@@ -700,7 +701,6 @@ struct qed_hwfn {
 	unsigned long iov_task_flags;
 #endif
 	struct z_stream_s *stream;
-	bool slowpath_wq_active;
 	struct workqueue_struct *slowpath_wq;
 	struct delayed_work slowpath_task;
 	unsigned long slowpath_task_flags;
diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 2c189c6..016d658 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1095,7 +1095,7 @@ static int qed_slowpath_delayed_work(struct qed_hwfn *hwfn,
 				     enum qed_slowpath_wq_flag wq_flag,
 				     unsigned long delay)
 {
-	if (!hwfn->slowpath_wq_active)
+	if (!test_bit(QED_SLOWPATH_ACTIVE, &hwfn->slowpath_task_flags))
 		return -EINVAL;
 
 	/* Memory barrier for setting atomic bit */
@@ -1133,7 +1133,8 @@ static void qed_slowpath_wq_stop(struct qed_dev *cdev)
 			continue;
 
 		/* Stop queuing new delayed works */
-		cdev->hwfns[i].slowpath_wq_active = false;
+		clear_bit(QED_SLOWPATH_ACTIVE,
+			  &cdev->hwfns[i].slowpath_task_flags);
 
 		/* Wait until the last periodic doorbell recovery is executed */
 		while (test_bit(QED_SLOWPATH_PERIODIC_DB_REC,
@@ -1153,7 +1154,7 @@ static void qed_slowpath_task(struct work_struct *work)
 	struct qed_ptt *ptt = qed_ptt_acquire(hwfn);
 
 	if (!ptt) {
-		if (hwfn->slowpath_wq_active)
+		if (test_bit(QED_SLOWPATH_ACTIVE, &hwfn->slowpath_task_flags))
 			queue_delayed_work(hwfn->slowpath_wq,
 					   &hwfn->slowpath_task, 0);
 
@@ -1199,7 +1200,7 @@ static int qed_slowpath_wq_start(struct qed_dev *cdev)
 		}
 
 		INIT_DELAYED_WORK(&hwfn->slowpath_task, qed_slowpath_task);
-		hwfn->slowpath_wq_active = true;
+		set_bit(QED_SLOWPATH_ACTIVE, &hwfn->slowpath_task_flags);
 	}
 
 	return 0;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net-next 2/3] qed: Add a flag for rescheduling the slowpath task
  2020-03-24 14:13 [PATCH net-next 0/3] qed: Fix and enhance slowpath workqueue mechanism Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag Yuval Basson
@ 2020-03-24 14:13 ` Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 3/3] qed: Fix race condition between scheduling and destroying the slowpath workqueue Yuval Basson
  2 siblings, 0 replies; 6+ messages in thread
From: Yuval Basson @ 2020-03-24 14:13 UTC (permalink / raw)
  To: davem, netdev; +Cc: Yuval Basson, Denis Bolotin

Rechedule delayed work in case ptt_acquire failed.
Since it is the same task don't reset task's flags.

Signed-off-by: Yuval Basson <ybason@marvell.com>
Signed-off-by: Denis Bolotin <dbolotin@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed.h      |  1 +
 drivers/net/ethernet/qlogic/qed/qed_main.c | 15 +++++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed.h b/drivers/net/ethernet/qlogic/qed/qed.h
index ca866c2..e3b238e 100644
--- a/drivers/net/ethernet/qlogic/qed/qed.h
+++ b/drivers/net/ethernet/qlogic/qed/qed.h
@@ -566,6 +566,7 @@ enum qed_slowpath_wq_flag {
 	QED_SLOWPATH_MFW_TLV_REQ,
 	QED_SLOWPATH_PERIODIC_DB_REC,
 	QED_SLOWPATH_ACTIVE,
+	QED_SLOWPATH_RESCHEDULE,
 };
 
 struct qed_hwfn {
diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index 016d658..fb13863 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1098,10 +1098,13 @@ static int qed_slowpath_delayed_work(struct qed_hwfn *hwfn,
 	if (!test_bit(QED_SLOWPATH_ACTIVE, &hwfn->slowpath_task_flags))
 		return -EINVAL;
 
-	/* Memory barrier for setting atomic bit */
-	smp_mb__before_atomic();
-	set_bit(wq_flag, &hwfn->slowpath_task_flags);
-	smp_mb__after_atomic();
+	if (wq_flag != QED_SLOWPATH_RESCHEDULE) {
+		/* Memory barrier for setting atomic bit */
+		smp_mb__before_atomic();
+		set_bit(wq_flag, &hwfn->slowpath_task_flags);
+		smp_mb__after_atomic();
+	}
+
 	queue_delayed_work(hwfn->slowpath_wq, &hwfn->slowpath_task, delay);
 
 	return 0;
@@ -1155,8 +1158,8 @@ static void qed_slowpath_task(struct work_struct *work)
 
 	if (!ptt) {
 		if (test_bit(QED_SLOWPATH_ACTIVE, &hwfn->slowpath_task_flags))
-			queue_delayed_work(hwfn->slowpath_wq,
-					   &hwfn->slowpath_task, 0);
+			qed_slowpath_delayed_work(hwfn,
+						  QED_SLOWPATH_RESCHEDULE, 0);
 
 		return;
 	}
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net-next 3/3] qed: Fix race condition between scheduling and destroying the slowpath workqueue
  2020-03-24 14:13 [PATCH net-next 0/3] qed: Fix and enhance slowpath workqueue mechanism Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag Yuval Basson
  2020-03-24 14:13 ` [PATCH net-next 2/3] qed: Add a flag for rescheduling the slowpath task Yuval Basson
@ 2020-03-24 14:13 ` Yuval Basson
  2 siblings, 0 replies; 6+ messages in thread
From: Yuval Basson @ 2020-03-24 14:13 UTC (permalink / raw)
  To: davem, netdev; +Cc: Yuval Basson, Denis Bolotin

Calling queue_delayed_work concurrently with
destroy_workqueue might race to an unexpected outcome -
scheduled task after wq is destroyed or other resources
(like ptt_pool) are freed (yields NULL pointer dereference).
cancel_delayed_work prevents the race by cancelling
the timer triggered for scheduling a new task.

Signed-off-by: Yuval Basson <ybason@marvell.com>
Signed-off-by: Denis Bolotin <dbolotin@marvell.com>
---
 drivers/net/ethernet/qlogic/qed/qed_main.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_main.c b/drivers/net/ethernet/qlogic/qed/qed_main.c
index fb13863..ade927d 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_main.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_main.c
@@ -1087,9 +1087,6 @@ static void qed_update_pf_params(struct qed_dev *cdev,
 #define QED_PERIODIC_DB_REC_INTERVAL_MS		100
 #define QED_PERIODIC_DB_REC_INTERVAL \
 	msecs_to_jiffies(QED_PERIODIC_DB_REC_INTERVAL_MS)
-#define QED_PERIODIC_DB_REC_WAIT_COUNT		10
-#define QED_PERIODIC_DB_REC_WAIT_INTERVAL \
-	(QED_PERIODIC_DB_REC_INTERVAL_MS / QED_PERIODIC_DB_REC_WAIT_COUNT)
 
 static int qed_slowpath_delayed_work(struct qed_hwfn *hwfn,
 				     enum qed_slowpath_wq_flag wq_flag,
@@ -1126,7 +1123,7 @@ void qed_periodic_db_rec_start(struct qed_hwfn *p_hwfn)
 
 static void qed_slowpath_wq_stop(struct qed_dev *cdev)
 {
-	int i, sleep_count = QED_PERIODIC_DB_REC_WAIT_COUNT;
+	int i;
 
 	if (IS_VF(cdev))
 		return;
@@ -1139,13 +1136,7 @@ static void qed_slowpath_wq_stop(struct qed_dev *cdev)
 		clear_bit(QED_SLOWPATH_ACTIVE,
 			  &cdev->hwfns[i].slowpath_task_flags);
 
-		/* Wait until the last periodic doorbell recovery is executed */
-		while (test_bit(QED_SLOWPATH_PERIODIC_DB_REC,
-				&cdev->hwfns[i].slowpath_task_flags) &&
-		       sleep_count--)
-			msleep(QED_PERIODIC_DB_REC_WAIT_INTERVAL);
-
-		flush_workqueue(cdev->hwfns[i].slowpath_wq);
+		cancel_delayed_work(&cdev->hwfns[i].slowpath_task);
 		destroy_workqueue(cdev->hwfns[i].slowpath_wq);
 	}
 }
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag
  2020-03-24 14:13 ` [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag Yuval Basson
@ 2020-03-24 23:36   ` David Miller
  2020-03-25 20:35     ` [EXT] " Yuval Basson
  0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2020-03-24 23:36 UTC (permalink / raw)
  To: ybason; +Cc: netdev, dbolotin

From: Yuval Basson <ybason@marvell.com>
Date: Tue, 24 Mar 2020 16:13:46 +0200

> The atomic opertaion might prevent a potential race condition.
> 
> Signed-off-by: Yuval Basson <ybason@marvell.com>
> Signed-off-by: Denis Bolotin <dbolotin@marvell.com>

There is no basis in fact behind this change.

Either explain clearly and precisely what race is fixed by this
change or remove this change.

Thank you.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [EXT] Re: [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag
  2020-03-24 23:36   ` David Miller
@ 2020-03-25 20:35     ` Yuval Basson
  0 siblings, 0 replies; 6+ messages in thread
From: Yuval Basson @ 2020-03-25 20:35 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Denis Bolotin



> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Wednesday, March 25, 2020 1:36 AM
> To: Yuval Basson <ybason@marvell.com>
> Cc: netdev@vger.kernel.org; Denis Bolotin <dbolotin@marvell.com>
> Subject: [EXT] Re: [PATCH net-next 1/3] qed: Replace wq_active Boolean
> with an atomic QED_SLOWPATH_ACTIVE flag
> 
> External Email
> 
> ----------------------------------------------------------------------
> From: Yuval Basson <ybason@marvell.com>
> Date: Tue, 24 Mar 2020 16:13:46 +0200
> 
> > The atomic opertaion might prevent a potential race condition.
> >
> > Signed-off-by: Yuval Basson <ybason@marvell.com>
> > Signed-off-by: Denis Bolotin <dbolotin@marvell.com>
> 
> There is no basis in fact behind this change.
> > thanks for the comment, indeed it seems the only patch required to solve the race is the third one
>> Sending a v2 with just the third patch.
> Either explain clearly and precisely what race is fixed by this change or
> remove this change.
> 
> Thank you.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-03-25 20:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-24 14:13 [PATCH net-next 0/3] qed: Fix and enhance slowpath workqueue mechanism Yuval Basson
2020-03-24 14:13 ` [PATCH net-next 1/3] qed: Replace wq_active Boolean with an atomic QED_SLOWPATH_ACTIVE flag Yuval Basson
2020-03-24 23:36   ` David Miller
2020-03-25 20:35     ` [EXT] " Yuval Basson
2020-03-24 14:13 ` [PATCH net-next 2/3] qed: Add a flag for rescheduling the slowpath task Yuval Basson
2020-03-24 14:13 ` [PATCH net-next 3/3] qed: Fix race condition between scheduling and destroying the slowpath workqueue Yuval Basson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.