* [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* 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
* [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