From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2980FA0524; Fri, 7 May 2021 18:55:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DFA224013F; Fri, 7 May 2021 18:55:21 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mails.dpdk.org (Postfix) with ESMTP id 8BA5040040 for ; Fri, 7 May 2021 18:55:20 +0200 (CEST) IronPort-SDR: D+qw3BIHlb29k0Ah7M952PaM22W/gRQC+QIAt4/XAXtMp1acYHca9nJoGIp9EY3WB/nX7OqzdW 6U3/kjq7Albw== X-IronPort-AV: E=McAfee;i="6200,9189,9977"; a="198419516" X-IronPort-AV: E=Sophos;i="5.82,281,1613462400"; d="scan'208";a="198419516" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2021 09:55:11 -0700 IronPort-SDR: MCbVbeFo2I/83pweTpnPSd95I41PZslfxK9HGEptvU1hH8mlCQd3lXVtH0kfvPwDZslfCupnje ansgC3QcnbBA== X-IronPort-AV: E=Sophos;i="5.82,281,1613462400"; d="scan'208";a="431444952" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.210.230]) ([10.213.210.230]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2021 09:55:10 -0700 To: Michal Krawczyk , dev@dpdk.org Cc: ndagan@amazon.com, gtzalik@amazon.com, igorch@amazon.com, upstream@semihalf.com, Stanislaw Kardach , Shay Agroskin References: <20210505073348.6394-1-mk@semihalf.com> <20210506142526.28245-1-mk@semihalf.com> <20210506142526.28245-17-mk@semihalf.com> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Fri, 7 May 2021 17:55:06 +0100 MIME-Version: 1.0 In-Reply-To: <20210506142526.28245-17-mk@semihalf.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 16/22] net/ena: handle spurious wakeups in ENA_WAIT_EVENT X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 5/6/2021 3:25 PM, Michal Krawczyk wrote: > From: Stanislaw Kardach > > 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 > Reviewed-by: Michal Krawczyk > Reviewed-by: Igor Chauskin > Reviewed-by: Shay Agroskin > --- > 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) \ >