[PATCH 1/2] ethdev: document rte_eth_dev_reset return codes

[PATCH 1/2] ethdev: document rte_eth_dev_reset return codes

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

This new function returns 0 on success and various error codes on
different failures. Attempt to document them.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_ether/rte_ethdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index aaf02b33d..9cdb9724a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2262,6 +2262,14 @@ void rte_eth_dev_close(uint16_t port_id);
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-EINVAL) if port identifier is invalid.
+ *   - (-ENOTSUP) if hardware doesn't support this function.
+ *   - (-EPERM) if not ran from the primary process.
+ *   - (-EIO) if re-initialisation failed.
+ *   - (-ENOMEM) if the reset failed due to OOM.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

[PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

When VF reset will be supported by drivers, the API will most likely
have to return -EAGAIN to avoid blocking when the VF cannot be reset
because the PF is down.
Document it immediately even if it's not yet supported, so that users
and developers can already take into account about this use case, and
thus avoid an API-incompatible change later on.

This is based on real-world production usage and customer escalations,
using earlier patches from Intel.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_ether/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9cdb9724a..126f42d3c 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2270,6 +2270,7 @@ void rte_eth_dev_close(uint16_t port_id);
  *   - (-EPERM) if not ran from the primary process.
  *   - (-EIO) if re-initialisation failed.
  *   - (-ENOMEM) if the reset failed due to OOM.
+ *   - (-EAGAIN) if PF is not up and the reset cannot proceed yet.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

Re: [PATCH 1/2] ethdev: document rte_eth_dev_reset return codes

[Remy Horton]

On 19/10/2017 14:48, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
>
> This new function returns 0 on success and various error codes on
> different failures. Attempt to document them.
>
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> ---
>  lib/librte_ether/rte_ethdev.h | 8 ++++++++
>  1 file changed, 8 insertions(+)

Acked-by: Remy Horton <remy.horton@intel.com>

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Remy Horton]

On 19/10/2017 14:48, luca.boccassi@gmail.com wrote:
> Document it immediately even if it's not yet supported, so that users
> and developers can already take into account about this use case, and
> thus avoid an API-incompatible change later on.

I'm not sure about documenting unimplemented features, as API docs ought 
to describe what the code currently does. Then again reason seems OK and 
I don't think there's hard guidelines on this..


> This is based on real-world production usage and customer escalations,
> using earlier patches from Intel.

Can you give the patchwork link for these patches?


..Remy

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Luca Boccassi]

On Thu, 2017-10-19 at 15:53 +0100, Remy Horton wrote:
> On 19/10/2017 14:48, luca.boccassi@gmail.com wrote:
> > Document it immediately even if it's not yet supported, so that
> > users
> > and developers can already take into account about this use case,
> > and
> > thus avoid an API-incompatible change later on.
> 
> I'm not sure about documenting unimplemented features, as API docs
> ought 
> to describe what the code currently does. Then again reason seems OK
> and 
> I don't think there's hard guidelines on this..

Yeah I realised that, that's why it's a separate patch :-)

I'm just trying to avoid a foreseeable API breakage given we've been
using this API in production.

OTOH there are a few cases where perhaps EAGAIN is already a possible
error code to return - for example where the driver fails to
initialise? For example there's an error path that just returns -1 in
i40evf_dev_init

> > This is based on real-world production usage and customer
> > escalations,
> > using earlier patches from Intel.
> 
> Can you give the patchwork link for these patches?
> 
> 
> ..Remy

Based on these ones:

http://dpdk.org/dev/patchwork/patch/14009/
http://dpdk.org/dev/patchwork/patch/14011/
http://dpdk.org/dev/patchwork/patch/14010/

I had sent some feedback, as especially the ixgbe one was a blocking
call in case the PF was down, which is a deal breaker in most
situations given the API will be called from the controller thread in
most cases.

We've adapted and used these patches with the early rte_eth_dev_reset
for a year in production now, and we had a customer who requested it
since they were running into the problem it solves (PF flaps).

I have adapted them on the latest 17.11 tree and tested with X540
10gbit cards, and it seems to work as before. Should I send an RFC and
CC all of you?

Incidentally, are there specific reasons why the VF functionality was
dropped since the first patches were sent?

-- 
Kind regards,
Luca Boccassi

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Thomas Monjalon]

19/10/2017 16:53, Remy Horton:
> 
> On 19/10/2017 14:48, luca.boccassi@gmail.com wrote:
> > Document it immediately even if it's not yet supported, so that users
> > and developers can already take into account about this use case, and
> > thus avoid an API-incompatible change later on.
> 
> I'm not sure about documenting unimplemented features, as API docs ought 
> to describe what the code currently does. Then again reason seems OK and 
> I don't think there's hard guidelines on this..

An API does not need to be implemented to describe its behaviour.
We just have to agree about which behaviour we want to expect.

It seems this error code is reasonable.
But I am not sure you need to give the details about the cause
of the error. The most important is to advise app writer to retry
resetting later when the device is ready. Isn't it?

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Remy Horton]

On 19/10/2017 17:14, Luca Boccassi wrote:
[..]
> We've adapted and used these patches with the early rte_eth_dev_reset
> for a year in production now, and we had a customer who requested it
> since they were running into the problem it solves (PF flaps).
>
> I have adapted them on the latest 17.11 tree and tested with X540
> 10gbit cards, and it seems to work as before. Should I send an RFC and
> CC all of you?

Since it sounds stable, probably best to post the updated/rebased patch. 
Should get merged as long as nothing breaks.


> Incidentally, are there specific reasons why the VF functionality was
> dropped since the first patches were sent?

I'm personally not sure, but the others should know.

[PATCH v2 1/2] ethdev: document rte_eth_dev_reset return codes

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

This new function returns 0 on success and various error codes on
different failures. Attempt to document them.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_ether/rte_ethdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b773589d6..697f6b2e0 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2269,6 +2269,14 @@ void rte_eth_dev_close(uint16_t port_id);
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-EINVAL) if port identifier is invalid.
+ *   - (-ENOTSUP) if hardware doesn't support this function.
+ *   - (-EPERM) if not ran from the primary process.
+ *   - (-EIO) if re-initialisation failed.
+ *   - (-ENOMEM) if the reset failed due to OOM.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

[PATCH v2 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

When VF reset will be supported by drivers, the API will most likely
have to return -EAGAIN to avoid blocking when the VF cannot be reset
because the PF is down.
Document it immediately even if it's not yet supported, so that users
and developers can already take into account about this use case, and
thus avoid an API-incompatible change later on.

This is based on real-world production usage and customer escalations,
using earlier patches from Intel.

Signed-off-by: Luca Boccassi <bluca@debian.org>
---
 lib/librte_ether/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 697f6b2e0..2a0129a2a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2277,6 +2277,7 @@ void rte_eth_dev_close(uint16_t port_id);
  *   - (-EPERM) if not ran from the primary process.
  *   - (-EIO) if re-initialisation failed.
  *   - (-ENOMEM) if the reset failed due to OOM.
+ *   - (-EAGAIN) if the reset temporarily failed and should be retried later.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Luca Boccassi]

On Tue, 2017-10-24 at 00:11 +0200, Thomas Monjalon wrote:
> 19/10/2017 16:53, Remy Horton:
> > 
> > On 19/10/2017 14:48, luca.boccassi@gmail.com wrote:
> > > Document it immediately even if it's not yet supported, so that
> > > users
> > > and developers can already take into account about this use case,
> > > and
> > > thus avoid an API-incompatible change later on.
> > 
> > I'm not sure about documenting unimplemented features, as API docs
> > ought 
> > to describe what the code currently does. Then again reason seems
> > OK and 
> > I don't think there's hard guidelines on this..
> 
> An API does not need to be implemented to describe its behaviour.
> We just have to agree about which behaviour we want to expect.
> 
> It seems this error code is reasonable.
> But I am not sure you need to give the details about the cause
> of the error. The most important is to advise app writer to retry
> resetting later when the device is ready. Isn't it?

Sure, makes sense, see v2.

-- 
Kind regards,
Luca Boccassi

Re: [PATCH 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Luca Boccassi]

On Tue, 2017-10-24 at 07:18 +0100, Remy Horton wrote:
> On 19/10/2017 17:14, Luca Boccassi wrote:
> [..]
> > We've adapted and used these patches with the early
> > rte_eth_dev_reset
> > for a year in production now, and we had a customer who requested
> > it
> > since they were running into the problem it solves (PF flaps).
> > 
> > I have adapted them on the latest 17.11 tree and tested with X540
> > 10gbit cards, and it seems to work as before. Should I send an RFC
> > and
> > CC all of you?
> 
> Since it sounds stable, probably best to post the updated/rebased
> patch. 
> Should get merged as long as nothing breaks.

I can send an RFC later today.

> 
> > Incidentally, are there specific reasons why the VF functionality
> > was
> > dropped since the first patches were sent?
> 
> I'm personally not sure, but the others should know.

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v2 1/2] ethdev: document rte_eth_dev_reset return codes

[Thomas Monjalon]

24/10/2017 13:03, luca.boccassi@gmail.com:
> From: Luca Boccassi <bluca@debian.org>
> 
> This new function returns 0 on success and various error codes on
> different failures. Attempt to document them.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>

Suggested title: "ethdev: document error codes of reset"

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [PATCH v2 2/2] ethdev: pre-emptively document rte_eth_dev_reset error code

