On 11/14/2022 4:12 PM, Dariusz Sosnowski wrote:
Hi Ferruh,
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@amd.com>
Sent: Monday, November 14, 2022 15:08
To: Dariusz Sosnowski <dsosnow...@nvidia.com>; Aman Singh
<aman.deep.si...@intel.com>; Yuying Zhang <yuying.zh...@intel.com>;
NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>;
Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
Cc: dev@dpdk.org
Subject: Re: [PATCH v2 3/3] ethdev: document special cases of port start and
stop
External email: Use caution opening links or attachments
On 11/9/2022 7:06 PM, Dariusz Sosnowski wrote:
This patch clarifies the handling of following cases in the ethdev API
docs:
- If rte_eth_dev_start() returns (-EAGAIN) for some port,
it cannot be started until other port is started.
- If rte_eth_dev_stop() returns (-EBUSY) for some port,
it cannot be stopped until other port is stopped.
EAGAIN and EBUSY has kind of standard meaning, I am not sure if it is good
idea to change this meaning to a specific "dependency to other port" use
case.
Why not keep common generic meanings of the error codes?
In my opinion, using standard error meanings for EAGAIN and EBUSY would make
the returned errors too vague for the API user.
If we used the standard meanings, it's not clear why the port cannot be started
or stopped and what the user should do about it.
Providing specific reasons in the API makes it more understandable. On top of that, they
are "subcases" of standard errors:
I think generic error meaning is not vague, although underlying reason is.
- "Port cannot be stopped because another port depends on it" is a special case of
"Device or resource is busy".
- "Port cannot be started because it depends on other port being started" is a special
case of "Resource temporarily unavailable".
However, I see your concern, so maybe let's do the following. To not limit the
API, we could for example:
- In the documentation of returned values - provide the generic meaning for the
EAGAIN and EBUSY:
- rte_eth_dev_stop(): EBUSY is returned if stopping the port is not
allowed in the current state.
- rte_eth_dev_start(): EAGAIN is returned if start operation must be
retried.
- In the function description provide the specific use case of "dependency on other
port" as an example
of EBUSY/EAGAIN usage
- Depending on the use cases emerging in the future, new examples can be
added,
if EBUSY/EAGAIN is suitable for the new use cases.
What do you think?
OK to above generic error documentation.
And what do you think to document "dependency on other port" in the
driver dev_ops function comment, since it will be an instance of the
generic error message.
When stopping the port in testpmd fails due to (-EBUSY), port's state
is switched back to STARTED to allow users to manually retry stopping
the port.
No additional changes in testpmd are required to handle failure to
start port with (-EAGAIN).
If rte_eth_dev_start() fails, port's state is switched to STOPPED and
users are allowed to retry the operation.
Signed-off-by: Dariusz Sosnowski <dsosnow...@nvidia.com>
---
app/test-pmd/testpmd.c | 10 +++++++++-
lib/ethdev/rte_ethdev.h | 9 +++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
aa7ea29f15..5a69e3c77a 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3118,6 +3118,7 @@ stop_port(portid_t pid)
int need_check_link_status = 0;
portid_t peer_pl[RTE_MAX_ETHPORTS];
int peer_pi;
+ int ret;
if (port_id_is_invalid(pid, ENABLED_WARN))
return;
@@ -3167,9 +3168,16 @@ stop_port(portid_t pid)
if (port->flow_list)
port_flow_flush(pi);
- if (eth_dev_stop_mp(pi) != 0)
+ ret = eth_dev_stop_mp(pi);
+ if (ret != 0) {
RTE_LOG(ERR, EAL, "rte_eth_dev_stop failed for port
%u\n",
pi);
+ if (ret == -EBUSY) {
+ /* Allow to retry stopping the port. */
+ port->port_status = RTE_PORT_STARTED;
If the stop() failed, isn't the current status should be STARTED independent
from the error type?
Correct. I'll update it in v3.
+ continue;
+ }
+ }
if (port->port_status == RTE_PORT_HANDLING)
port->port_status = RTE_PORT_STOPPED; diff --git
a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h index
13fe73d5a3..abf5a24f92 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2701,10 +2701,14 @@ int rte_eth_dev_tx_queue_stop(uint16_t
port_id, uint16_t tx_queue_id);
* On success, all basic functions exported by the Ethernet API (link status,
* receive/transmit, and so on) can be invoked.
*
+ * If the port depends on another one being started,
+ * PMDs might return (-EAGAIN) to notify about such requirement.
+ *
* @param port_id
* The port identifier of the Ethernet device.
* @return
* - 0: Success, Ethernet device started.
+ * - -EAGAIN: If it depends on another port to be started first.
* - <0: Error code of the driver device start function.
*/
int rte_eth_dev_start(uint16_t port_id); @@ -2713,10 +2717,15 @@ int
rte_eth_dev_start(uint16_t port_id);
* Stop an Ethernet device. The device can be restarted with a call to
* rte_eth_dev_start()
*
+ * If the port provides some resources for other ports
+ * and it cannot be stopped before them,
+ * PMDs might return (-EBUSY) to notify about such requirement.
+ *
* @param port_id
* The port identifier of the Ethernet device.
* @return
* - 0: Success, Ethernet device stopped.
+ * - -EBUSY: If it depends on another port to be stopped first.
* - <0: Error code of the driver device stop function.
*/
int rte_eth_dev_stop(uint16_t port_id);
Best regards,
Dariusz Sosnowski
