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

[David Harton (dharton)]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Wednesday, August 23, 2017 9:24 AM
> To: David Harton (dharton) <dharton at cisco.com>
> Cc: dev at 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 at monjalon.net]
> > > Sent: Wednesday, August 23, 2017 3:52 AM
> > > To: David Harton (dharton) <dharton at cisco.com>
> > > Cc: dev at 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