DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
@ 2017-08-23  1:19 David Harton
  2017-08-23  2:55 ` [dpdk-dev] [PATCH v2 1/2] " David Harton
  0 siblings, 1 reply; 9+ messages in thread
From: David Harton @ 2017-08-23  1:19 UTC (permalink / raw)
  To: thomas; +Cc: dev, 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.

Signed-off-by: David Harton <dharton@cisco.com>
---
 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..0319e39 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;
 }
 
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23  1:19 [dpdk-dev] [PATCH] ethdev: stop overriding rx_nombuf by rte_eth_stats_get David Harton
@ 2017-08-23  2:55 ` David Harton
  2017-08-23  7:51   ` Thomas Monjalon
  2017-08-23 21:56   ` Stephen Hemminger
  0 siblings, 2 replies; 9+ messages in thread
From: David Harton @ 2017-08-23  2:55 UTC (permalink / raw)
  To: thomas; +Cc: dev, 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.

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;
 }
 
-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23  2:55 ` [dpdk-dev] [PATCH v2 1/2] " David Harton
@ 2017-08-23  7:51   ` Thomas Monjalon
  2017-08-23 12:18     ` David Harton (dharton)
  2017-08-23 21:56   ` Stephen Hemminger
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2017-08-23  7:51 UTC (permalink / raw)
  To: David Harton; +Cc: dev

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23  7:51   ` Thomas Monjalon
@ 2017-08-23 12:18     ` David Harton (dharton)
  2017-08-23 13:24       ` Thomas Monjalon
  0 siblings, 1 reply; 9+ messages in thread
From: David Harton (dharton) @ 2017-08-23 12:18 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev


> -----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>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23 12:18     ` David Harton (dharton)
@ 2017-08-23 13:24       ` Thomas Monjalon
  2017-08-23 21:27         ` David Harton (dharton)
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2017-08-23 13:24 UTC (permalink / raw)
  To: David Harton (dharton); +Cc: dev

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23 13:24       ` Thomas Monjalon
@ 2017-08-23 21:27         ` David Harton (dharton)
  2017-08-23 21:35           ` Thomas Monjalon
  0 siblings, 1 reply; 9+ messages in thread
From: David Harton (dharton) @ 2017-08-23 21:27 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev



> -----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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23 21:27         ` David Harton (dharton)
@ 2017-08-23 21:35           ` Thomas Monjalon
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2017-08-23 21:35 UTC (permalink / raw)
  To: David Harton (dharton); +Cc: dev

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23  2:55 ` [dpdk-dev] [PATCH v2 1/2] " David Harton
  2017-08-23  7:51   ` Thomas Monjalon
@ 2017-08-23 21:56   ` Stephen Hemminger
  2017-08-23 22:19     ` David Harton (dharton)
  1 sibling, 1 reply; 9+ messages in thread
From: Stephen Hemminger @ 2017-08-23 21:56 UTC (permalink / raw)
  To: David Harton; +Cc: thomas, dev

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get
  2017-08-23 21:56   ` Stephen Hemminger
@ 2017-08-23 22:19     ` David Harton (dharton)
  0 siblings, 0 replies; 9+ messages in thread
From: David Harton (dharton) @ 2017-08-23 22:19 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: thomas, dev



> -----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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-08-23 22:19 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23  1:19 [dpdk-dev] [PATCH] ethdev: stop overriding rx_nombuf by rte_eth_stats_get David Harton
2017-08-23  2:55 ` [dpdk-dev] [PATCH v2 1/2] " David Harton
2017-08-23  7:51   ` Thomas Monjalon
2017-08-23 12:18     ` David Harton (dharton)
2017-08-23 13:24       ` Thomas Monjalon
2017-08-23 21:27         ` David Harton (dharton)
2017-08-23 21:35           ` Thomas Monjalon
2017-08-23 21:56   ` Stephen Hemminger
2017-08-23 22:19     ` David Harton (dharton)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).