[dpdk-dev] [PATCH v7 01/16] distributor: fix missing handshake synchronization

Lukasz Wojciechowski l.wojciechow at partner.samsung.com
Sat Oct 17 05:13:53 CEST 2020


All suggested changes applied and published in v8.

W dniu 16.10.2020 o 01:47, Honnappa Nagarahalli pisze:
> <snip>
>
>> rte_distributor_return_pkt function which is run on worker cores must wait
>> for distributor core to clear handshake on retptr64 before using those
>> buffers. While the handshake is set distributor core controls buffers and any
>> operations on worker side might overwrite buffers which are unread yet.
>> Same situation appears in the legacy single distributor. Function
>> rte_distributor_return_pkt_single shouldn't modify the bufptr64 until
>> handshake on it is cleared by distributor lcore.
>>
>> Fixes: 775003ad2f96 ("distributor: add new burst-capable library")
>> Cc: david.hunt at intel.com
>> Cc: stable at dpdk.org
>>
>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow at partner.samsung.com>
>> Acked-by: David Hunt <david.hunt at intel.com>
>> ---
>>   lib/librte_distributor/rte_distributor.c        | 14 ++++++++++++++
>>   lib/librte_distributor/rte_distributor_single.c |  4 ++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/lib/librte_distributor/rte_distributor.c
>> b/lib/librte_distributor/rte_distributor.c
>> index 1c047f065..89493c331 100644
>> --- a/lib/librte_distributor/rte_distributor.c
>> +++ b/lib/librte_distributor/rte_distributor.c
>> @@ -160,6 +160,7 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>> {
>>   	struct rte_distributor_buffer *buf = &d->bufs[worker_id];
>>   	unsigned int i;
>> +	volatile int64_t *retptr64;
> volatile is not needed here as use of __atomic_load_n implies volatile inherently.
retptr64 variable removed at all
>>   	if (unlikely(d->alg_type == RTE_DIST_ALG_SINGLE)) {
>>   		if (num == 1)
>> @@ -169,6 +170,19 @@ rte_distributor_return_pkt(struct rte_distributor *d,
>>   			return -EINVAL;
>>   	}
>>
>> +	retptr64 = &(buf->retptr64[0]);
>> +	/* Spin while handshake bits are set (scheduler clears it).
>> +	 * Sync with worker on GET_BUF flag.
>> +	 */
>> +	while (unlikely(__atomic_load_n(retptr64, __ATOMIC_ACQUIRE)
> nit. we could avoid using the temp variable retptr64, you could use '&buf->retptr64[0]' directly.
> RELAXED memory order should be good as the thread_fence below will ensure that this load does not sink.
retptr64 variable removed and relaxed memory order used
>
> [1]
>> +			& RTE_DISTRIB_GET_BUF)) {
>> +		rte_pause();
>> +		uint64_t t = rte_rdtsc()+100;
>> +
>> +		while (rte_rdtsc() < t)
>> +			rte_pause();
>> +	}
>> +
>>   	/* Sync with distributor to acquire retptrs */
>>   	__atomic_thread_fence(__ATOMIC_ACQUIRE);
>>   	for (i = 0; i < RTE_DIST_BURST_SIZE; i++) diff --git
>> a/lib/librte_distributor/rte_distributor_single.c
>> b/lib/librte_distributor/rte_distributor_single.c
>> index abaf7730c..f4725b1d0 100644
>> --- a/lib/librte_distributor/rte_distributor_single.c
>> +++ b/lib/librte_distributor/rte_distributor_single.c
>> @@ -74,6 +74,10 @@ rte_distributor_return_pkt_single(struct
>> rte_distributor_single *d,
>>   	union rte_distributor_buffer_single *buf = &d->bufs[worker_id];
>>   	uint64_t req = (((int64_t)(uintptr_t)oldpkt) <<
>> RTE_DISTRIB_FLAG_BITS)
>>   			| RTE_DISTRIB_RETURN_BUF;
>> +	while (unlikely(__atomic_load_n(&buf->bufptr64,
>> __ATOMIC_RELAXED)
>> +			& RTE_DISTRIB_FLAGS_MASK))
>> +		rte_pause();
>> +
>>   	/* Sync with distributor on RETURN_BUF flag. */
>>   	__atomic_store_n(&(buf->bufptr64), req, __ATOMIC_RELEASE);
>>   	return 0;
>> --
>> 2.17.1

-- 
Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow at partner.samsung.com



More information about the dev mailing list