Re: [dpdk-dev] [PATCH 3/5] app/testpmd: add missing transmit errors stats

[David Marchand <david.marchand@redhat.com>]

Hello Konstantin,

On Sat, Feb 16, 2019 at 1:50 PM Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

>
>
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Friday, February 15, 2019 7:39 PM
> > To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Cc: David Marchand <david.marchand@redhat.com>; Richardson, Bruce <
> bruce.richardson@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> > <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>;
> Iremonger, Bernard <bernard.iremonger@intel.com>; Maxime Coquelin
> > <mcoqueli@redhat.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Andrew
> Rybchenko <arybchenko@solarflare.com>; Wiles, Keith
> > <keith.wiles@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 3/5] app/testpmd: add missing transmit
> errors stats
> >
> > 15/02/2019 19:42, Ananyev, Konstantin:
> > > >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David
> Marchand
> > > >>> I am also for option 2 especially because of this.
> > > >>> A driver that refuses a packet for reason X (which is a
> limitation, or an
> > > >>> incorrect config or whatever that is not a transient condition)
> but gives
> > > >>> it back to the application is a bad driver.
> > >
> > > >>Why? What.s wrong to leave it to the upper layer to decide what to
> > > >>do with the packets that can't be sent (by one reason or another)?
> > >
> > > >How does the upper layer know if this is a transient state or
> something that can't be resolved?
> > >
> > > Via rte_errno, for example.
> >
> > rte_errno is not a result per packet.
>
> Surely it is not.
> But tx_burst() can return after first failure.
>
> > I think it is better to "eat" the packet
> > as it is done for those transmitted to the HW.
>
> Probably extra clarification is needed here.
> Right now tx_burst (at least for PMDs I am aware about) doesn't
> do any checking that:
> -  packet is correct and can be handled
>    (this is responsibility of tx_prepare)
> - HW/PMD SW state is in valid and properly configured
>   (link is up, queue is configured, HW initialized properly).
>
> All that really tx_burst() care about -there is enough free TX
> descriptors to fill. When that happens - tx_burst() returns
> straightway.
>
> So what particular error conditions are you talking about,
> and when you think we have to 'eat' the packets?
>

- This is how Intel drivers are written yes.
But some drivers try to do more and have (useless ?) checks on mbufs or
internal states.

Found the following ones last week.
There are more but it takes time to investigate.
https://git.dpdk.org/dpdk/tree/drivers/net/atlantic/atl_rxtx.c?id=b13baac8d5ffb6b0b7a6ca0def884d3f1a82babb#n1346
https://git.dpdk.org/dpdk/tree/drivers/net/fm10k/fm10k_rxtx.c?id=b13baac8d5ffb6b0b7a6ca0def884d3f1a82babb#n646
https://git.dpdk.org/dpdk/tree/drivers/net/cxgbe/sge.c?id=b13baac8d5ffb6b0b7a6ca0def884d3f1a82babb#n1123

I would say first, we can have a cleanup to get rid of the unneeded checks,
and see what the different maintainers think about this.
Then look again at the situation.


- I will drop this patch on testpmd which was wrong.
But I intend to send an update on the doc to describe oerrors as solution 2:
2/ API break = oerrors stop being incremented for temporary
        unavailability (i.e. queue full, kind of ERETRY),
        report only packets which will be never sent,
        may be a small performance gain for some drivers

There is some cleanup to do as well in quite a few drivers.


-- 
David Marchand