From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6F809A2F6B for ; Tue, 8 Oct 2019 19:05:48 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 01FC11C044; Tue, 8 Oct 2019 19:05:46 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 2A4931C043; Tue, 8 Oct 2019 19:05:45 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 53E2E883C44; Tue, 8 Oct 2019 17:05:44 +0000 (UTC) Received: from dhcp-25.97.bos.redhat.com (unknown [10.18.25.121]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4800610013A1; Tue, 8 Oct 2019 17:05:43 +0000 (UTC) From: Aaron Conole To: Ruifeng Wang Cc: david.hunt@intel.com, dev@dpdk.org, hkalra@marvell.com, gavin.hu@arm.com, honnappa.nagarahalli@arm.com, nd@arm.com, stable@dpdk.org References: <20191008095524.1585-1-ruifeng.wang@arm.com> Date: Tue, 08 Oct 2019 13:05:42 -0400 In-Reply-To: <20191008095524.1585-1-ruifeng.wang@arm.com> (Ruifeng Wang's message of "Tue, 8 Oct 2019 17:55:24 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.69]); Tue, 08 Oct 2019 17:05:44 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH] lib/distributor: fix deadlock issue for aarch64 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Ruifeng Wang writes: > Distributor and worker threads rely on data structs in cache line > for synchronization. The shared data structs were not protected. > This caused deadlock issue on weaker memory ordering platforms as > aarch64. > Fix this issue by adding memory barriers to ensure synchronization > among cores. > > Bugzilla ID: 342 > Fixes: 775003ad2f96 ("distributor: add new burst-capable library") > Cc: stable@dpdk.org > > Signed-off-by: Ruifeng Wang > Reviewed-by: Gavin Hu > --- I see a failure in the distributor_autotest (on one of the builds): 64/82 DPDK:fast-tests / distributor_autotest FAIL 0.37 s (exit status 255 or signal 127 SIGinvalid) --- command --- DPDK_TEST='distributor_autotest' /home/travis/build/ovsrobot/dpdk/build/app/test/dpdk-test -l 0-1 --file-prefix=distributor_autotest --- stdout --- EAL: Probing VFIO support... APP: HPET is not enabled, using TSC as default timer RTE>>distributor_autotest === Basic distributor sanity tests === Worker 0 handled 32 packets Sanity test with all zero hashes done. Worker 0 handled 32 packets Sanity test with non-zero hashes done === testing big burst (single) === Sanity test of returned packets done === Sanity test with mbuf alloc/free (single) === Sanity test with mbuf alloc/free passed Too few cores to run worker shutdown test === Basic distributor sanity tests === Worker 0 handled 32 packets Sanity test with all zero hashes done. Worker 0 handled 32 packets Sanity test with non-zero hashes done === testing big burst (burst) === Sanity test of returned packets done === Sanity test with mbuf alloc/free (burst) === Line 326: Packet count is incorrect, 1048568, expected 1048576 Test Failed RTE>> --- stderr --- EAL: Detected 2 lcore(s) EAL: Detected 1 NUMA nodes EAL: Multi-process socket /var/run/dpdk/distributor_autotest/mp_socket EAL: Selected IOVA mode 'PA' EAL: No available hugepages reported in hugepages-1048576kB ------- Not sure how to help debug further. I'll re-start the job to see if it 'clears' up - but I guess there may be a delicate synchronization somewhere that needs to be accounted. > lib/librte_distributor/rte_distributor.c | 28 ++++++++++------ > lib/librte_distributor/rte_distributor_v20.c | 34 +++++++++++++------- > 2 files changed, 41 insertions(+), 21 deletions(-) > > diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c > index 21eb1fb0a..7bf96e224 100644 > --- a/lib/librte_distributor/rte_distributor.c > +++ b/lib/librte_distributor/rte_distributor.c > @@ -50,7 +50,8 @@ rte_distributor_request_pkt_v1705(struct rte_distributor *d, > > retptr64 = &(buf->retptr64[0]); > /* Spin while handshake bits are set (scheduler clears it) */ > - while (unlikely(*retptr64 & RTE_DISTRIB_GET_BUF)) { > + while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF)) { > rte_pause(); > uint64_t t = rte_rdtsc()+100; > > @@ -76,7 +77,8 @@ rte_distributor_request_pkt_v1705(struct rte_distributor *d, > * Finally, set the GET_BUF to signal to distributor that cache > * line is ready for processing > */ > - *retptr64 |= RTE_DISTRIB_GET_BUF; > + __atomic_store_n(retptr64, *retptr64 | RTE_DISTRIB_GET_BUF, > + __ATOMIC_RELEASE); > } > BIND_DEFAULT_SYMBOL(rte_distributor_request_pkt, _v1705, 17.05); > MAP_STATIC_SYMBOL(void rte_distributor_request_pkt(struct rte_distributor *d, > @@ -99,7 +101,8 @@ rte_distributor_poll_pkt_v1705(struct rte_distributor *d, > } > > /* If bit is set, return */ > - if (buf->bufptr64[0] & RTE_DISTRIB_GET_BUF) > + if (__atomic_load_n(&(buf->bufptr64[0]), __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF) > return -1; > > /* since bufptr64 is signed, this should be an arithmetic shift */ > @@ -116,6 +119,8 @@ rte_distributor_poll_pkt_v1705(struct rte_distributor *d, > * on the next cacheline while we're working. > */ > buf->bufptr64[0] |= RTE_DISTRIB_GET_BUF; > + __atomic_store_n(&(buf->bufptr64[0]), > + buf->bufptr64[0] | RTE_DISTRIB_GET_BUF, __ATOMIC_RELEASE); > > return count; > } > @@ -183,7 +188,8 @@ rte_distributor_return_pkt_v1705(struct rte_distributor *d, > RTE_DISTRIB_FLAG_BITS) | RTE_DISTRIB_RETURN_BUF; > > /* set the GET_BUF but even if we got no returns */ > - buf->retptr64[0] |= RTE_DISTRIB_GET_BUF; > + __atomic_store_n(&(buf->retptr64[0]), > + buf->retptr64[0] | RTE_DISTRIB_GET_BUF, __ATOMIC_RELEASE); > > return 0; > } > @@ -273,7 +279,8 @@ handle_returns(struct rte_distributor *d, unsigned int wkr) > unsigned int count = 0; > unsigned int i; > > - if (buf->retptr64[0] & RTE_DISTRIB_GET_BUF) { > + if (__atomic_load_n(&(buf->retptr64[0]), __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF) { > for (i = 0; i < RTE_DIST_BURST_SIZE; i++) { > if (buf->retptr64[i] & RTE_DISTRIB_RETURN_BUF) { > oldbuf = ((uintptr_t)(buf->retptr64[i] >> > @@ -287,7 +294,7 @@ handle_returns(struct rte_distributor *d, unsigned int wkr) > d->returns.start = ret_start; > d->returns.count = ret_count; > /* Clear for the worker to populate with more returns */ > - buf->retptr64[0] = 0; > + __atomic_store_n(&(buf->retptr64[0]), 0, __ATOMIC_RELEASE); > } > return count; > } > @@ -307,7 +314,8 @@ release(struct rte_distributor *d, unsigned int wkr) > struct rte_distributor_buffer *buf = &(d->bufs[wkr]); > unsigned int i; > > - while (!(d->bufs[wkr].bufptr64[0] & RTE_DISTRIB_GET_BUF)) > + while (!(__atomic_load_n(&(d->bufs[wkr].bufptr64[0]), __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF)) > rte_pause(); > > handle_returns(d, wkr); > @@ -328,7 +336,8 @@ release(struct rte_distributor *d, unsigned int wkr) > d->backlog[wkr].count = 0; > > /* Clear the GET bit */ > - buf->bufptr64[0] &= ~RTE_DISTRIB_GET_BUF; > + __atomic_store_n(&(buf->bufptr64[0]), > + buf->bufptr64[0] & ~RTE_DISTRIB_GET_BUF, __ATOMIC_RELEASE); > return buf->count; > > } > @@ -574,7 +583,8 @@ rte_distributor_clear_returns_v1705(struct rte_distributor *d) > > /* throw away returns, so workers can exit */ > for (wkr = 0; wkr < d->num_workers; wkr++) > - d->bufs[wkr].retptr64[0] = 0; > + __atomic_store_n(&(d->bufs[wkr].retptr64[0]), 0, > + __ATOMIC_RELEASE); > } > BIND_DEFAULT_SYMBOL(rte_distributor_clear_returns, _v1705, 17.05); > MAP_STATIC_SYMBOL(void rte_distributor_clear_returns(struct rte_distributor *d), > diff --git a/lib/librte_distributor/rte_distributor_v20.c b/lib/librte_distributor/rte_distributor_v20.c > index cdc0969a8..3a5810c6d 100644 > --- a/lib/librte_distributor/rte_distributor_v20.c > +++ b/lib/librte_distributor/rte_distributor_v20.c > @@ -34,9 +34,10 @@ rte_distributor_request_pkt_v20(struct rte_distributor_v20 *d, > union rte_distributor_buffer_v20 *buf = &d->bufs[worker_id]; > int64_t req = (((int64_t)(uintptr_t)oldpkt) << RTE_DISTRIB_FLAG_BITS) > | RTE_DISTRIB_GET_BUF; > - while (unlikely(buf->bufptr64 & RTE_DISTRIB_FLAGS_MASK)) > + while (unlikely(__atomic_load_n(&(buf->bufptr64), __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_FLAGS_MASK)) > rte_pause(); > - buf->bufptr64 = req; > + __atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE); > } > VERSION_SYMBOL(rte_distributor_request_pkt, _v20, 2.0); > > @@ -45,7 +46,8 @@ rte_distributor_poll_pkt_v20(struct rte_distributor_v20 *d, > unsigned worker_id) > { > union rte_distributor_buffer_v20 *buf = &d->bufs[worker_id]; > - if (buf->bufptr64 & RTE_DISTRIB_GET_BUF) > + if (__atomic_load_n(&(buf->bufptr64), __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF) > return NULL; > > /* since bufptr64 is signed, this should be an arithmetic shift */ > @@ -73,7 +75,7 @@ rte_distributor_return_pkt_v20(struct rte_distributor_v20 *d, > union rte_distributor_buffer_v20 *buf = &d->bufs[worker_id]; > uint64_t req = (((int64_t)(uintptr_t)oldpkt) << RTE_DISTRIB_FLAG_BITS) > | RTE_DISTRIB_RETURN_BUF; > - buf->bufptr64 = req; > + __atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE); > return 0; > } > VERSION_SYMBOL(rte_distributor_return_pkt, _v20, 2.0); > @@ -117,7 +119,7 @@ handle_worker_shutdown(struct rte_distributor_v20 *d, unsigned int wkr) > { > d->in_flight_tags[wkr] = 0; > d->in_flight_bitmask &= ~(1UL << wkr); > - d->bufs[wkr].bufptr64 = 0; > + __atomic_store_n(&(d->bufs[wkr].bufptr64), 0, __ATOMIC_RELEASE); > if (unlikely(d->backlog[wkr].count != 0)) { > /* On return of a packet, we need to move the > * queued packets for this core elsewhere. > @@ -165,13 +167,17 @@ process_returns(struct rte_distributor_v20 *d) > const int64_t data = d->bufs[wkr].bufptr64; > uintptr_t oldbuf = 0; > > - if (data & RTE_DISTRIB_GET_BUF) { > + if (__atomic_load_n(&data, __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF) { > flushed++; > if (d->backlog[wkr].count) > - d->bufs[wkr].bufptr64 = > - backlog_pop(&d->backlog[wkr]); > + __atomic_store_n(&(d->bufs[wkr].bufptr64), > + backlog_pop(&d->backlog[wkr]), > + __ATOMIC_RELEASE); > else { > - d->bufs[wkr].bufptr64 = RTE_DISTRIB_GET_BUF; > + __atomic_store_n(&(d->bufs[wkr].bufptr64), > + RTE_DISTRIB_GET_BUF, > + __ATOMIC_RELEASE); > d->in_flight_tags[wkr] = 0; > d->in_flight_bitmask &= ~(1UL << wkr); > } > @@ -251,7 +257,8 @@ rte_distributor_process_v20(struct rte_distributor_v20 *d, > } > } > > - if ((data & RTE_DISTRIB_GET_BUF) && > + if ((__atomic_load_n(&data, __ATOMIC_ACQUIRE) > + & RTE_DISTRIB_GET_BUF) && > (d->backlog[wkr].count || next_mb)) { > > if (d->backlog[wkr].count) > @@ -280,13 +287,16 @@ rte_distributor_process_v20(struct rte_distributor_v20 *d, > * if they are ready */ > for (wkr = 0; wkr < d->num_workers; wkr++) > if (d->backlog[wkr].count && > - (d->bufs[wkr].bufptr64 & RTE_DISTRIB_GET_BUF)) { > + (__atomic_load_n(&(d->bufs[wkr].bufptr64), > + __ATOMIC_ACQUIRE) & RTE_DISTRIB_GET_BUF)) { > > int64_t oldbuf = d->bufs[wkr].bufptr64 >> > RTE_DISTRIB_FLAG_BITS; > store_return(oldbuf, d, &ret_start, &ret_count); > > - d->bufs[wkr].bufptr64 = backlog_pop(&d->backlog[wkr]); > + __atomic_store_n(&(d->bufs[wkr].bufptr64), > + backlog_pop(&d->backlog[wkr]), > + __ATOMIC_RELEASE); > } > > d->returns.start = ret_start;