On 9/23/2019 3:41 PM, Wiles, Keith wrote:
>
>
>> On Sep 23, 2019, at 8:22 AM, Smoczynski, MarcinX
>> <marcinx.smoczyn...@intel.com> wrote:
>>
>> When OS sends more packets than are being read with a single
>> 'rte_eth_rx_burst' call, rx packets are getting stucked in the tap pmd
>> and are unable to receive, because trigger_seen is getting updated
>> and consecutive calls are not getting any packets.
>>
>> Do not update trigger_seen unless less than a max number of packets were
>> received allowing next call to receive the rest.
>>
>> Remove unnecessary compiler barrier.
>>
>> Fixes: a0d8e807d9 ("net/tap: add Rx trigger")
>> Cc: sta...@dpdk.org
>>
>> Tested-by: Mariusz Drost <mariuszx.dr...@intel.com>
>> Tested-by: Konstantin Ananyev <konstantin.anan...@intel.com>
>> Signed-off-by: Marcin Smoczynski <marcinx.smoczyn...@intel.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 64bd04911..9c3adb832 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -353,10 +353,8 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs,
>> uint16_t nb_pkts)
>>
>> if (trigger == rxq->trigger_seen)
>> return 0;
>> - if (trigger)
>> - rxq->trigger_seen = trigger;
>> +
>> process_private = rte_eth_devices[rxq->in_port].process_private;
>> - rte_compiler_barrier();
>> for (num_rx = 0; num_rx < nb_pkts; ) {
>> struct rte_mbuf *mbuf = rxq->pool;
>> struct rte_mbuf *seg = NULL;
>> @@ -433,6 +431,9 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs,
>> uint16_t nb_pkts)
>> rxq->stats.ipackets += num_rx;
>> rxq->stats.ibytes += num_rx_bytes;
>>
>> + if (trigger && num_rx < nb_pkts)
>> + rxq->trigger_seen = trigger;
>> +
>> return num_rx;
>
> Looks reasonable to me. I was looking at the code for this patch and noticed
> what I believe is a bit odd.
>
> The line around 1352 does set req->trigger_seen = 1;, but the tap_trigger
> global variable is always set to tap_trigger = (tap_trigger + 1) |
> 0x80000000; in the signal handler.
> Just seems the line around 1352 should be set to at least 0x80000001 to begin
> with just to be constant. Not for this patch and maybe it does not matter in
> the long run.
>
>
> Reviewed-by: Keith Wiles <keith.wi...@intel.com>
Applied to dpdk-next-net/master, thanks.
