diff mbox series

[v3,16/22] net/ena: handle spurious wakeups in ENA_WAIT_EVENT

Message ID 20210506142526.28245-17-mk@semihalf.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers show
Series net/ena: update ENA PMD to v2.3.0 | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Michal Krawczyk May 6, 2021, 2:25 p.m. UTC
From: Stanislaw Kardach <kda@semihalf.com>

pthread_cond_timedwait() may spuriously wakeup according to POSIX.
Therefore it is required to check whether predicate is actually true
before finishing the waiting loop.

Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
Reviewed-by: Michal Krawczyk <mk@semihalf.com>
Reviewed-by: Igor Chauskin <igorch@amazon.com>
Reviewed-by: Shay Agroskin <shayagr@amazon.com>
---
 drivers/net/ena/base/ena_plat_dpdk.h | 75 +++++++++++++++++-----------
 1 file changed, 47 insertions(+), 28 deletions(-)

Comments

Ferruh Yigit May 7, 2021, 4:55 p.m. UTC | #1
On 5/6/2021 3:25 PM, Michal Krawczyk wrote:
> From: Stanislaw Kardach <kda@semihalf.com>
> 
> pthread_cond_timedwait() may spuriously wakeup according to POSIX.
> Therefore it is required to check whether predicate is actually true
> before finishing the waiting loop.
> 
> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
> Reviewed-by: Michal Krawczyk <mk@semihalf.com>
> Reviewed-by: Igor Chauskin <igorch@amazon.com>
> Reviewed-by: Shay Agroskin <shayagr@amazon.com>
> ---
>  drivers/net/ena/base/ena_plat_dpdk.h | 75 +++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
> index 4498d53703..1d0454bebe 100644
> --- a/drivers/net/ena/base/ena_plat_dpdk.h
> +++ b/drivers/net/ena/base/ena_plat_dpdk.h
> @@ -77,6 +77,14 @@ typedef uint64_t dma_addr_t;
>  #define mmiowb rte_io_wmb
>  #define __iomem
>  
> +#ifndef READ_ONCE
> +#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
> +#endif
> +
> +#define READ_ONCE8(var) READ_ONCE(var)
> +#define READ_ONCE16(var) READ_ONCE(var)
> +#define READ_ONCE32(var) READ_ONCE(var)
> +
>  #define US_PER_S 1000000
>  #define ENA_GET_SYSTEM_USECS()						       \
>  	(rte_get_timer_cycles() * US_PER_S / rte_get_timer_hz())
> @@ -137,40 +145,59 @@ extern int ena_logtype_com;
>  	({(void)flags; rte_spinlock_unlock(&(spinlock)); })
>  #define ENA_SPINLOCK_DESTROY(spinlock) ((void)spinlock)
>  
> -#define q_waitqueue_t							       \
> -	struct {							       \
> -		pthread_cond_t cond;					       \
> -		pthread_mutex_t mutex;					       \
> -	}
> +typedef struct {
> +	pthread_cond_t cond;
> +	pthread_mutex_t mutex;
> +	uint8_t flag;
> +} ena_wait_event_t;
>  
> -#define ena_wait_queue_t q_waitqueue_t
> -
> -#define ENA_WAIT_EVENT_INIT(waitqueue)					       \
> +#define ENA_WAIT_EVENT_INIT(waitevent)					       \
>  	do {								       \
> -		pthread_mutex_init(&(waitqueue).mutex, NULL);		       \
> -		pthread_cond_init(&(waitqueue).cond, NULL);		       \
> +		ena_wait_event_t *_we = &(waitevent);			       \
> +		pthread_mutex_init(&_we->mutex, NULL);			       \
> +		pthread_cond_init(&_we->cond, NULL);			       \
> +		_we->flag = 0;						       \
>  	} while (0)
>  
>  #define ENA_WAIT_EVENT_WAIT(waitevent, timeout)				       \
>  	do {								       \
> +		ena_wait_event_t *_we = &(waitevent);			       \
> +		typeof(timeout) _tmo = (timeout);			       \
> +		int ret = 0;						       \
>  		struct timespec wait;					       \
>  		struct timeval now;					       \
>  		unsigned long timeout_us;				       \
>  		gettimeofday(&now, NULL);				       \
> -		wait.tv_sec = now.tv_sec + timeout / 1000000UL;		       \
> -		timeout_us = timeout % 1000000UL;			       \
> +		wait.tv_sec = now.tv_sec + _tmo / 1000000UL;		       \
> +		timeout_us = _tmo % 1000000UL;				       \
>  		wait.tv_nsec = (now.tv_usec + timeout_us) * 1000UL;	       \
> -		pthread_mutex_lock(&waitevent.mutex);			       \
> -		pthread_cond_timedwait(&waitevent.cond,			       \
> -				&waitevent.mutex, &wait);		       \
> -		pthread_mutex_unlock(&waitevent.mutex);			       \
> +		pthread_mutex_lock(&_we->mutex);			       \
> +		while (ret == 0 && !_we->flag) {			       \
> +			ret = pthread_cond_timedwait(&_we->cond,	       \
> +				&_we->mutex, &wait);			       \
> +		}							       \
> +		/* Asserts only if not working on ena_wait_event_t */	       \
> +		if (unlikely(ret != 0 && ret != ETIMEDOUT))		       \
> +			rte_panic("Invalid wait event. pthread ret: %d\n",     \
> +				  ret);		

Drivers are not allowed to call 'rte_panic', please remove it.

Application should decide to exit or not. Maybe application is OK to run without
'ena' device if it has multiple other devices.
			       \
> +		else if (unlikely(ret == ETIMEDOUT))			       \
> +			ena_trc_err(NULL,				       \
> +				"Timeout waiting for " #waitevent "\n");       \
> +		_we->flag = 0;						       \
> +		pthread_mutex_unlock(&_we->mutex);			       \
> +	} while (0)
> +#define ENA_WAIT_EVENT_SIGNAL(waitevent)				       \
> +	do {								       \
> +		ena_wait_event_t *_we = &(waitevent);			       \
> +		pthread_mutex_lock(&_we->mutex);			       \
> +		_we->flag = 1;						       \
> +		pthread_cond_signal(&_we->cond);			       \
> +		pthread_mutex_unlock(&_we->mutex);			       \
>  	} while (0)
> -#define ENA_WAIT_EVENT_SIGNAL(waitevent) pthread_cond_signal(&waitevent.cond)
>  /* pthread condition doesn't need to be rearmed after usage */
>  #define ENA_WAIT_EVENT_CLEAR(...)
> -#define ENA_WAIT_EVENT_DESTROY(admin_queue) ((void)(admin_queue))
> +#define ENA_WAIT_EVENT_DESTROY(waitevent) ((void)(waitevent))
>  
> -#define ena_wait_event_t ena_wait_queue_t
>  #define ENA_MIGHT_SLEEP()
>  
>  #define ena_time_t uint64_t
> @@ -284,15 +311,7 @@ extern rte_atomic64_t ena_alloc_cnt;
>  #define ENA_TIME_EXPIRE(timeout)  (timeout < rte_get_timer_cycles())
>  #define ENA_GET_SYSTEM_TIMEOUT(timeout_us)				       \
>      (timeout_us * rte_get_timer_hz() / 1000000 + rte_get_timer_cycles())
> -#define ENA_WAIT_EVENTS_DESTROY(waitqueue) ((void)(waitqueue))
> -
> -#ifndef READ_ONCE
> -#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
> -#endif
> -
> -#define READ_ONCE8(var) READ_ONCE(var)
> -#define READ_ONCE16(var) READ_ONCE(var)
> -#define READ_ONCE32(var) READ_ONCE(var)
> +#define ENA_WAIT_EVENTS_DESTROY(admin_queue) ((void)(admin_queue))
>  
>  /* The size must be 8 byte align */
>  #define ENA_MEMCPY_TO_DEVICE_64(dst, src, size)				       \
>
diff mbox series

Patch

diff --git a/drivers/net/ena/base/ena_plat_dpdk.h b/drivers/net/ena/base/ena_plat_dpdk.h
index 4498d53703..1d0454bebe 100644
--- a/drivers/net/ena/base/ena_plat_dpdk.h
+++ b/drivers/net/ena/base/ena_plat_dpdk.h
@@ -77,6 +77,14 @@  typedef uint64_t dma_addr_t;
 #define mmiowb rte_io_wmb
 #define __iomem
 
+#ifndef READ_ONCE
+#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
+#endif
+
+#define READ_ONCE8(var) READ_ONCE(var)
+#define READ_ONCE16(var) READ_ONCE(var)
+#define READ_ONCE32(var) READ_ONCE(var)
+
 #define US_PER_S 1000000
 #define ENA_GET_SYSTEM_USECS()						       \
 	(rte_get_timer_cycles() * US_PER_S / rte_get_timer_hz())
@@ -137,40 +145,59 @@  extern int ena_logtype_com;
 	({(void)flags; rte_spinlock_unlock(&(spinlock)); })
 #define ENA_SPINLOCK_DESTROY(spinlock) ((void)spinlock)
 
-#define q_waitqueue_t							       \
-	struct {							       \
-		pthread_cond_t cond;					       \
-		pthread_mutex_t mutex;					       \
-	}
+typedef struct {
+	pthread_cond_t cond;
+	pthread_mutex_t mutex;
+	uint8_t flag;
+} ena_wait_event_t;
 
-#define ena_wait_queue_t q_waitqueue_t
-
-#define ENA_WAIT_EVENT_INIT(waitqueue)					       \
+#define ENA_WAIT_EVENT_INIT(waitevent)					       \
 	do {								       \
-		pthread_mutex_init(&(waitqueue).mutex, NULL);		       \
-		pthread_cond_init(&(waitqueue).cond, NULL);		       \
+		ena_wait_event_t *_we = &(waitevent);			       \
+		pthread_mutex_init(&_we->mutex, NULL);			       \
+		pthread_cond_init(&_we->cond, NULL);			       \
+		_we->flag = 0;						       \
 	} while (0)
 
 #define ENA_WAIT_EVENT_WAIT(waitevent, timeout)				       \
 	do {								       \
+		ena_wait_event_t *_we = &(waitevent);			       \
+		typeof(timeout) _tmo = (timeout);			       \
+		int ret = 0;						       \
 		struct timespec wait;					       \
 		struct timeval now;					       \
 		unsigned long timeout_us;				       \
 		gettimeofday(&now, NULL);				       \
-		wait.tv_sec = now.tv_sec + timeout / 1000000UL;		       \
-		timeout_us = timeout % 1000000UL;			       \
+		wait.tv_sec = now.tv_sec + _tmo / 1000000UL;		       \
+		timeout_us = _tmo % 1000000UL;				       \
 		wait.tv_nsec = (now.tv_usec + timeout_us) * 1000UL;	       \
-		pthread_mutex_lock(&waitevent.mutex);			       \
-		pthread_cond_timedwait(&waitevent.cond,			       \
-				&waitevent.mutex, &wait);		       \
-		pthread_mutex_unlock(&waitevent.mutex);			       \
+		pthread_mutex_lock(&_we->mutex);			       \
+		while (ret == 0 && !_we->flag) {			       \
+			ret = pthread_cond_timedwait(&_we->cond,	       \
+				&_we->mutex, &wait);			       \
+		}							       \
+		/* Asserts only if not working on ena_wait_event_t */	       \
+		if (unlikely(ret != 0 && ret != ETIMEDOUT))		       \
+			rte_panic("Invalid wait event. pthread ret: %d\n",     \
+				  ret);					       \
+		else if (unlikely(ret == ETIMEDOUT))			       \
+			ena_trc_err(NULL,				       \
+				"Timeout waiting for " #waitevent "\n");       \
+		_we->flag = 0;						       \
+		pthread_mutex_unlock(&_we->mutex);			       \
+	} while (0)
+#define ENA_WAIT_EVENT_SIGNAL(waitevent)				       \
+	do {								       \
+		ena_wait_event_t *_we = &(waitevent);			       \
+		pthread_mutex_lock(&_we->mutex);			       \
+		_we->flag = 1;						       \
+		pthread_cond_signal(&_we->cond);			       \
+		pthread_mutex_unlock(&_we->mutex);			       \
 	} while (0)
-#define ENA_WAIT_EVENT_SIGNAL(waitevent) pthread_cond_signal(&waitevent.cond)
 /* pthread condition doesn't need to be rearmed after usage */
 #define ENA_WAIT_EVENT_CLEAR(...)
-#define ENA_WAIT_EVENT_DESTROY(admin_queue) ((void)(admin_queue))
+#define ENA_WAIT_EVENT_DESTROY(waitevent) ((void)(waitevent))
 
-#define ena_wait_event_t ena_wait_queue_t
 #define ENA_MIGHT_SLEEP()
 
 #define ena_time_t uint64_t
@@ -284,15 +311,7 @@  extern rte_atomic64_t ena_alloc_cnt;
 #define ENA_TIME_EXPIRE(timeout)  (timeout < rte_get_timer_cycles())
 #define ENA_GET_SYSTEM_TIMEOUT(timeout_us)				       \
     (timeout_us * rte_get_timer_hz() / 1000000 + rte_get_timer_cycles())
-#define ENA_WAIT_EVENTS_DESTROY(waitqueue) ((void)(waitqueue))
-
-#ifndef READ_ONCE
-#define READ_ONCE(var) (*((volatile typeof(var) *)(&(var))))
-#endif
-
-#define READ_ONCE8(var) READ_ONCE(var)
-#define READ_ONCE16(var) READ_ONCE(var)
-#define READ_ONCE32(var) READ_ONCE(var)
+#define ENA_WAIT_EVENTS_DESTROY(admin_queue) ((void)(admin_queue))
 
 /* The size must be 8 byte align */
 #define ENA_MEMCPY_TO_DEVICE_64(dst, src, size)				       \