[3/5] app/testpmd: add missing transmit errors stats
Checks
Commit Message
pmd can report transmit errors but those stats are not accounted here.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test-pmd/testpmd.c | 1 +
1 file changed, 1 insertion(+)
Comments
On Thu, Feb 14, 2019 at 04:42:50PM +0100, David Marchand wrote:
> pmd can report transmit errors but those stats are not accounted here.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> app/test-pmd/testpmd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 984155a..3acd97b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1838,6 +1838,7 @@ struct extmem_param {
> total_recv += stats.ipackets;
> total_xmit += stats.opackets;
> total_rx_dropped += stats.imissed;
> + port->tx_dropped += stats.oerrors;
> total_tx_dropped += port->tx_dropped;
> total_rx_nombuf += stats.rx_nombuf;
>
>
Without knowing as to whether the line is needed or not, the line itself
looks out of place. All other lines are assignments to local variables,
apart from this. Should a local variable be defined for consistency?
/Bruce
On Thu, Feb 14, 2019 at 5:30 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:
> On Thu, Feb 14, 2019 at 04:42:50PM +0100, David Marchand wrote:
> > pmd can report transmit errors but those stats are not accounted here.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > app/test-pmd/testpmd.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 984155a..3acd97b 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -1838,6 +1838,7 @@ struct extmem_param {
> > total_recv += stats.ipackets;
> > total_xmit += stats.opackets;
> > total_rx_dropped += stats.imissed;
> > + port->tx_dropped += stats.oerrors;
> > total_tx_dropped += port->tx_dropped;
> > total_rx_nombuf += stats.rx_nombuf;
> >
> >
> Without knowing as to whether the line is needed or not, the line itself
> looks out of place. All other lines are assignments to local variables,
> apart from this. Should a local variable be defined for consistency?
>
Indeed this looks wrong to add it to port->tx_dropped.
It actually "works" since this part is called when stopping forwarding and
port->tx_dropped gets reset later when starting forwarding again.
I suppose I should move this to total_tx_dropped instead.
Thanks Bruce.
On Thu, Feb 14, 2019 at 6:39 PM David Marchand <david.marchand@redhat.com>
wrote:
> On Thu, Feb 14, 2019 at 5:30 PM Bruce Richardson <
> bruce.richardson@intel.com> wrote:
>
>> On Thu, Feb 14, 2019 at 04:42:50PM +0100, David Marchand wrote:
>> > pmd can report transmit errors but those stats are not accounted here.
>> >
>> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>> > ---
>> > app/test-pmd/testpmd.c | 1 +
>> > 1 file changed, 1 insertion(+)
>> >
>> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> > index 984155a..3acd97b 100644
>> > --- a/app/test-pmd/testpmd.c
>> > +++ b/app/test-pmd/testpmd.c
>> > @@ -1838,6 +1838,7 @@ struct extmem_param {
>> > total_recv += stats.ipackets;
>> > total_xmit += stats.opackets;
>> > total_rx_dropped += stats.imissed;
>> > + port->tx_dropped += stats.oerrors;
>> > total_tx_dropped += port->tx_dropped;
>> > total_rx_nombuf += stats.rx_nombuf;
>> >
>> >
>> Without knowing as to whether the line is needed or not, the line itself
>> looks out of place. All other lines are assignments to local variables,
>> apart from this. Should a local variable be defined for consistency?
>>
>
>
Thinking again about this oerrors stats...
We had a discussion with Maxime, last week.
So I want to make sure that what we both agreed makes sense :-)
What is the purpose of oerrors ?
Since the drivers (via rte_eth_tx_burst return value) report the numbers of
packets successfully transmitted, the application can try to retransmit the
packets that did not make it and counts this.
If the driver counts such "missed" packets, then it does the job the
application will do anyway (wasting some cycles).
But what is more a problem is that the application does not know if the
packets in oerrors are its own retries or problems that the driver can not
detect (hw problems) but the hw can.
So the best option is that oerrors does not report the packets the driver
refuses (and I can see some drivers that do not comply to this) but only
"external" errors from the driver pov.
Back to my patch here, if we agree on this definition of oerrors, I can not
add it to total_tx_dropped, but I suppose I can add some "TX HW errors: "
and "Total TX HW errors: " logs so that we are aware that something went
bad "further" than the driver.
Let's sleep on it :-)
14/02/2019 19:51, David Marchand:
> What is the purpose of oerrors ?
>
> Since the drivers (via rte_eth_tx_burst return value) report the numbers of
> packets successfully transmitted, the application can try to retransmit the
> packets that did not make it and counts this.
> If the driver counts such "missed" packets, then it does the job the
> application will do anyway (wasting some cycles).
> But what is more a problem is that the application does not know if the
> packets in oerrors are its own retries or problems that the driver can not
> detect (hw problems) but the hw can.
>
> So the best option is that oerrors does not report the packets the driver
> refuses (and I can see some drivers that do not comply to this) but only
> "external" errors from the driver pov.
I can see the benefit of having driver errors in the stats,
so it is generically stored for later analysis or print.
It could be managed at ethdev level instead of the application
doing the computation.
What about splitting the Tx errors in 2 fields? oerrors / ofull ?
Who said it's awful? sorry Bruce for anticipating ;)
On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> 14/02/2019 19:51, David Marchand:
> > What is the purpose of oerrors ?
> >
> > Since the drivers (via rte_eth_tx_burst return value) report the numbers
> of
> > packets successfully transmitted, the application can try to retransmit
> the
> > packets that did not make it and counts this.
> > If the driver counts such "missed" packets, then it does the job the
> > application will do anyway (wasting some cycles).
> > But what is more a problem is that the application does not know if the
> > packets in oerrors are its own retries or problems that the driver can
> not
> > detect (hw problems) but the hw can.
> >
> > So the best option is that oerrors does not report the packets the driver
> > refuses (and I can see some drivers that do not comply to this) but only
> > "external" errors from the driver pov.
>
> I can see the benefit of having driver errors in the stats,
> so it is generically stored for later analysis or print.
> It could be managed at ethdev level instead of the application
> doing the computation.
>
> What about splitting the Tx errors in 2 fields? oerrors / ofull ?
> Who said it's awful? sorry Bruce for anticipating ;)
>
Summary, correct me if we are not aligned :-)
- ofull (maybe ofifoerrors?) is actually a count of SW failed transmits
- it would be handled in rte_eth_tx_burst() itself in a generic way
- the drivers do not need to track such SW failed transmits
- oerrors only counts packets HW failed transmits, dropped out of the
driver tx_pkt_burst() knowledge
- the application does not have to track SW failed transmits since the
stats is in ethdev
It sounds good to me, this means an ethdev abi breakage.
I will drop my current patch anyway.
Touching oerrors would be a separate effort.
On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
> On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
> <[1]thomas@monjalon.net> wrote:
>
> 14/02/2019 19:51, David Marchand:
> > What is the purpose of oerrors ?
> >
> > Since the drivers (via rte_eth_tx_burst return value) report the
> numbers of
> > packets successfully transmitted, the application can try to
> retransmit the
> > packets that did not make it and counts this.
> > If the driver counts such "missed" packets, then it does the job
> the
> > application will do anyway (wasting some cycles).
> > But what is more a problem is that the application does not know
> if the
> > packets in oerrors are its own retries or problems that the driver
> can not
> > detect (hw problems) but the hw can.
> >
> > So the best option is that oerrors does not report the packets the
> driver
> > refuses (and I can see some drivers that do not comply to this)
> but only
> > "external" errors from the driver pov.
> I can see the benefit of having driver errors in the stats,
> so it is generically stored for later analysis or print.
> It could be managed at ethdev level instead of the application
> doing the computation.
> What about splitting the Tx errors in 2 fields? oerrors / ofull ?
> Who said it's awful? sorry Bruce for anticipating ;)
>
> Summary, correct me if we are not aligned :-)
> - ofull (maybe ofifoerrors?) is actually a count of SW failed transmits
> - it would be handled in rte_eth_tx_burst() itself in a generic way
> - the drivers do not need to track such SW failed transmits
> - oerrors only counts packets HW failed transmits, dropped out of the
> driver tx_pkt_burst() knowledge
> - the application does not have to track SW failed transmits since the
> stats is in ethdev
> It sounds good to me, this means an ethdev abi breakage.
Hang on, why do we need ethdev to track this at all, given that it's
trivial for apps to track this themselves. Would we not be better just to
add this tracking into testpmd and leave ethdev and drivers alone? Perhaps
I'm missing something?
> On Feb 15, 2019, at 8:05 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
>
> On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
>> On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
>> <[1]thomas@monjalon.net> wrote:
>>
>> 14/02/2019 19:51, David Marchand:
>>> What is the purpose of oerrors ?
>>>
>>> Since the drivers (via rte_eth_tx_burst return value) report the
>> numbers of
>>> packets successfully transmitted, the application can try to
>> retransmit the
>>> packets that did not make it and counts this.
>>> If the driver counts such "missed" packets, then it does the job
>> the
>>> application will do anyway (wasting some cycles).
>>> But what is more a problem is that the application does not know
>> if the
>>> packets in oerrors are its own retries or problems that the driver
>> can not
>>> detect (hw problems) but the hw can.
>>>
>>> So the best option is that oerrors does not report the packets the
>> driver
>>> refuses (and I can see some drivers that do not comply to this)
>> but only
>>> "external" errors from the driver pov.
>> I can see the benefit of having driver errors in the stats,
>> so it is generically stored for later analysis or print.
>> It could be managed at ethdev level instead of the application
>> doing the computation.
>> What about splitting the Tx errors in 2 fields? oerrors / ofull ?
>> Who said it's awful? sorry Bruce for anticipating ;)
>>
>> Summary, correct me if we are not aligned :-)
>> - ofull (maybe ofifoerrors?) is actually a count of SW failed transmits
>> - it would be handled in rte_eth_tx_burst() itself in a generic way
>> - the drivers do not need to track such SW failed transmits
>> - oerrors only counts packets HW failed transmits, dropped out of the
>> driver tx_pkt_burst() knowledge
>> - the application does not have to track SW failed transmits since the
>> stats is in ethdev
>> It sounds good to me, this means an ethdev abi breakage.
>
> Hang on, why do we need ethdev to track this at all, given that it's
> trivial for apps to track this themselves. Would we not be better just to
> add this tracking into testpmd and leave ethdev and drivers alone? Perhaps
> I'm missing something?
Adding the counters to ethdev stats is a good idea to me, the number of tx full failures is a great counter as it can tell you a lot about performance of the application. if the ofull counter is high then we have a lot of re-xmit attempts which can point to the problem quicker IMO. Adding it to the PMDs is the right place for this type of information as it is a very common needed counter.
>
Regards,
Keith
On Fri, Feb 15, 2019 at 3:05 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:
> On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
> > On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
> > <[1]thomas@monjalon.net> wrote:
> >
> > 14/02/2019 19:51, David Marchand:
> > > What is the purpose of oerrors ?
> > >
> > > Since the drivers (via rte_eth_tx_burst return value) report the
> > numbers of
> > > packets successfully transmitted, the application can try to
> > retransmit the
> > > packets that did not make it and counts this.
> > > If the driver counts such "missed" packets, then it does the job
> > the
> > > application will do anyway (wasting some cycles).
> > > But what is more a problem is that the application does not know
> > if the
> > > packets in oerrors are its own retries or problems that the driver
> > can not
> > > detect (hw problems) but the hw can.
> > >
> > > So the best option is that oerrors does not report the packets the
> > driver
> > > refuses (and I can see some drivers that do not comply to this)
> > but only
> > > "external" errors from the driver pov.
> > I can see the benefit of having driver errors in the stats,
> > so it is generically stored for later analysis or print.
> > It could be managed at ethdev level instead of the application
> > doing the computation.
> > What about splitting the Tx errors in 2 fields? oerrors / ofull ?
> > Who said it's awful? sorry Bruce for anticipating ;)
> >
> > Summary, correct me if we are not aligned :-)
> > - ofull (maybe ofifoerrors?) is actually a count of SW failed
> transmits
> > - it would be handled in rte_eth_tx_burst() itself in a generic way
> > - the drivers do not need to track such SW failed transmits
> > - oerrors only counts packets HW failed transmits, dropped out of the
> > driver tx_pkt_burst() knowledge
> > - the application does not have to track SW failed transmits since the
> > stats is in ethdev
> > It sounds good to me, this means an ethdev abi breakage.
>
> Hang on, why do we need ethdev to track this at all, given that it's
> trivial for apps to track this themselves. Would we not be better just to
> add this tracking into testpmd and leave ethdev and drivers alone? Perhaps
> I'm missing something?
>
This was my first intention but Thomas hopped in ;-)
testpmd does it already via the fs->fwd_dropped stats and ovs has its
tx_dropped stat.
The problem is that all drivers have different approach about this.
Some drivers only count real hw errors in oerrors.
But others count the packets it can't send in oerrors (+ there are some
cases that seem buggy to me where the driver will always refuse the mbufs
for reason X and the application can retry indefinitely to send...).
15/02/2019 16:04, David Marchand:
> On Fri, Feb 15, 2019 at 3:05 PM Bruce Richardson <bruce.richardson@intel.com>
> wrote:
>
> > On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
> > > On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
> > > <[1]thomas@monjalon.net> wrote:
> > >
> > > 14/02/2019 19:51, David Marchand:
> > > > What is the purpose of oerrors ?
> > > >
> > > > Since the drivers (via rte_eth_tx_burst return value) report the
> > > numbers of
> > > > packets successfully transmitted, the application can try to
> > > retransmit the
> > > > packets that did not make it and counts this.
> > > > If the driver counts such "missed" packets, then it does the job
> > > the
> > > > application will do anyway (wasting some cycles).
> > > > But what is more a problem is that the application does not know
> > > if the
> > > > packets in oerrors are its own retries or problems that the driver
> > > can not
> > > > detect (hw problems) but the hw can.
> > > >
> > > > So the best option is that oerrors does not report the packets the
> > > driver
> > > > refuses (and I can see some drivers that do not comply to this)
> > > but only
> > > > "external" errors from the driver pov.
> > > I can see the benefit of having driver errors in the stats,
> > > so it is generically stored for later analysis or print.
> > > It could be managed at ethdev level instead of the application
> > > doing the computation.
> > > What about splitting the Tx errors in 2 fields? oerrors / ofull ?
> > > Who said it's awful? sorry Bruce for anticipating ;)
> > >
> > > Summary, correct me if we are not aligned :-)
> > > - ofull (maybe ofifoerrors?) is actually a count of SW failed
> > transmits
> > > - it would be handled in rte_eth_tx_burst() itself in a generic way
> > > - the drivers do not need to track such SW failed transmits
> > > - oerrors only counts packets HW failed transmits, dropped out of the
> > > driver tx_pkt_burst() knowledge
> > > - the application does not have to track SW failed transmits since the
> > > stats is in ethdev
> > > It sounds good to me, this means an ethdev abi breakage.
> >
> > Hang on, why do we need ethdev to track this at all, given that it's
> > trivial for apps to track this themselves. Would we not be better just to
> > add this tracking into testpmd and leave ethdev and drivers alone? Perhaps
> > I'm missing something?
> >
>
> This was my first intention but Thomas hopped in ;-)
I was just opening the discussion :)
> testpmd does it already via the fs->fwd_dropped stats and ovs has its
> tx_dropped stat.
>
> The problem is that all drivers have different approach about this.
> Some drivers only count real hw errors in oerrors.
> But others count the packets it can't send in oerrors (+ there are some
> cases that seem buggy to me where the driver will always refuse the mbufs
> for reason X and the application can retry indefinitely to send...).
We have 3 options:
1/ status quo = oerrors is inconsistent across drivers
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
3/ API + ABI break = same as 2/ +
report ERETRY errors in ofull (same as tx_burst() delta)
Note that the option 2 is a light API break which does not require
any deprecation notice because the original definition of oerrors
is really vague: "failed transmitted packets"
By changing the definition of errors to "packets lost", we can count
HW errors + packets not matching requirements.
As David suggests, the packets not matching requirements can be freed
as it is done for packets successfully transmitted to the HW.
We need also to update the definition of the return value of
rte_eth_tx_burst(): "packets actually stored in transmit descriptors".
We should also count the bad packets rejected by the driver.
Then the number of bad packets would be the difference between
the return value of rte_eth_tx_burst() and opackets counter.
This solution is fixing some bugs and enforce a consistent behaviour.
The option 3 is breaking the ABI and may degrade the performances.
The only benefit is convenience or semantic: ofull would be the
equivalent of imissed.
The application can count the same by making the difference between
the burst size and the return of rte_eth_tx_burst.
My vote is for the option 2.
On Fri, Feb 15, 2019 at 5:19 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 15/02/2019 16:04, David Marchand:
> > On Fri, Feb 15, 2019 at 3:05 PM Bruce Richardson <
> bruce.richardson@intel.com>
> > wrote:
> >
> > > On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
> > > > On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
> > > > <[1]thomas@monjalon.net> wrote:
> > > >
> > > > 14/02/2019 19:51, David Marchand:
> > > > > What is the purpose of oerrors ?
> > > > >
> > > > > Since the drivers (via rte_eth_tx_burst return value) report
> the
> > > > numbers of
> > > > > packets successfully transmitted, the application can try to
> > > > retransmit the
> > > > > packets that did not make it and counts this.
> > > > > If the driver counts such "missed" packets, then it does the
> job
> > > > the
> > > > > application will do anyway (wasting some cycles).
> > > > > But what is more a problem is that the application does not
> know
> > > > if the
> > > > > packets in oerrors are its own retries or problems that the
> driver
> > > > can not
> > > > > detect (hw problems) but the hw can.
> > > > >
> > > > > So the best option is that oerrors does not report the
> packets the
> > > > driver
> > > > > refuses (and I can see some drivers that do not comply to
> this)
> > > > but only
> > > > > "external" errors from the driver pov.
> > > > I can see the benefit of having driver errors in the stats,
> > > > so it is generically stored for later analysis or print.
> > > > It could be managed at ethdev level instead of the application
> > > > doing the computation.
> > > > What about splitting the Tx errors in 2 fields? oerrors / ofull
> ?
> > > > Who said it's awful? sorry Bruce for anticipating ;)
> > > >
> > > > Summary, correct me if we are not aligned :-)
> > > > - ofull (maybe ofifoerrors?) is actually a count of SW failed
> > > transmits
> > > > - it would be handled in rte_eth_tx_burst() itself in a generic
> way
> > > > - the drivers do not need to track such SW failed transmits
> > > > - oerrors only counts packets HW failed transmits, dropped out of
> the
> > > > driver tx_pkt_burst() knowledge
> > > > - the application does not have to track SW failed transmits
> since the
> > > > stats is in ethdev
> > > > It sounds good to me, this means an ethdev abi breakage.
> > >
> > > Hang on, why do we need ethdev to track this at all, given that it's
> > > trivial for apps to track this themselves. Would we not be better just
> to
> > > add this tracking into testpmd and leave ethdev and drivers alone?
> Perhaps
> > > I'm missing something?
> > >
> >
> > This was my first intention but Thomas hopped in ;-)
>
> I was just opening the discussion :)
>
> > testpmd does it already via the fs->fwd_dropped stats and ovs has its
> > tx_dropped stat.
> >
> > The problem is that all drivers have different approach about this.
> > Some drivers only count real hw errors in oerrors.
> > But others count the packets it can't send in oerrors (+ there are some
> > cases that seem buggy to me where the driver will always refuse the mbufs
> > for reason X and the application can retry indefinitely to send...).
>
> We have 3 options:
> 1/ status quo = oerrors is inconsistent across drivers
> 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
> 3/ API + ABI break = same as 2/ +
> report ERETRY errors in ofull (same as tx_burst() delta)
>
> Note that the option 2 is a light API break which does not require
> any deprecation notice because the original definition of oerrors
> is really vague: "failed transmitted packets"
> By changing the definition of errors to "packets lost", we can count
> HW errors + packets not matching requirements.
> As David suggests, the packets not matching requirements can be freed
> as it is done for packets successfully transmitted to the HW.
> We need also to update the definition of the return value of
> rte_eth_tx_burst(): "packets actually stored in transmit descriptors".
> We should also count the bad packets rejected by the driver.
> Then the number of bad packets would be the difference between
> the return value of rte_eth_tx_burst() and opackets counter.
> This solution is fixing some bugs and enforce a consistent behaviour.
>
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.
> The option 3 is breaking the ABI and may degrade the performances.
> The only benefit is convenience or semantic: ofull would be the
> equivalent of imissed.
> The application can count the same by making the difference between
> the burst size and the return of rte_eth_tx_burst.
>
> My vote is for the option 2.
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Friday, February 15, 2019 5:32 PM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: 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
>
> On Fri, Feb 15, 2019 at 5:19 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> > 15/02/2019 16:04, David Marchand:
> > > On Fri, Feb 15, 2019 at 3:05 PM Bruce Richardson <
> > bruce.richardson@intel.com>
> > > wrote:
> > >
> > > > On Fri, Feb 15, 2019 at 10:33:47AM +0100, David Marchand wrote:
> > > > > On Fri, Feb 15, 2019 at 9:58 AM Thomas Monjalon
> > > > > <[1]thomas@monjalon.net> wrote:
> > > > >
> > > > > 14/02/2019 19:51, David Marchand:
> > > > > > What is the purpose of oerrors ?
> > > > > >
> > > > > > Since the drivers (via rte_eth_tx_burst return value) report
> > the
> > > > > numbers of
> > > > > > packets successfully transmitted, the application can try to
> > > > > retransmit the
> > > > > > packets that did not make it and counts this.
> > > > > > If the driver counts such "missed" packets, then it does the
> > job
> > > > > the
> > > > > > application will do anyway (wasting some cycles).
> > > > > > But what is more a problem is that the application does not
> > know
> > > > > if the
> > > > > > packets in oerrors are its own retries or problems that the
> > driver
> > > > > can not
> > > > > > detect (hw problems) but the hw can.
> > > > > >
> > > > > > So the best option is that oerrors does not report the
> > packets the
> > > > > driver
> > > > > > refuses (and I can see some drivers that do not comply to
> > this)
> > > > > but only
> > > > > > "external" errors from the driver pov.
> > > > > I can see the benefit of having driver errors in the stats,
> > > > > so it is generically stored for later analysis or print.
> > > > > It could be managed at ethdev level instead of the application
> > > > > doing the computation.
> > > > > What about splitting the Tx errors in 2 fields? oerrors / ofull
> > ?
> > > > > Who said it's awful? sorry Bruce for anticipating ;)
> > > > >
> > > > > Summary, correct me if we are not aligned :-)
> > > > > - ofull (maybe ofifoerrors?) is actually a count of SW failed
> > > > transmits
> > > > > - it would be handled in rte_eth_tx_burst() itself in a generic
> > way
> > > > > - the drivers do not need to track such SW failed transmits
> > > > > - oerrors only counts packets HW failed transmits, dropped out of
> > the
> > > > > driver tx_pkt_burst() knowledge
> > > > > - the application does not have to track SW failed transmits
> > since the
> > > > > stats is in ethdev
> > > > > It sounds good to me, this means an ethdev abi breakage.
> > > >
> > > > Hang on, why do we need ethdev to track this at all, given that it's
> > > > trivial for apps to track this themselves. Would we not be better just
> > to
> > > > add this tracking into testpmd and leave ethdev and drivers alone?
> > Perhaps
> > > > I'm missing something?
> > > >
> > >
> > > This was my first intention but Thomas hopped in ;-)
> >
> > I was just opening the discussion :)
> >
> > > testpmd does it already via the fs->fwd_dropped stats and ovs has its
> > > tx_dropped stat.
> > >
> > > The problem is that all drivers have different approach about this.
> > > Some drivers only count real hw errors in oerrors.
> > > But others count the packets it can't send in oerrors (+ there are some
> > > cases that seem buggy to me where the driver will always refuse the mbufs
> > > for reason X and the application can retry indefinitely to send...).
> >
> > We have 3 options:
> > 1/ status quo = oerrors is inconsistent across drivers
> > 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
> > 3/ API + ABI break = same as 2/ +
> > report ERETRY errors in ofull (same as tx_burst() delta)
> >
> > Note that the option 2 is a light API break which does not require
> > any deprecation notice because the original definition of oerrors
> > is really vague: "failed transmitted packets"
> > By changing the definition of errors to "packets lost", we can count
> > HW errors + packets not matching requirements.
> > As David suggests, the packets not matching requirements can be freed
> > as it is done for packets successfully transmitted to the HW.
> > We need also to update the definition of the return value of
> > rte_eth_tx_burst(): "packets actually stored in transmit descriptors".
> > We should also count the bad packets rejected by the driver.
> > Then the number of bad packets would be the difference between
> > the return value of rte_eth_tx_burst() and opackets counter.
> > This solution is fixing some bugs and enforce a consistent behaviour.
> >
>
> 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)?
>
>
> > The option 3 is breaking the ABI and may degrade the performances.
> > The only benefit is convenience or semantic: ofull would be the
> > equivalent of imissed.
> > The application can count the same by making the difference between
> > the burst size and the return of rte_eth_tx_burst.
> >
> > My vote is for the option 2.
> >
>
>
>
> --
> David Marchand
On Fri, Feb 15, 2019 at 7:15 PM Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:
> > -----Original Message-----
> > 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?
>>> 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.
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.
I think it is better to "eat" the packet
as it is done for those transmitted to the HW.
On Fri, 15 Feb 2019 20:38:59 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:
> 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.
> I think it is better to "eat" the packet
> as it is done for those transmitted to the HW.
>
>
First off rte_errno doesn't work for a burst API.
IMHO (which matches /2) all drivers should only increment oerrors for something for
a packet which it could not transmit because of hardware condition (link down etc)
or mbuf which has parameters which can not be handled. In either case, the packet
must be dropped by driver and oerrors incremented. The driver should also maintain
internal stats (available by xstats) for any conditions like this.
When no tx descriptors are available, the driver must not increment any counter
and return partial success to the application. If application then wants to do
back pressure or drop it should keep its own statistics.
This is close to the original model in the Intel drivers, and matches what BSD and
Linux do on the OS level for drivers. Like many driver assumptions the corner
cases were not explicitly documented and new drivers probably don't follow
the same pattern.
> -----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?
Konstantin
>
> On Fri, 15 Feb 2019 20:38:59 +0100
> Thomas Monjalon <thomas@monjalon.net> wrote:
>
> > 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.
> > I think it is better to "eat" the packet
> > as it is done for those transmitted to the HW.
> >
> >
>
> First off rte_errno doesn't work for a burst API.
It doesn't allow to return individual error value for each packet.
Though if we stop after first error happens - it is sufficient.
>
> IMHO (which matches /2) all drivers should only increment oerrors for something for
> a packet which it could not transmit because of hardware condition (link down etc)
It sounds too expensive to check that HW is in healthy state and link is up for every tx_burst
operation.
> or mbuf which has parameters which can not be handled.
Right now it is responsibility of different function - tx_prepare().
> In either case, the packet
> must be dropped by driver and oerrors incremented.
Right now tx_burst() only cares - is there enough free TX descriptors to submit packet or not.
From my perspective - it is better to keep it that way.
If the user would like to drop packets if the link is down,
or if the packet is malformed - there should be no problem to create a wrapper on
top of tx_burst() that would do all these extra checking and packet freeing.
>The driver should also maintain
> internal stats (available by xstats) for any conditions like this.
>
> When no tx descriptors are available, the driver must not increment any counter
> and return partial success to the application. If application then wants to do
> back pressure or drop it should keep its own statistics.
>
> This is close to the original model in the Intel drivers, and matches what BSD and
> Linux do on the OS level for drivers. Like many driver assumptions the corner
> cases were not explicitly documented and new drivers probably don't follow
> the same pattern.
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.
Hi David,
Hello Konstantin,
On Sat, Feb 16, 2019 at 1:50 PM Ananyev, Konstantin <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>> wrote:
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net<mailto:thomas@monjalon.net>]
> Sent: Friday, February 15, 2019 7:39 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>>
> Cc: David Marchand <david.marchand@redhat.com<mailto:david.marchand@redhat.com>>; Richardson, Bruce <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>; dev@dpdk.org<mailto:dev@dpdk.org>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com<mailto:wenzhuo.lu@intel.com>>; Wu, Jingjing <jingjing.wu@intel.com<mailto:jingjing.wu@intel.com>>; Iremonger, Bernard <bernard.iremonger@intel.com<mailto:bernard.iremonger@intel.com>>; Maxime Coquelin
> <mcoqueli@redhat.com<mailto:mcoqueli@redhat.com>>; Yigit, Ferruh <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>; Andrew Rybchenko <arybchenko@solarflare.com<mailto:arybchenko@solarflare.com>>; Wiles, Keith
> <keith.wiles@intel.com<mailto: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<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.
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
@@ -1838,6 +1838,7 @@ struct extmem_param {
total_recv += stats.ipackets;
total_xmit += stats.opackets;
total_rx_dropped += stats.imissed;
+ port->tx_dropped += stats.oerrors;
total_tx_dropped += port->tx_dropped;
total_rx_nombuf += stats.rx_nombuf;