ethdev: claim device reset as async
Checks
Commit Message
Device reset should be implemented in an async way since it is
possible to be invoked in interrupt thread and sometimes to reset a
device need to wait for some dependency, for example, a VF expects for
PF ready or a NIC function as part of a SOC wait for the whole system
reset complete, and all these time-consuming tasks will block the
interrupt thread.
The patch rename rte_eth_dev_reset to rte_eth_dev_reset_async and
rework the implementation. It will spawn a new thread which will call
ops->dev_reset, and when finished it will raise the event
RTE_ETH_EVENT_RESET_COMPLETE. The application should always wait for
this event before it continues to configure and restart the device.
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
RFC3 -> v1
- fix state check in rte_eth_find_next and rte_eth_find_next_owned_by
- fix compile error in netvsc
- update document and release notes
- improve comments and couple typo fix
app/test-pmd/testpmd.c | 50 +++++++++++++++++++++++++++-
doc/guides/prog_guide/poll_mode_drv.rst | 34 ++++++++++++-------
doc/guides/rel_notes/release_19_05.rst | 6 ++++
drivers/net/ixgbe/ixgbe_ethdev.c | 4 +--
drivers/net/netvsc/hn_var.h | 2 +-
drivers/net/netvsc/hn_vf.c | 4 +--
lib/librte_ethdev/rte_ethdev.c | 56 +++++++++++++++++++++++++++++---
lib/librte_ethdev/rte_ethdev.h | 50 +++++++++++++++++-----------
lib/librte_ethdev/rte_ethdev_version.map | 7 ++++
9 files changed, 171 insertions(+), 42 deletions(-)
Comments
On Wed, 20 Mar 2019 12:54:03 +0800
Qi Zhang <qi.z.zhang@intel.com> wrote:
> + ret = rte_ctrl_thread_create(&tid, pthread_name, NULL,
> + do_dev_reset, args);
> +
> + return ret;
combine the ret = and return (ie make it a tail call)
Hi,
You forgot to Cc Andrew, co-maintainer of ethdev.
20/03/2019 05:54, Qi Zhang:
> Device reset should be implemented in an async way since it is
> possible to be invoked in interrupt thread and sometimes to reset a
> device need to wait for some dependency, for example, a VF expects for
> PF ready or a NIC function as part of a SOC wait for the whole system
> reset complete, and all these time-consuming tasks will block the
> interrupt thread.
> The patch rename rte_eth_dev_reset to rte_eth_dev_reset_async and
> rework the implementation. It will spawn a new thread which will call
> ops->dev_reset, and when finished it will raise the event
> RTE_ETH_EVENT_RESET_COMPLETE. The application should always wait for
> this event before it continues to configure and restart the device.
>
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> - * When this function is called, it first stops the port and then calls the
> - * PMD specific dev_uninit( ) and dev_init( ) to return the port to initial
> - * state, in which no Tx and Rx queues are setup, as if the port has been
> - * reset and not started. The port keeps the port id it had before the
> - * function call.
> - *
> - * After calling rte_eth_dev_reset( ), the application should use
> - * rte_eth_dev_configure( ), rte_eth_rx_queue_setup( ),
> - * rte_eth_tx_queue_setup( ), and rte_eth_dev_start( )
> - * to reconfigure the device as appropriate.
> - *
> - * Note: To avoid unexpected behavior, the application should stop calling
> - * Tx and Rx functions before calling rte_eth_dev_reset( ). For thread
> - * safety, all these controlling functions should be called from the same
> - * thread.
> + * @note
> + * Device reset may have the dependency, for example, a VF reset expects
> + * PF ready, or a NIC function as a part of a SOC need to wait for other
> + * parts of the system be ready, these are time-consuming tasks and will
> + * block current thread.
> + *
> + * As the name, rte_eth_dev_reset_async is an async API, it will spwan a
> + * new thread to call ops->dev_reset, once it is finished, it will raise
> + * the RTE_ETH_EVENT_RESET_COMPLETE event to notify application. That makes
> + * things easy for an application that want to reset the device from the
> + * interrupt thread since typically a RTE_ETH_EVENT_INTR_RESET handler is
> + * invoked in interrupt thread. The typical implementation of ops->dev_reset
> + * will do some hardware reset operations through calling dev_uninit() and
> + * dev_init().
> + *
> + * Application should not assume device reset is finished after
> + * rte_eth_dev_reset_async return, it should always wait for a
> + * RTE_ETH_EVENT_RESET_COMPLETE event and check the reset result.
> + * If reset success, application should call rte_eth_dev_configure( ),
> + * rte_eth_rx_queue_setup( ), rte_eth_tx_queue_setup( ),
> + * and rte_eth_dev_start( ) to reconfigure the device as appropriate.
> + *
> + * @Note
> + * To avoid unexpected behavior, the application should stop calling
> + * Tx and Rx functions before calling rte_eth_dev_reset_async( ).
> *
> * @param port_id
> * The port identifier of the Ethernet device.
> @@ -1880,12 +1892,10 @@ void rte_eth_dev_close(uint16_t port_id);
> * - (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 or device is removed.
> * - (-ENOMEM) if the reset failed due to OOM.
> - * - (-EAGAIN) if the reset temporarily failed and should be retried later.
> + * - (<0) other errors from low level driver.
> */
> -int rte_eth_dev_reset(uint16_t port_id);
> +int rte_eth_dev_reset_async(uint16_t port_id);
Sorry I didn't check whether this API is better or not,
but I know it cannot be accepted before proposing a deprecation notice.
Perhaps you may keep the old API and just add the new one.
Honestly, I never really agreed with the purpose of the reset API.
So making it async or not, I have no real opinion...
... but spawning a new thread at each function call, I feel it is bad.
On 4/5/19 1:01 AM, Thomas Monjalon wrote:
> Hi,
>
> You forgot to Cc Andrew, co-maintainer of ethdev.
>
> 20/03/2019 05:54, Qi Zhang:
>> Device reset should be implemented in an async way since it is
>> possible to be invoked in interrupt thread and sometimes to reset a
>> device need to wait for some dependency, for example, a VF expects for
>> PF ready or a NIC function as part of a SOC wait for the whole system
>> reset complete, and all these time-consuming tasks will block the
>> interrupt thread.
>> The patch rename rte_eth_dev_reset to rte_eth_dev_reset_async and
>> rework the implementation. It will spawn a new thread which will call
>> ops->dev_reset, and when finished it will raise the event
>> RTE_ETH_EVENT_RESET_COMPLETE. The application should always wait for
>> this event before it continues to configure and restart the device.
>>
>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>> ---
>> - * When this function is called, it first stops the port and then calls the
>> - * PMD specific dev_uninit( ) and dev_init( ) to return the port to initial
>> - * state, in which no Tx and Rx queues are setup, as if the port has been
>> - * reset and not started. The port keeps the port id it had before the
>> - * function call.
>> - *
>> - * After calling rte_eth_dev_reset( ), the application should use
>> - * rte_eth_dev_configure( ), rte_eth_rx_queue_setup( ),
>> - * rte_eth_tx_queue_setup( ), and rte_eth_dev_start( )
>> - * to reconfigure the device as appropriate.
>> - *
>> - * Note: To avoid unexpected behavior, the application should stop calling
>> - * Tx and Rx functions before calling rte_eth_dev_reset( ). For thread
>> - * safety, all these controlling functions should be called from the same
>> - * thread.
>> + * @note
>> + * Device reset may have the dependency, for example, a VF reset expects
>> + * PF ready, or a NIC function as a part of a SOC need to wait for other
>> + * parts of the system be ready, these are time-consuming tasks and will
>> + * block current thread.
>> + *
>> + * As the name, rte_eth_dev_reset_async is an async API, it will spwan a
>> + * new thread to call ops->dev_reset, once it is finished, it will raise
>> + * the RTE_ETH_EVENT_RESET_COMPLETE event to notify application. That makes
>> + * things easy for an application that want to reset the device from the
>> + * interrupt thread since typically a RTE_ETH_EVENT_INTR_RESET handler is
>> + * invoked in interrupt thread. The typical implementation of ops->dev_reset
>> + * will do some hardware reset operations through calling dev_uninit() and
>> + * dev_init().
>> + *
>> + * Application should not assume device reset is finished after
>> + * rte_eth_dev_reset_async return, it should always wait for a
>> + * RTE_ETH_EVENT_RESET_COMPLETE event and check the reset result.
>> + * If reset success, application should call rte_eth_dev_configure( ),
>> + * rte_eth_rx_queue_setup( ), rte_eth_tx_queue_setup( ),
>> + * and rte_eth_dev_start( ) to reconfigure the device as appropriate.
>> + *
>> + * @Note
>> + * To avoid unexpected behavior, the application should stop calling
>> + * Tx and Rx functions before calling rte_eth_dev_reset_async( ).
>> *
>> * @param port_id
>> * The port identifier of the Ethernet device.
>> @@ -1880,12 +1892,10 @@ void rte_eth_dev_close(uint16_t port_id);
>> * - (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 or device is removed.
>> * - (-ENOMEM) if the reset failed due to OOM.
>> - * - (-EAGAIN) if the reset temporarily failed and should be retried later.
>> + * - (<0) other errors from low level driver.
>> */
>> -int rte_eth_dev_reset(uint16_t port_id);
>> +int rte_eth_dev_reset_async(uint16_t port_id);
> Sorry I didn't check whether this API is better or not,
> but I know it cannot be accepted before proposing a deprecation notice.
> Perhaps you may keep the old API and just add the new one.
>
> Honestly, I never really agreed with the purpose of the reset API.
> So making it async or not, I have no real opinion...
> ... but spawning a new thread at each function call, I feel it is bad.
I agree with Thomas.
It looks very dangerous from locking point of view when a PMD
callback is executed from dynamically created thread.
@@ -2232,6 +2232,54 @@ close_port(portid_t pid)
printf("Done\n");
}
+static int reset_ret;
+static int reset_done;
+
+static int
+on_reset_complete(__rte_unused uint16_t port_id,
+ __rte_unused enum rte_eth_event_type event,
+ __rte_unused void *cb_arg,
+ void *ret_param)
+{
+ RTE_ASSERT(event == RTE_ETH_EVENT_RESET_COMPLETE);
+
+ reset_ret = *(int *)ret_param;
+ reset_done = 1;
+ return 0;
+}
+
+static int
+do_dev_reset_sync(portid_t pid)
+{
+ int ret;
+
+ ret = rte_eth_dev_callback_register(pid,
+ RTE_ETH_EVENT_RESET_COMPLETE,
+ on_reset_complete, NULL);
+
+ if (ret) {
+ printf("Fail to reigster callback function\n");
+ return ret;
+ }
+
+ reset_done = 0;
+ ret = rte_eth_dev_reset_async(pid);
+ if (ret)
+ goto finish;
+
+ while (!reset_done)
+ rte_delay_ms(10);
+
+ ret = reset_ret;
+
+finish:
+ rte_eth_dev_callback_unregister(pid,
+ RTE_ETH_EVENT_RESET_COMPLETE,
+ on_reset_complete, NULL);
+
+ return ret;
+}
+
void
reset_port(portid_t pid)
{
@@ -2260,7 +2308,7 @@ reset_port(portid_t pid)
continue;
}
- diag = rte_eth_dev_reset(pi);
+ diag = do_dev_reset_sync(pi);
if (diag == 0) {
port = &ports[pi];
port->need_reconfig = 1;
@@ -581,7 +581,7 @@ NIC Reset API
.. code-block:: c
- int rte_eth_dev_reset(uint16_t port_id);
+ int rte_eth_dev_reset_sync(uint16_t port_id);
Sometimes a port has to be reset passively. For example when a PF is
reset, all its VFs should also be reset by the application to make them
@@ -594,7 +594,7 @@ the application should register a callback function to handle these
events. When a PMD needs to trigger a reset, it can trigger an
RTE_ETH_EVENT_INTR_RESET event. On receiving an RTE_ETH_EVENT_INTR_RESET
event, applications can handle it as follows: Stop working queues, stop
-calling Rx and Tx functions, and then call rte_eth_dev_reset(). For
+calling Rx and Tx functions, and then call rte_eth_dev_reset_sync(). For
thread safety all these operations should be called from the same thread.
For example when PF is reset, the PF sends a message to notify VFs of
@@ -605,13 +605,23 @@ This means that a PF reset triggers an RTE_ETH_EVENT_INTR_RESET
event within VFs. The function _rte_eth_dev_callback_process() will
call the registered callback function. The callback function can trigger
the application to handle all operations the VF reset requires including
-stopping Rx/Tx queues and calling rte_eth_dev_reset().
-
-The rte_eth_dev_reset() itself is a generic function which only does
-some hardware reset operations through calling dev_unint() and
-dev_init(), and itself does not handle synchronization, which is handled
-by application.
-
-The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
-the application to handle reset event. It is duty of application to
-handle all synchronization before it calls rte_eth_dev_reset().
+stopping Rx/Tx queues and calling rte_eth_dev_reset_async().
+
+As the name, rte_eth_dev_reset_async is an async API, it will spwan a
+new thread to call ops->dev_reset, once it is finished, it will raise
+the RTE_ETH_EVENT_RESET_COMPLETE event to notify application. That makes
+things easy for an application that want to reset the device from the
+interrupt thread since typically a RTE_ETH_EVENT_INTR_RESET handler is
+invoked in interrupt thread. The typical implementation of ops->dev_reset
+will do some hardware reset operations through calling dev_uninit() and
+dev_init().
+
+Application should not assume device reset is finished after
+rte_eth_dev_reset_async return, it should always wait for a
+RTE_ETH_EVENT_RESET_COMPLETE event and check the reset result.
+If reset success, application should call rte_eth_dev_configure(),
+rte_eth_rx_queue_setup(), rte_eth_tx_queue_setup(),
+and rte_eth_dev_start() to reconfigure the device as appropriate.
+
+To avoid unexpected behavior, the application should stop calling
+Tx and Rx functions before calling rte_eth_dev_reset_async().
@@ -91,6 +91,12 @@ New Features
* Added promiscuous mode support.
+* **Claim device reset as async API.**
+
+ rte_eth_dev_reset is replaced by rte_eth_dev_reset_async which is implemented
+ as an async API. Application should wait for RTE_ETH_EVENT_RESET_COMPLETE
+ event after invoke the API before continues to configure and restart the
+ device.
Removed Items
-------------
@@ -1631,8 +1631,8 @@ eth_ixgbevf_dev_init(struct rte_eth_dev *eth_dev)
PMD_INIT_LOG(ERR, "VF Initialization Failure: %d", diag);
/*
* This error code will be propagated to the app by
- * rte_eth_dev_reset, so use a public error code rather than
- * the internal-only IXGBE_ERR_RESET_FAILED
+ * rte_eth_dev_reset_async, so use a public error code rather
+ * than the internal-only IXGBE_ERR_RESET_FAILED
*/
return -EAGAIN;
}
@@ -177,7 +177,7 @@ int hn_vf_configure(struct rte_eth_dev *dev,
const struct rte_eth_conf *dev_conf);
const uint32_t *hn_vf_supported_ptypes(struct rte_eth_dev *dev);
int hn_vf_start(struct rte_eth_dev *dev);
-void hn_vf_reset(struct rte_eth_dev *dev);
+void hn_vf_reset_async(struct rte_eth_dev *dev);
void hn_vf_stop(struct rte_eth_dev *dev);
void hn_vf_close(struct rte_eth_dev *dev);
@@ -358,9 +358,9 @@ void hn_vf_stop(struct rte_eth_dev *dev)
rte_spinlock_unlock(&hv->vf_lock); \
}
-void hn_vf_reset(struct rte_eth_dev *dev)
+void hn_vf_reset_async(struct rte_eth_dev *dev)
{
- VF_ETHDEV_FUNC(dev, rte_eth_dev_reset);
+ VF_ETHDEV_FUNC(dev, rte_eth_dev_reset_async);
}
void hn_vf_close(struct rte_eth_dev *dev)
@@ -331,7 +331,8 @@ rte_eth_find_next(uint16_t port_id)
{
while (port_id < RTE_MAX_ETHPORTS &&
rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
- rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED)
+ rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED &&
+ rte_eth_devices[port_id].state != RTE_ETH_DEV_RESETTING)
port_id++;
if (port_id >= RTE_MAX_ETHPORTS)
@@ -556,7 +557,8 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
{
while (port_id < RTE_MAX_ETHPORTS &&
((rte_eth_devices[port_id].state != RTE_ETH_DEV_ATTACHED &&
- rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED) ||
+ rte_eth_devices[port_id].state != RTE_ETH_DEV_REMOVED &&
+ rte_eth_devices[port_id].state != RTE_ETH_DEV_RESETTING) ||
rte_eth_devices[port_id].data->owner.id != owner_id))
port_id++;
@@ -1490,10 +1492,38 @@ rte_eth_dev_close(uint16_t port_id)
dev->data->tx_queues = NULL;
}
+struct dev_reset_args {
+ struct rte_eth_dev *dev;
+ enum rte_eth_dev_state pre_state;
+};
+
+static void *
+do_dev_reset(void *args)
+{
+ struct dev_reset_args *reset_args = args;
+ struct rte_eth_dev *dev = reset_args->dev;
+ int ret;
+
+ ret = dev->dev_ops->dev_reset(dev);
+
+ /*restore previous device state */
+ dev->state = reset_args->pre_state;
+
+ /* notify users */
+ _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_RESET_COMPLETE,
+ &ret);
+
+ free(args);
+ return NULL;
+}
+
int
-rte_eth_dev_reset(uint16_t port_id)
+rte_eth_dev_reset_async(uint16_t port_id)
{
struct rte_eth_dev *dev;
+ struct dev_reset_args *args;
+ char pthread_name[20];
+ pthread_t tid;
int ret;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1501,10 +1531,26 @@ rte_eth_dev_reset(uint16_t port_id)
RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_reset, -ENOTSUP);
+ /* already on resetting */
+ if (dev->state == RTE_ETH_DEV_RESETTING)
+ return 0;
+
+ args = calloc(1, sizeof(struct dev_reset_args));
+ if (!args)
+ return -ENOMEM;
+
rte_eth_dev_stop(port_id);
- ret = dev->dev_ops->dev_reset(dev);
- return eth_err(port_id, ret);
+ /* store previous device state temporary */
+ args->pre_state = dev->state;
+
+ dev->state = RTE_ETH_DEV_RESETTING;
+ snprintf(pthread_name, sizeof(pthread_name),
+ "do_dev_reset_%d", port_id);
+ ret = rte_ctrl_thread_create(&tid, pthread_name, NULL,
+ do_dev_reset, args);
+
+ return ret;
}
int __rte_experimental
@@ -1308,6 +1308,8 @@ enum rte_eth_dev_state {
RTE_ETH_DEV_ATTACHED,
/** Device is in removed state when plug-out is detected. */
RTE_ETH_DEV_REMOVED,
+ /** Device is on resetting when rte_eth_dev_reset_async is called. */
+ RTE_ETH_DEV_RESETTING,
};
struct rte_eth_dev_sriov {
@@ -1857,21 +1859,31 @@ void rte_eth_dev_close(uint16_t port_id);
* RTE_ETH_EVENT_INTR_RESET event is detected, but can also use it to start
* a port reset in other circumstances.
*
- * When this function is called, it first stops the port and then calls the
- * PMD specific dev_uninit( ) and dev_init( ) to return the port to initial
- * state, in which no Tx and Rx queues are setup, as if the port has been
- * reset and not started. The port keeps the port id it had before the
- * function call.
- *
- * After calling rte_eth_dev_reset( ), the application should use
- * rte_eth_dev_configure( ), rte_eth_rx_queue_setup( ),
- * rte_eth_tx_queue_setup( ), and rte_eth_dev_start( )
- * to reconfigure the device as appropriate.
- *
- * Note: To avoid unexpected behavior, the application should stop calling
- * Tx and Rx functions before calling rte_eth_dev_reset( ). For thread
- * safety, all these controlling functions should be called from the same
- * thread.
+ * @note
+ * Device reset may have the dependency, for example, a VF reset expects
+ * PF ready, or a NIC function as a part of a SOC need to wait for other
+ * parts of the system be ready, these are time-consuming tasks and will
+ * block current thread.
+ *
+ * As the name, rte_eth_dev_reset_async is an async API, it will spwan a
+ * new thread to call ops->dev_reset, once it is finished, it will raise
+ * the RTE_ETH_EVENT_RESET_COMPLETE event to notify application. That makes
+ * things easy for an application that want to reset the device from the
+ * interrupt thread since typically a RTE_ETH_EVENT_INTR_RESET handler is
+ * invoked in interrupt thread. The typical implementation of ops->dev_reset
+ * will do some hardware reset operations through calling dev_uninit() and
+ * dev_init().
+ *
+ * Application should not assume device reset is finished after
+ * rte_eth_dev_reset_async return, it should always wait for a
+ * RTE_ETH_EVENT_RESET_COMPLETE event and check the reset result.
+ * If reset success, application should call rte_eth_dev_configure( ),
+ * rte_eth_rx_queue_setup( ), rte_eth_tx_queue_setup( ),
+ * and rte_eth_dev_start( ) to reconfigure the device as appropriate.
+ *
+ * @Note
+ * To avoid unexpected behavior, the application should stop calling
+ * Tx and Rx functions before calling rte_eth_dev_reset_async( ).
*
* @param port_id
* The port identifier of the Ethernet device.
@@ -1880,12 +1892,10 @@ void rte_eth_dev_close(uint16_t port_id);
* - (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 or device is removed.
* - (-ENOMEM) if the reset failed due to OOM.
- * - (-EAGAIN) if the reset temporarily failed and should be retried later.
+ * - (<0) other errors from low level driver.
*/
-int rte_eth_dev_reset(uint16_t port_id);
+int rte_eth_dev_reset_async(uint16_t port_id);
/**
* Enable receipt in promiscuous mode for an Ethernet device.
@@ -2617,6 +2627,8 @@ enum rte_eth_event_type {
/**< queue state event (enabled/disabled) */
RTE_ETH_EVENT_INTR_RESET,
/**< reset interrupt event, sent to VF on PF reset */
+ RTE_ETH_EVENT_RESET_COMPLETE,
+ /**< inform application that reset is completed */
RTE_ETH_EVENT_VF_MBOX, /**< message from the VF received by PF */
RTE_ETH_EVENT_MACSEC, /**< MACsec offload related event */
RTE_ETH_EVENT_INTR_RMV, /**< device removal event */
@@ -229,6 +229,13 @@ DPDK_18.11 {
} DPDK_18.08;
+DPDK_19.05 {
+ global:
+
+ rte_eth_dev_reset_async;
+
+} DPDK_18.08;
+
EXPERIMENTAL {
global: