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

Ananyev, Konstantin konstantin.ananyev at intel.com
Sun Feb 24 12:55:18 CET 2019


Hi David,


Hello Konstantin,

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


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net<mailto:thomas at monjalon.net>]
> Sent: Friday, February 15, 2019 7:39 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com<mailto:konstantin.ananyev at intel.com>>
> Cc: David Marchand <david.marchand at redhat.com<mailto:david.marchand at redhat.com>>; Richardson, Bruce <bruce.richardson at intel.com<mailto:bruce.richardson at intel.com>>; dev at dpdk.org<mailto:dev at dpdk.org>; Lu, Wenzhuo
> <wenzhuo.lu at intel.com<mailto:wenzhuo.lu at intel.com>>; Wu, Jingjing <jingjing.wu at intel.com<mailto:jingjing.wu at intel.com>>; Iremonger, Bernard <bernard.iremonger at intel.com<mailto:bernard.iremonger at intel.com>>; Maxime Coquelin
> <mcoqueli at redhat.com<mailto:mcoqueli at redhat.com>>; Yigit, Ferruh <ferruh.yigit at intel.com<mailto:ferruh.yigit at intel.com>>; Andrew Rybchenko <arybchenko at solarflare.com<mailto:arybchenko at solarflare.com>>; Wiles, Keith
> <keith.wiles at intel.com<mailto:keith.wiles at 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 at dpdk.org<mailto:dev-bounces at 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.

I didn’t look at all your links above, but fm10k checks definitely seems redundant.
Cleanup of such things sounds like a good thing.
Thanks
Konstantin


More information about the dev mailing list