Re: [dpdk-dev] [PATCH] eventdev: fix Rx adapter stalls on event device backpressure

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2021-11-10 11:08, Jayatheerthan, Jay wrote:
>> -----Original Message-----
>> From: Kundapura, Ganapati <ganapati.kundapura@intel.com>
>> Sent: Wednesday, November 10, 2021 1:56 PM
>> To: mattias.ronnblom <mattias.ronnblom@ericsson.com>; jerinj@marvell.com; Jayatheerthan, Jay <jay.jayatheerthan@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: RE: [PATCH] eventdev: fix Rx adapter stalls on event device backpressure
>>
>> Hi Mattias,
>>
>>> -----Original Message-----
>>> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>> Sent: 09 November 2021 13:58
>>> To: Kundapura, Ganapati <ganapati.kundapura@intel.com>;
>>> jerinj@marvell.com; Jayatheerthan, Jay <jay.jayatheerthan@intel.com>
>>> Cc: dev@dpdk.org; stable@dpdk.org
>>> Subject: Re: [PATCH] eventdev: fix Rx adapter stalls on event device
>>> backpressure
>>>
>>> On 2021-11-09 07:26, Kundapura, Ganapati wrote:
>>>> Hi Mattias,
>>>>
>>>>> -----Original Message-----
>>>>> From: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>>> Sent: 08 November 2021 19:14
>>>>> To: jerinj@marvell.com; Jayatheerthan, Jay
>>>>> <jay.jayatheerthan@intel.com>
>>>>> Cc: dev@dpdk.org; Kundapura, Ganapati
>>> <ganapati.kundapura@intel.com>;
>>>>> stable@dpdk.org
>>>>> Subject: Re: [PATCH] eventdev: fix Rx adapter stalls on event device
>>>>> backpressure
>>>>>
>>>>> On 2021-11-08 14:25, Mattias Rönnblom wrote:
>>>>>> In the Eventdev Ethernet RX Adapter, correctly handle the case where
>>>>>> the circular enqueue buffer head and tail index points to the same
>>>>>> element (i.e., the buffer is full) and the buffer has wrapped.
> Could you update the description to reflect last instead of tail ? last > 0 doesn’t mean the buffer is full. It is just a marker for event device enqueue when tail has rolled over and head hasn’t. Below would work, if you want to use it.
>
> "In the Eventdev Ethernet RX Adapter, correctly handle the case where
> the circular enqueue buffer head and last index points to the same
> element."


Good point. I'll send a v2. Thanks.


>>>>>> This bug may be triggered in case there is backpressure from the
>>>>>> event device to the RX adapter.
>>>>>>
>>>>>> Fixes: 8113fd15e229 ("eventdev/eth_rx: make enqueue buffer
>>>>>> circular")
>>>>>> Cc: ganapati.kundapura@intel.com
>>>>>> Cc: stable@dpdk.org
>>>>> Disregard the stable cc. This bug does not appear in any released
>>>>> DPDK version (e.g., 21.08).
>>>>>
>>>>>
>>>>>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>>>>>> ---
>>>>>>     lib/eventdev/rte_event_eth_rx_adapter.c | 22 ++++++++++++++------
>>> --
>>>>>>     1 file changed, 14 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/eventdev/rte_event_eth_rx_adapter.c
>>>>> b/lib/eventdev/rte_event_eth_rx_adapter.c
>>>>>> index 56318b5a6f..809416d9b7 100644
>>>>>> --- a/lib/eventdev/rte_event_eth_rx_adapter.c
>>>>>> +++ b/lib/eventdev/rte_event_eth_rx_adapter.c
>>>>>> @@ -777,19 +777,25 @@ rxa_flush_event_buffer(struct
>>>>> event_eth_rx_adapter *rx_adapter,
>>>>>>     		       struct eth_event_enqueue_buffer *buf,
>>>>>>     		       struct rte_event_eth_rx_adapter_stats *stats)
>>>>>>     {
>>>>>> -	uint16_t count = buf->last ? buf->last - buf->head : buf->count;
>>>>>> +	uint16_t count = buf->count;
>>>>>> +	uint16_t n = 0;
>>>>>>
>>>>>>     	if (!count)
>>>>>>     		return 0;
>>>>>>
>>>>>> -	uint16_t n = rte_event_enqueue_new_burst(rx_adapter-
>>>>>> eventdev_id,
>>>>>> -					rx_adapter->event_port_id,
>>>>>> -					&buf->events[buf->head],
>>>>>> -					count);
>>>>>> -	if (n != count)
>>>>>> -		stats->rx_enq_retry++;
>>>>>> +	if (buf->last)
>>>>>> +		count = buf->last - buf->head;
>>>>>> +
>>>>>> +	if (count) {
>>>>>> +		n = rte_event_enqueue_new_burst(rx_adapter-
>>>>>> eventdev_id,
>>>>>> +						rx_adapter->event_port_id,
>>>>>> +						&buf->events[buf->head],
>>>>>> +						count);
>>>>>> +		if (n != count)
>>>>>> +			stats->rx_enq_retry++;
>>>>>>
>>>>>> -	buf->head += n;
>>>>>> +		buf->head += n;
>>>>>> +	}
>>>>>>
>>>>>>     	if (buf->last && n == count) {
>>>>>>     		uint16_t n1;
>>>> When head = tail, count is the number of events in the event buffer
>>>> i.e count = buf->count and last = 0 Last is the marker used in case of roll
>>> over.
>>>> In case of tail roll over and head is not, events are processed from head to
>>> last and zero to tail.
>>>> Looks like change is same as the original.
>>>> Could you please clarify more on this change and also clarify if you were
>>> able to reproduce the backpressure issue?
>>>
>>>
>>> The enqueue buffer state I encountered was last != 0 and head == tail, and
>>> size != 0. In that case, the function returns early, since count == 0, even
>>> though there are events stored from 0 to tail. head, tail, last and size were all
>>> 192, from what I remember.
>>>
>> Issue seems to be head is catching up with last not tail when the tail is rolled over.
>> When head = last, count is zero and returning without processing the events from 0 to tail.
>> Approved. You can add my ack
>>
>>> For reasons I didn't analyze, it only seem to occur when the event port's
>>> enqueue burst size was larger than 32 (the RX burst used against the RX
>>> adapter's ethdev queues), and there was backpressure from the event
>>> device.
>>>