From: Dariusz Sosnowski <dsosnowski@nvidia.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>,
Aman Singh <aman.deep.singh@intel.com>,
Yuying Zhang <yuying.zhang@intel.com>,
"NBU-Contact-Thomas Monjalon (EXTERNAL)" <thomas@monjalon.net>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [PATCH v2 3/3] ethdev: document special cases of port start and stop
Date: Mon, 14 Nov 2022 16:12:49 +0000 [thread overview]
Message-ID: <MN0PR12MB61523675B7A5BC9922F71674A4059@MN0PR12MB6152.namprd12.prod.outlook.com> (raw)
In-Reply-To: <2f11dfbb-fa91-6fcc-c88b-8723e6d55753@amd.com>
Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Monday, November 14, 2022 15:08
> To: Dariusz Sosnowski <dsosnowski@nvidia.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> Andrew Rybchenko <andrew.rybchenko@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:
- "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?
> > 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 <dsosnowski@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
next prev parent reply other threads:[~2022-11-14 16:12 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-09 16:31 [PATCH 0/3] " Dariusz Sosnowski
2022-11-09 16:31 ` [PATCH 1/3] net/mlx5: fix log level on failed transfer proxy stop Dariusz Sosnowski
2022-11-09 16:31 ` [PATCH 2/3] doc: document E-Switch limitations with HWS in mlx5 PMD Dariusz Sosnowski
2022-11-09 16:31 ` [PATCH 3/3] ethdev: document special cases of port start and stop Dariusz Sosnowski
2022-11-09 19:06 ` [PATCH v2 0/3] " Dariusz Sosnowski
2022-11-09 19:06 ` [PATCH v2 1/3] net/mlx5: fix log level on failed transfer proxy stop Dariusz Sosnowski
2022-11-14 13:17 ` Slava Ovsiienko
2022-11-09 19:06 ` [PATCH v2 2/3] doc: document E-Switch limitations with HWS in mlx5 PMD Dariusz Sosnowski
2022-11-14 13:18 ` Slava Ovsiienko
2022-11-09 19:06 ` [PATCH v2 3/3] ethdev: document special cases of port start and stop Dariusz Sosnowski
2022-11-14 14:07 ` Ferruh Yigit
2022-11-14 16:12 ` Dariusz Sosnowski [this message]
2022-11-14 17:10 ` Ferruh Yigit
2022-11-14 18:22 ` Dariusz Sosnowski
2022-11-14 18:19 ` [PATCH v3 0/3] " Dariusz Sosnowski
2022-11-14 18:19 ` [PATCH v3 1/3] net/mlx5: fix log level on failed transfer proxy stop Dariusz Sosnowski
2022-11-14 18:19 ` [PATCH v3 2/3] net/mlx5: document E-Switch limitations with HWS in mlx5 PMD Dariusz Sosnowski
2022-11-15 8:49 ` Slava Ovsiienko
2022-11-14 18:19 ` [PATCH v3 3/3] ethdev: document special cases of port start and stop Dariusz Sosnowski
2022-11-14 18:32 ` Ferruh Yigit
2022-11-21 22:53 ` [PATCH v3 0/3] " Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MN0PR12MB61523675B7A5BC9922F71674A4059@MN0PR12MB6152.namprd12.prod.outlook.com \
--to=dsosnowski@nvidia.com \
--cc=aman.deep.singh@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=thomas@monjalon.net \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).