Re: [dpdk-dev] [PATCH] eventdev: fix incorrect MP/MC tail updates in rte_event_ring

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2018-03-29 14:38, Van Haaren, Harry wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Mattias Rönnblom
>> Sent: Wednesday, March 28, 2018 10:17 AM
>> To: dev@dpdk.org
>> Cc: Mattias Rönnblom <hofors@lysator.liu.se>
>> Subject: [dpdk-dev] [PATCH] eventdev: fix incorrect MP/MC tail updates in
>> rte_event_ring
>>
>> rte_event_ring enqueue and dequeue tail updates were hardcoded for a
>> SC/SP configuration.
>>
>> Signed-off-by: Mattias Rönnblom <hofors@lysator.liu.se>
>> ---
> 
> Hi Mattias,
> 
> 
> Thanks for your patch, a few notes;
> 
> 1)
> You can CC the maintainer of a section - the event rings falls under
> the eventdev library, and Jerin Jacob is the maintainer.
> 
> 
> 2)
> In DPDK we note patches that are "Fixes",
> so we can track what commit they fix. Fixes patches are
> also often candidates for backporting.
> 
> Details on getting the fixes line here:
> https://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body
> 
> For this patch, the following is the fixline:
> 
> Fixes: dc39e2f359b5 ("eventdev: add ring structure for events")
> Cc: bruce.richardson@intel.com
> 
> 

Thanks for your help. I'll resubmit and try to get the administrative 
details correct.

>>   lib/librte_eventdev/rte_event_ring.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_eventdev/rte_event_ring.h
>> b/lib/librte_eventdev/rte_event_ring.h
>> index 29d4228..07b4559 100644
>> --- a/lib/librte_eventdev/rte_event_ring.h
>> +++ b/lib/librte_eventdev/rte_event_ring.h
>> @@ -99,7 +99,7 @@ rte_event_ring_enqueue_burst(struct rte_event_ring *r,
>>
>>   	ENQUEUE_PTRS(&r->r, &r[1], prod_head, events, n, struct rte_event);
>>
>> -	update_tail(&r->r.prod, prod_head, prod_next, 1, 1);
>> +	update_tail(&r->r.prod, prod_head, prod_next, 1, r->r.prod.single);
>>   end:
>>   	if (free_space != NULL)
>>   		*free_space = free_entries - n;
>> @@ -140,7 +140,7 @@ rte_event_ring_dequeue_burst(struct rte_event_ring *r,
>>
>>   	DEQUEUE_PTRS(&r->r, &r[1], cons_head, events, n, struct rte_event);
>>
>> -	update_tail(&r->r.cons, cons_head, cons_next, 1, 0);
>> +	update_tail(&r->r.cons, cons_head, cons_next, 1, r->r.cons.single);
> 
> 
> The signature of update_tail() is as follows:
> 
> static __rte_always_inline void
> update_tail(struct rte_ring_headtail *ht, uint32_t old_val, uint32_t new_val,
> 		uint32_t single, uint32_t enqueue)
> 
> It seems to be that the 2nd last parameter sets the single/multi producer?
> 

Yes, you are right. I ported this fix from my 17.08 tree, and 
update_tail() has changed since then - something I failed to notice.

I reproduced the issue on 17.08, and even on a machine with a strong 
memory model, the event ring doesn't work correctly in a MP/SC 
configuration. With the fix, it at least passes my tests on 17.08. I 
didn't try MP/MC, and I also didn't try anything beyond building on HEAD.

Regards,
	Mattias