raw/ifpga: fix pthread cannot join
PATCH 1/1 raw/ifpga: fix pthread cannot join
From: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to thread handler function.
--- drivers/raw/ifpga/ifpga_rawdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/raw/ifpga/ifpga_rawdev.c +4 -4
@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];
@@ -544,7 +544,9 @@ static int set_surprise_link_check_aer( {int ret; -if (ifpga_monitor_start ==1 ) { +if (ifpga_monitor_start ==1 && ifpga_monitor_start_thread) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't cancel the thread" );
@@ -553,8 +555,6 @@ static int set_surprise_link_check_aer(if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; -return ret; }
Signature · 1 line
1.8.3.1
Hi Wei,
Some ci/iol-intel-Functional issues, pls check. Thanks a lot.
Quoted 57 lines
-----Original Message-----
From: Huang, Wei <[email protected]>
Sent: Wednesday, January 19, 2022 13:54
To: [email protected]; Xu, Rosen <[email protected]>; Zhang, Qi Z <[email protected]>
Cc: [email protected]; Zhang, Tianfei <[email protected]>; Yigit, Ferruh <[email protected]>
Subject: [PATCH v1] raw/ifpga: fix pthread cannot joinFrom: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to thread handler function.
Fixes 9c006c45 ("raw/ifpga: scan PCIe BDF device tree")Signed-off-by Tianfei Zhang <[email protected]>--- drivers/raw/ifpga/ifpga_rawdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/raw/ifpga/ifpga_rawdev.c +4
-2@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];@@ -544,7 +544,9 @@ static int set_surprise_link_check_aer( {int ret; -if (ifpga_monitor_start ==1 ) { +if (ifpga_monitor_start ==1 && ifpga_monitor_start_thread) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret) IFPGA_RAWDEV_PMD_ERR("Can't cancel thethread"); @@ -553,8 +555,6 @@ static int set_surprise_link_check_aer(
drivers/raw/ifpga/ifpga_rawdev.c +0
-3if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; -return ret; } --1.8.3.1
PATCH v2 1/1 raw/ifpga: fix pthread cannot join
In reply to Wei HuangFrom: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to notify thread handler function.
---
v2: update commit log
--- drivers/raw/ifpga/ifpga_rawdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/raw/ifpga/ifpga_rawdev.c +4 -4
@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];
@@ -544,7 +544,9 @@ static int set_surprise_link_check_aer( {int ret; -if (ifpga_monitor_start ==1 ) { +if ((ifpga_monitor_start ==1 ) && ifpga_monitor_start_thread) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't cancel the thread" );
@@ -553,8 +555,6 @@ static int set_surprise_link_check_aer(if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; -return ret; }
Signature · 1 line
1.8.3.1
PATCH v3 1/1 raw/ifpga: fix pthread cannot join
From: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to notify thread handler function.
---
v2: update commit log
---
v3: set thread id to 0 after pthread_join
--- drivers/raw/ifpga/ifpga_rawdev.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/raw/ifpga/ifpga_rawdev.c +7 -4
@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];
@@ -542,18 +542,21 @@ static int set_surprise_link_check_aer(static int ifpga_monitor_stop_func (void ) { +void *status;int ret; -if (ifpga_monitor_start ==1 ) { +if ((ifpga_monitor_start ==1 ) && ifpga_monitor_start_thread) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't cancel the thread" ); - ret =pthread_join (ifpga_monitor_start_thread,NULL ); + ret =pthread_join (ifpga_monitor_start_thread, &status);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; + ifpga_monitor_start_thread =0 ;return ret; }
Signature · 1 line
1.8.3.1
Quoted 6 lines · Wei Huang
From: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to notify thread handler function.
Quoted 8 lines
Fixes 9c006c45 ("raw/ifpga: scan PCIe BDF device tree")Signed-off-by Tianfei Zhang <[email protected]>---
v2: update commit log
---
v3: set thread id to 0 afterpthread_join
Evaluating/setting a pthread_t variable is a bad idea: this type should be considered opaque.
Plus here, the ifpga_monitor_start variable already indicates whether ifpga_monitor_start_thread is valid.
Only checking for ifpga_monitor_start in ifpga_rawdev_gsd_handle loop should be enough.
Quoted 18 lines
--- drivers/raw/ifpga/ifpga_rawdev.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)drivers/raw/ifpga/ifpga_rawdev.c +1
-1@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];
drivers/raw/ifpga/ifpga_rawdev.c static int set_surprise_link_check_aer( · line 545
Quoted 4 lines
drivers/raw/ifpga/ifpga_rawdev.c +1
-0@@ -542,18 +542,21 @@ static int set_surprise_link_check_aer(static int ifpga_monitor_stop_func (void ) { +void *status;
This status variable is unused, no need for it.
Quoted 23 lines
drivers/raw/ifpga/ifpga_rawdev.c +5
-4int ret; -if (ifpga_monitor_start ==1 ) { +if ((ifpga_monitor_start ==1 ) && ifpga_monitor_start_thread) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't cancel the thread" ); - ret =pthread_join (ifpga_monitor_start_thread,NULL ); + ret =pthread_join (ifpga_monitor_start_thread, &status);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; + ifpga_monitor_start_thread =0 ;return ret; } --1.8.3.1
Signature · 1 line
David Marchand
drivers/raw/ifpga/ifpga_rawdev.c static int set_surprise_link_check_aer( · line 500
Quoted 10 lines · On Thu, 20 Jan 2022 00:58:14 -0500 Wei Huang <[email protected]> wrote:
drivers/raw/ifpga/ifpga_rawdev.c +1
-1@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) {
This will work on x86 CPU but not on CPU's with weak memory ordering like ARM.
You need to use atomic_builtin when accessing a flag shared between threads that is not being protected by a lock.
Quoted 23 lines
-----Original Message-----
From: Stephen Hemminger <[email protected]>
Sent: Thursday, January 20, 2022 11:47 PM
To: Huang, Wei <[email protected]>
Cc: [email protected]; Xu, Rosen <[email protected]>; Zhang, Qi Z <[email protected]>; [email protected]; Zhang, Tianfei <[email protected]>; Yigit, Ferruh <[email protected]>
Subject: Re: [PATCH v3] raw/ifpga: fix pthread cannot joinQuoted 9 lines · On Thu, 20 Jan 2022 00:58:14 -0500 Wei Huang <[email protected]> wrote:
drivers/raw/ifpga/ifpga_rawdev.c +0
-0@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000
Quoted 8 lines
drivers/raw/ifpga/ifpga_rawdev.c static int set_surprise_link_check_aer( · line 500
Quoted 2 lines
drivers/raw/ifpga/ifpga_rawdev.c +1
-1-while (1 ) { +while (ifpga_monitor_start) {This will work on x86 CPU but not on CPU's with weak memory ordering like ARM.
You need to use
atomic_builtinwhen accessing a flag shared between threads that is not being protected by a lock.
We submitted a v5 patch for your suggestion, but got a " coding style issues" which said that:
Warning in drivers/raw/ifpga/ifpga_rawdev.c:
Using rte_atomicNN_xxx
We look at the devtools/checkpatches.sh script, it mentioned that rte_atomic APIs are forbidden.
ifpga_monitor_start just a flags to enable and disable the threads not share with threads. When the driver init it will start the thread, when the driver remove it want to stop the thread. I don't think it will has a concurrency issue on this scenario.
PATCH v4 1/1 raw/ifpga: fix pthread cannot join
In reply to Wei HuangFrom: Tianfei Zhang <[email protected]>
When we want to close a thread, we should set a flag to notify thread handler function.
---
v2: update commit log
---
v3: set thread id to 0 after pthread_join
---
v4: do not evaluate and set pthread_t variable
--- drivers/raw/ifpga/ifpga_rawdev.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/raw/ifpga/ifpga_rawdev.c +5 -4
@@ -497,7 +497,7 @@ static int set_surprise_link_check_aer(int gsd_enable, ret;#define MS 1000 -while (1 ) { +while (ifpga_monitor_start) { gsd_enable =0 ;for (i =0 ; i <IFPGA_RAWDEV_NUM ; i++) { ifpga_rdev = &ifpga_rawdevices[i];
@@ -542,19 +542,20 @@ static int set_surprise_link_check_aer(static int ifpga_monitor_stop_func (void ) { +void *status;int ret;if (ifpga_monitor_start ==1 ) { + ifpga_monitor_start =0 ; + ret =pthread_cancel (ifpga_monitor_start_thread);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't cancel the thread" ); - ret =pthread_join (ifpga_monitor_start_thread,NULL ); + ret =pthread_join (ifpga_monitor_start_thread, &status);if (ret)IFPGA_RAWDEV_PMD_ERR ("Can't join the thread" ); - ifpga_monitor_start =0 ; -return ret; }
Signature · 1 line
1.8.3.1