[dpdk-stable] [PATCH 2/3] net/pcap: fix transmit return count in error conditions

[David Marchand]

On Wed, Jul 24, 2019 at 8:36 PM Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>
> On 7/24/2019 12:54 PM, David Marchand wrote:
> > When a packet cannot be transmitted, the driver is supposed to free this
> > packet and report it as handled.
> > This is to prevent the application from retrying to send the same packet
> > and ending up in a liveloop since the driver will never manage to send
> > it.
> >
> > Fixes: 49a0a2ffd5db ("net/pcap: fix possible mbuf double freeing")
> > Fixes: 6db141c91e1f ("pcap: support jumbo frames")
> > CC: stable at dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand at redhat.com>
> > ---
> >  drivers/net/pcap/rte_eth_pcap.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> > index 470867d..5e5aab7 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -354,7 +354,8 @@ struct pmd_devargs_all {
> >                                       mbuf->pkt_len,
> >                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > -                             break;
> > +                             rte_pktmbuf_free(mbuf);
> > +                             continue;
>
> +1
> Very recently 'rte_pktmbuf_free()' was moved because it wasn't compatible with
> return value, but this looks better, to free the mbuf and record it as error.
>
> >                       }
> >               }
> >
> > @@ -373,7 +374,7 @@ struct pmd_devargs_all {
> >       dumper_q->tx_stat.bytes += tx_bytes;
> >       dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
> >
> > -     return num_tx;
> > +     return nb_pkts;
> >  }
> >
> >  /*
> > @@ -439,14 +440,15 @@ struct pmd_devargs_all {
> >                                       mbuf->pkt_len,
> >                                       RTE_ETHER_MAX_JUMBO_FRAME_LEN);
> >
> > -                             break;
> > +                             rte_pktmbuf_free(mbuf);
> > +                             continue;
> >                       }
> >               }
> >
> > -             if (unlikely(ret != 0))
> > -                     break;
> > -             num_tx++;
> > -             tx_bytes += mbuf->pkt_len;
> > +             if (ret == 0) {
> > +                     num_tx++;
> > +                     tx_bytes += mbuf->pkt_len;
> > +             }
>
> I don't know this part, this is in 'eth_pcap_tx()' which writes packets to the
> interfaces.
>
> if 'pcap_sendpacket()' fails this doesn't mean packet can't be sent and may
> cause a liveloop. Why not keep the existing behavior and let application to decide?

The manual is not clear to me.
Do we really have temporary situations where retries are fine? and if
so, can we differentiate them from things like incorrect permissions
etc...

If we can't, dropping is safer.

-- 
David Marchand