[dpdk-dev,v2,1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get

  rte_eth_stats_get() unconditonally would set rx_nombuf
even if the device was setting the value.  A check has
been added in rte_eth_stats_get() to leave the device
value in-tact when non-zero.

Signed-off-by: David Harton <dharton@cisco.com>
---

v2: Fixed braces complaint required by other coding standards.

 lib/librte_ether/rte_ethdev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
  

23/08/2017 04:55, David Harton:
> rte_eth_stats_get() unconditonally would set rx_nombuf
> even if the device was setting the value.  A check has
> been added in rte_eth_stats_get() to leave the device
> value in-tact when non-zero.

If we get this counter from stats->rx_nombuf, why keeping
dev->data->rx_mbuf_alloc_failed ?
We could rework other PMDs to not use this global variable.
It is inconsistent to use it for some PMDs but not all.
And it seems not used outside of PMDs.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, August 23, 2017 3:52 AM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by
> rte_eth_stats_get
> 
> 23/08/2017 04:55, David Harton:
> > rte_eth_stats_get() unconditonally would set rx_nombuf even if the
> > device was setting the value.  A check has been added in
> > rte_eth_stats_get() to leave the device value in-tact when non-zero.
> 
> If we get this counter from stats->rx_nombuf, why keeping
> dev->data->rx_mbuf_alloc_failed ?
> We could rework other PMDs to not use this global variable.
> It is inconsistent to use it for some PMDs but not all.
> And it seems not used outside of PMDs.

Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well since we will have an ABI breakage anyway?

On an somewhat related note, since we are introducing an ABI breakage how do you feel about re-adding the return code for the vlan_offload_set vector?  Some devices conditionally provide the ability to modify some offload and the caller should know.  Since I've got your attention thought I'd ask here before posting the patch.

<soapbox>
In fact, I believe all the API function calls should provide a return code to help mitigate ABI breakages and also provide the ability to let the caller distinguish between - no device, not supported and some other error.  A control plane often needs to understand these distinctions to properly orchestrate the system and/or report real errors.  This is more than I'm willing to take on myself but believe it's a principle I'd like to discuss (can start separate thread if desired).
</soapbox>
  

23/08/2017 14:18, David Harton (dharton):
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Wednesday, August 23, 2017 3:52 AM
> > To: David Harton (dharton) <dharton@cisco.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by
> > rte_eth_stats_get
> > 
> > 23/08/2017 04:55, David Harton:
> > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the
> > > device was setting the value.  A check has been added in
> > > rte_eth_stats_get() to leave the device value in-tact when non-zero.
> > 
> > If we get this counter from stats->rx_nombuf, why keeping
> > dev->data->rx_mbuf_alloc_failed ?
> > We could rework other PMDs to not use this global variable.
> > It is inconsistent to use it for some PMDs but not all.
> > And it seems not used outside of PMDs.
> 
> Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well since we will have an ABI breakage anyway?

Not asking, just giving my thought :)

> On an somewhat related note, since we are introducing an ABI breakage how do you feel about re-adding the return code for the vlan_offload_set vector?  Some devices conditionally provide the ability to modify some offload and the caller should know.  Since I've got your attention thought I'd ask here before posting the patch.

Seems reasonnable

> <soapbox>
> In fact, I believe all the API function calls should provide a return code to help mitigate ABI breakages and also provide the ability to let the caller distinguish between - no device, not supported and some other error.  A control plane often needs to understand these distinctions to properly orchestrate the system and/or report real errors.  This is more than I'm willing to take on myself but believe it's a principle I'd like to discuss (can start separate thread if desired).
> </soapbox>

Yes you're right
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, August 23, 2017 9:24 AM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by
> rte_eth_stats_get
> 
> 23/08/2017 14:18, David Harton (dharton):
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Wednesday, August 23, 2017 3:52 AM
> > > To: David Harton (dharton) <dharton@cisco.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by
> > > rte_eth_stats_get
> > >
> > > 23/08/2017 04:55, David Harton:
> > > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the
> > > > device was setting the value.  A check has been added in
> > > > rte_eth_stats_get() to leave the device value in-tact when non-zero.
> > >
> > > If we get this counter from stats->rx_nombuf, why keeping
> > > dev->data->rx_mbuf_alloc_failed ?
> > > We could rework other PMDs to not use this global variable.
> > > It is inconsistent to use it for some PMDs but not all.
> > > And it seems not used outside of PMDs.
> >
> > Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well
> since we will have an ABI breakage anyway?
> 
> Not asking, just giving my thought :)

I did some more digging.  For this count it looks like some devices:
- have their own internal version
- have a count shared with the pf
- rely on this field to maintain the count
- don't count this failure at all :(

With that said I'd like to keep with the requested changes.

Thoughts?
Dave