[Thomas Monjalon]

24/10/2017 13:03, luca.boccassi@gmail.com:
> From: Luca Boccassi <bluca@debian.org>
> 
> When VF reset will be supported by drivers, the API will most likely
> have to return -EAGAIN to avoid blocking when the VF cannot be reset
> because the PF is down.
> Document it immediately even if it's not yet supported, so that users
> and developers can already take into account about this use case, and
> thus avoid an API-incompatible change later on.
> 
> This is based on real-world production usage and customer escalations,
> using earlier patches from Intel.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>

Suggested title: "ethdev: document new error code for reset"

Acked-by: Thomas Monjalon <thomas@monjalon.net>

[PATCH v3 1/2] ethdev: document error codes of reset

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

This new function returns 0 on success and various error codes on
different failures. Attempt to document them.

Signed-off-by: Luca Boccassi <bluca@debian.org>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ether/rte_ethdev.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index b773589d6..697f6b2e0 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2269,6 +2269,14 @@ void rte_eth_dev_close(uint16_t port_id);
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-EINVAL) if port identifier is invalid.
+ *   - (-ENOTSUP) if hardware doesn't support this function.
+ *   - (-EPERM) if not ran from the primary process.
+ *   - (-EIO) if re-initialisation failed.
+ *   - (-ENOMEM) if the reset failed due to OOM.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

[PATCH v3 2/2] ethdev: document new error code for reset

[luca.boccassi]

From: Luca Boccassi <bluca@debian.org>

When VF reset will be supported by drivers, the API will most likely
have to return -EAGAIN to avoid blocking when the VF cannot be reset
because the PF is down.
Document it immediately even if it's not yet supported, so that users
and developers can already take into account about this use case, and
thus avoid an API-incompatible change later on.

This is based on real-world production usage and customer escalations,
using earlier patches from Intel.

Signed-off-by: Luca Boccassi <bluca@debian.org>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ether/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 697f6b2e0..2a0129a2a 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -2277,6 +2277,7 @@ void rte_eth_dev_close(uint16_t port_id);
  *   - (-EPERM) if not ran from the primary process.
  *   - (-EIO) if re-initialisation failed.
  *   - (-ENOMEM) if the reset failed due to OOM.
+ *   - (-EAGAIN) if the reset temporarily failed and should be retried later.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
-- 
2.11.0

Re: [PATCH v2 1/2] ethdev: document rte_eth_dev_reset return codes

[Luca Boccassi]

On Tue, 2017-10-24 at 14:27 +0200, Thomas Monjalon wrote:
> 24/10/2017 13:03, luca.boccassi@gmail.com:
> > From: Luca Boccassi <bluca@debian.org>
> > 
> > This new function returns 0 on success and various error codes on
> > different failures. Attempt to document them.
> > 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> 
> Suggested title: "ethdev: document error codes of reset"
> 
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Ok, see v3.

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v3 1/2] ethdev: document error codes of reset

[Ferruh Yigit]

On 10/24/2017 6:19 AM, luca.boccassi@gmail.com wrote:
> From: Luca Boccassi <bluca@debian.org>
> 
> This new function returns 0 on success and various error codes on
> different failures. Attempt to document them.
> 
> Signed-off-by: Luca Boccassi <bluca@debian.org>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Series applied to dpdk-next-net/master, thanks.

Re: [PATCH v3 1/2] ethdev: document error codes of reset

[Luca Boccassi]

On Tue, 2017-10-24 at 13:41 -0700, Ferruh Yigit wrote:
> On 10/24/2017 6:19 AM, luca.boccassi@gmail.com wrote:
> > From: Luca Boccassi <bluca@debian.org>
> > 
> > This new function returns 0 on success and various error codes on
> > different failures. Attempt to document them.
> > 
> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Series applied to dpdk-next-net/master, thanks.

Thanks!

Will it make it into 17.11-rc2?

-- 
Kind regards,
Luca Boccassi

Re: [PATCH v3 1/2] ethdev: document error codes of reset

[Ferruh Yigit]

On 10/25/2017 5:01 AM, Luca Boccassi wrote:
> On Tue, 2017-10-24 at 13:41 -0700, Ferruh Yigit wrote:
>> On 10/24/2017 6:19 AM, luca.boccassi@gmail.com wrote:
>>> From: Luca Boccassi <bluca@debian.org>
>>>
>>> This new function returns 0 on success and various error codes on
>>> different failures. Attempt to document them.
>>>
>>> Signed-off-by: Luca Boccassi <bluca@debian.org>
>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> Series applied to dpdk-next-net/master, thanks.
> 
> Thanks!
> 
> Will it make it into 17.11-rc2?

Most probably, next-net not pulled yet, it will be pulled for rc2. Unless has
been removed while merged into main tree, will be in rc2.