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

[Thomas Monjalon <thomas@monjalon.net>]

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