> 
> > On an somewhat related note, since we are introducing an ABI breakage
> how do you feel about re-adding the return code for the vlan_offload_set
> vector?  Some devices conditionally provide the ability to modify some
> offload and the caller should know.  Since I've got your attention thought
> I'd ask here before posting the patch.
> 
> Seems reasonnable
> 
> > <soapbox>
> > In fact, I believe all the API function calls should provide a return
> code to help mitigate ABI breakages and also provide the ability to let
> the caller distinguish between - no device, not supported and some other
> error.  A control plane often needs to understand these distinctions to
> properly orchestrate the system and/or report real errors.  This is more
> than I'm willing to take on myself but believe it's a principle I'd like
> to discuss (can start separate thread if desired).
> > </soapbox>
> 
> Yes you're right
  

23/08/2017 23:27, David Harton (dharton):
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 23/08/2017 14:18, David Harton (dharton):
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > 23/08/2017 04:55, David Harton:
> > > > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the
> > > > > device was setting the value.  A check has been added in
> > > > > rte_eth_stats_get() to leave the device value in-tact when non-zero.
> > > >
> > > > If we get this counter from stats->rx_nombuf, why keeping
> > > > dev->data->rx_mbuf_alloc_failed ?
> > > > We could rework other PMDs to not use this global variable.
> > > > It is inconsistent to use it for some PMDs but not all.
> > > > And it seems not used outside of PMDs.
> > >
> > > Are you also asking to remove dev->data->rx_mbuf_alloc_failed as well
> > since we will have an ABI breakage anyway?
> > 
> > Not asking, just giving my thought :)
> 
> I did some more digging.  For this count it looks like some devices:
> - have their own internal version
> - have a count shared with the pf
> - rely on this field to maintain the count
> - don't count this failure at all :(
> 
> With that said I'd like to keep with the requested changes.
> 
> Thoughts?

I don't see how it is a problem for removing dev->data->rx_mbuf_alloc_failed.
If this field is used, we just have to replace it by a PMD internal variable.
Isn't it?
  

On Tue, 22 Aug 2017 22:55:55 -0400
David Harton <dharton@cisco.com> wrote:

> rte_eth_stats_get() unconditonally would set rx_nombuf
> even if the device was setting the value.  A check has
> been added in rte_eth_stats_get() to leave the device
> value in-tact when non-zero.
> 
> Signed-off-by: David Harton <dharton@cisco.com>
> ---
> 
> v2: Fixed braces complaint required by other coding standards.
> 
>  lib/librte_ether/rte_ethdev.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0597641..0a1d3b8 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1336,8 +1336,11 @@ struct rte_eth_dev *
>  	memset(stats, 0, sizeof(*stats));
>  
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
> -	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
>  	(*dev->dev_ops->stats_get)(dev, stats);
> +	/* only set rx_nombuf if not set by the device */
> +	if (!stats->rx_nombuf)
> +		stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> +
>  	return 0;
>  }
>  

This seems backwards. It seems like the original way worked fine.
If device specific code wanted to override rx_nombuf it could do so either
by adding it's additional value or just setting rx_nombuf.

Adding special cases seems like it would start a bad precedent and the
could would end up quite complex as some values had one semantic and others
were only from driver.
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, August 23, 2017 5:57 PM
> To: David Harton (dharton) <dharton@cisco.com>
> Cc: thomas@monjalon.net; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf
> by rte_eth_stats_get
> 
> On Tue, 22 Aug 2017 22:55:55 -0400
> David Harton <dharton@cisco.com> wrote:
> 
> > rte_eth_stats_get() unconditonally would set rx_nombuf even if the
> > device was setting the value.  A check has been added in
> > rte_eth_stats_get() to leave the device value in-tact when non-zero.
> >
> > Signed-off-by: David Harton <dharton@cisco.com>
> > ---
> >
> > v2: Fixed braces complaint required by other coding standards.
> >
> >  lib/librte_ether/rte_ethdev.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 0597641..0a1d3b8 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1336,8 +1336,11 @@ struct rte_eth_dev *
> >  	memset(stats, 0, sizeof(*stats));
> >
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
> > -	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> >  	(*dev->dev_ops->stats_get)(dev, stats);
> > +	/* only set rx_nombuf if not set by the device */
> > +	if (!stats->rx_nombuf)
> > +		stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> > +
> >  	return 0;
> >  }
> >
> 
> This seems backwards. It seems like the original way worked fine.
> If device specific code wanted to override rx_nombuf it could do so either
> by adding it's additional value or just setting rx_nombuf.
> 
> Adding special cases seems like it would start a bad precedent and the
> could would end up quite complex as some values had one semantic and
> others were only from driver.

Eternal apologies.  This is another example of me trying to upstream a fix we've held on to for far too long and not realizing it has been addressed.  I see that this was fixed here:
53ecfa24fbcd17d9855937391ce347f37434fbfa

Again, apologies...I'll be careful publishing any further fixes trying to determine if others have fixed in different ways.

Regards,
Dave