[dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Qi Zhang]

rte_eth_dev_reset should be implemented in an async way since it is
possible 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, all these time consuming task will block the
the interrupt thread.
The patch claims rte_eth_dev_reset is an async function and introduce
a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this event
when finish reset in background. The applicaiton should always wait
for this event before continue to configure and restart the device.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.h | 48 ++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 7070e9ab4..541b5161d 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -1814,21 +1814,34 @@ 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.
+ *
+ * So we claimed rte_eth_dev_reset as an async API, that makes things easy
+ * for an application that what to reset the device from the interrupt
+ * thread since typically a RTE_ETH_EVENT_INTR_RESET handler is invoked in
+ * interrupt thread.
+ *
+ * PMD is responsrible to implement ops->dev_reset in an async way, it can
+ * offload the whole task into a separate thread, or maybe just pending on
+ * hardware interrupt as reset dependency ready or start a timely alarm
+ * to poll register status as a background daemon. PMD is also responsible
+ * to raise the RTE_ETH_EVENT_RESET_COMPLETE event to notify the application
+ * when reset is complete.
+ *
+ * Application should not assume device reset is finished after
+ * rte_eth_dev_reset 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( ).
  *
  * @param port_id
  *   The port identifier of the Ethernet device.
@@ -1838,9 +1851,6 @@ void rte_eth_dev_close(uint16_t port_id);
  *   - (-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.
  */
 int rte_eth_dev_reset(uint16_t port_id);
 
@@ -2574,6 +2584,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 */
-- 
2.13.6

[dpdk-dev] [RFC 2/4] net/i40e: enable async device reset

[Qi Zhang]

Handle device reset in a separate thread and raise
RTE_ETH_EVENT_RESET_COMPLETE event when it is done.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c    | 28 +++++++++++++++++++++++-----
 drivers/net/i40e/i40e_ethdev_vf.c | 28 +++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 85a6a867f..611570159 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2439,12 +2439,29 @@ i40e_dev_close(struct rte_eth_dev *dev)
 	I40E_WRITE_FLUSH(hw);
 }
 
+static void *
+do_dev_reset(void *args)
+{
+	struct rte_eth_dev *dev = args;
+	int ret;
+
+	ret = eth_i40e_dev_uninit(dev);
+	if (!ret)
+		ret = eth_i40e_dev_init(dev, NULL);
+
+	_rte_eth_dev_callback_process(dev,
+			RTE_ETH_EVENT_RESET_COMPLETE,
+			&ret);
+	return NULL;
+}
+
 /*
  * Reset PF device only to re-initialize resources in PMD layer
  */
 static int
 i40e_dev_reset(struct rte_eth_dev *dev)
 {
+	pthread_t tid;
 	int ret;
 
 	/* When a DPDK PMD PF begin to reset PF port, it should notify all
@@ -2456,11 +2473,12 @@ i40e_dev_reset(struct rte_eth_dev *dev)
 	if (dev->data->sriov.active)
 		return -ENOTSUP;
 
-	ret = eth_i40e_dev_uninit(dev);
-	if (ret)
-		return ret;
-
-	ret = eth_i40e_dev_init(dev, NULL);
+	/**
+	 * Since dev_reset should be implemented as async, do
+	 * reset in a separate thread.
+	 */
+	ret = rte_ctrl_thread_create(&tid, "i40e_dev_reset",
+			NULL, do_dev_reset, dev);
 
 	return ret;
 }
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index 001c301b9..e10e81792 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -2284,19 +2284,37 @@ i40evf_dev_close(struct rte_eth_dev *dev)
 	i40evf_disable_irq0(hw);
 }
 
+static void *
+do_dev_reset(void *args)
+{
+	struct rte_eth_dev *dev = args;
+	int ret;
+
+	ret = i40evf_dev_uninit(dev);
+	if (!ret)
+		ret = i40evf_dev_init(dev);
+
+	_rte_eth_dev_callback_process(dev,
+			RTE_ETH_EVENT_RESET_COMPLETE,
+			&ret);
+	return NULL;
+}
+
 /*
  * Reset VF device only to re-initialize resources in PMD layer
  */
 static int
 i40evf_dev_reset(struct rte_eth_dev *dev)
 {
+	pthread_t tid;
 	int ret;
 
-	ret = i40evf_dev_uninit(dev);
-	if (ret)
-		return ret;
-
-	ret = i40evf_dev_init(dev);
+	/**
+	 * Since dev_reset should be implemented as async, do
+	 * reset in a separate thread.
+	 */
+	ret = rte_ctrl_thread_create(&tid, "i40evf_dev_reset",
+				NULL, do_dev_reset, dev);
 
 	return ret;
 }
-- 
2.13.6

[dpdk-dev] [RFC 3/4] net/ixgbe: enable async device reset

[Qi Zhang]

Handle device reset in a separate thread and raise
RTE_ETH_EVENT_RESET_COMPLETE event when it is done.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 48 +++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index 26b192737..6e2d6fc1a 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2923,12 +2923,29 @@ ixgbe_dev_close(struct rte_eth_dev *dev)
 	ixgbe_set_rar(hw, 0, hw->mac.addr, 0, IXGBE_RAH_AV);
 }
 
+static void *
+do_dev_reset(void *args)
+{
+	struct rte_eth_dev *dev = args;
+	int ret;
+
+	ret = eth_ixgbe_dev_uninit(dev);
+	if (!ret)
+		ret = eth_ixgbe_dev_init(dev, NULL);
+
+	_rte_eth_dev_callback_process(dev,
+			RTE_ETH_EVENT_RESET_COMPLETE,
+			&ret);
+	return NULL;
+}
+
 /*
  * Reset PF device.
  */
 static int
 ixgbe_dev_reset(struct rte_eth_dev *dev)
 {
+	pthread_t tid;
 	int ret;
 
 	/* When a DPDK PMD PF begin to reset PF port, it should notify all
@@ -2940,11 +2957,8 @@ ixgbe_dev_reset(struct rte_eth_dev *dev)
 	if (dev->data->sriov.active)
 		return -ENOTSUP;
 
-	ret = eth_ixgbe_dev_uninit(dev);
-	if (ret)
-		return ret;
-
-	ret = eth_ixgbe_dev_init(dev, NULL);
+	ret = rte_ctrl_thread_create(&tid, "ixgbe_dev_reset",
+				NULL, do_dev_reset, dev);
 
 	return ret;
 }
@@ -5173,19 +5187,33 @@ ixgbevf_dev_close(struct rte_eth_dev *dev)
 	ixgbevf_remove_mac_addr(dev, 0);
 }
 
+static void *
+do_dev_vf_reset(void *args)
+{
+	struct rte_eth_dev *dev = args;
+	int ret;
+
+	ret = eth_ixgbevf_dev_uninit(dev);
+	if (!ret)
+		ret = eth_ixgbevf_dev_init(dev);
+
+	_rte_eth_dev_callback_process(dev,
+			RTE_ETH_EVENT_RESET_COMPLETE,
+			&ret);
+	return NULL;
+}
+
 /*
  * Reset VF device
  */
 static int
 ixgbevf_dev_reset(struct rte_eth_dev *dev)
 {
+	pthread_t tid;
 	int ret;
 
-	ret = eth_ixgbevf_dev_uninit(dev);
-	if (ret)
-		return ret;
-
-	ret = eth_ixgbevf_dev_init(dev);
+	ret = rte_ctrl_thread_create(&tid, "ixgbevf_dev_reset",
+				NULL, do_dev_vf_reset, dev);
 
 	return ret;
 }
-- 
2.13.6

[dpdk-dev] [RFC 4/4] testpmd: enable async device reset

[Qi Zhang]

rte_eth_dev_reset is claimed as an async API, so re-work
on the device reset handling.

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 app/test-pmd/testpmd.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index ee48db2a3..24d5c5d9c 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1918,6 +1918,59 @@ close_port(portid_t pid)
 	printf("Done\n");
 }
 
+static pthread_mutex_t dev_reset_lock;
+static pthread_cond_t dev_reset_cond;
+static int reset_status;
+
+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);
+
+	pthread_cond_broadcast(&dev_reset_cond);
+
+	reset_status = *(int *)ret_param;
+	return 0;
+}
+
+static int
+do_dev_reset_sync(portid_t pid)
+{
+	int ret;
+
+	pthread_mutex_init(&dev_reset_lock, NULL);
+	pthread_cond_init(&dev_reset_cond, NULL);
+
+	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;
+	}
+
+	ret = rte_eth_dev_reset(pid);
+	if (ret)
+		goto finish;
+
+	pthread_mutex_lock(&dev_reset_lock);
+	pthread_cond_wait(&dev_reset_cond, &dev_reset_lock);
+	pthread_mutex_unlock(&dev_reset_lock);
+
+	ret = reset_status;
+
+finish:
+	rte_eth_dev_callback_unregister(pid,
+			RTE_ETH_EVENT_RESET_COMPLETE,
+			on_reset_complete, NULL);
+
+	return ret;
+}
+
 void
 reset_port(portid_t pid)
 {
@@ -1946,7 +1999,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;
-- 
2.13.6

Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Kevin Traynor]

On 08/08/2018 08:00 AM, Qi Zhang wrote:
> rte_eth_dev_reset should be implemented in an async way since it is
> possible 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, all these time consuming task will block the
> the interrupt thread.
> The patch claims rte_eth_dev_reset is an async function and introduce
> a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this event
> when finish reset in background. The applicaiton should always wait
> for this event before continue to configure and restart the device.
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 48 ++++++++++++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 7070e9ab4..541b5161d 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1814,21 +1814,34 @@ 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.
> + *
> + * So we claimed rte_eth_dev_reset as an async API, that makes things easy
> + * for an application that what to reset the device from the interrupt
> + * thread since typically a RTE_ETH_EVENT_INTR_RESET handler is invoked in
> + * interrupt thread.
> + *
> + * PMD is responsrible to implement ops->dev_reset in an async way, it can
> + * offload the whole task into a separate thread, or maybe just pending on
> + * hardware interrupt as reset dependency ready or start a timely alarm
> + * to poll register status as a background daemon. PMD is also responsible
> + * to raise the RTE_ETH_EVENT_RESET_COMPLETE event to notify the application
> + * when reset is complete.
> + *
> + * Application should not assume device reset is finished after
> + * rte_eth_dev_reset 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.
> + *

If you intend to make this change for 18.11, then I think you need to
document it as part of 18.08. The function signature isn't changing, but
the meaning of success is entirely different now.

> + * @Note
> + * To avoid unexpected behavior, the application should stop calling
> + * Tx and Rx functions before calling rte_eth_dev_reset( ).
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
> @@ -1838,9 +1851,6 @@ void rte_eth_dev_close(uint16_t port_id);
>   *   - (-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.
>   */
>  int rte_eth_dev_reset(uint16_t port_id);
>  
> @@ -2574,6 +2584,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 */
> 

Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Zhang, Qi Z]

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Wednesday, August 8, 2018 7:15 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>
> Cc: dev@dpdk.org; Shelton, Benjamin H <benjamin.h.shelton@intel.com>;
> Vangati, Narender <narender.vangati@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> 0000-cover-letter.patch@dpdk.org
> Subject: Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async
> 
> On 08/08/2018 08:00 AM, Qi Zhang wrote:
> > rte_eth_dev_reset should be implemented in an async way since it is
> > possible 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, all these time consuming task will block the the
> > interrupt thread.
> > The patch claims rte_eth_dev_reset is an async function and introduce
> > a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this
> event
> > when finish reset in background. The applicaiton should always wait
> > for this event before continue to configure and restart the device.
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  lib/librte_ethdev/rte_ethdev.h | 48
> > ++++++++++++++++++++++++++----------------
> >  1 file changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_ethdev.h
> > b/lib/librte_ethdev/rte_ethdev.h index 7070e9ab4..541b5161d 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1814,21 +1814,34 @@ 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.
> > + *
> > + * So we claimed rte_eth_dev_reset as an async API, that makes things
> > + easy
> > + * for an application that what to reset the device from the
> > + interrupt
> > + * thread since typically a RTE_ETH_EVENT_INTR_RESET handler is
> > + invoked in
> > + * interrupt thread.
> > + *
> > + * PMD is responsrible to implement ops->dev_reset in an async way,
> > + it can
> > + * offload the whole task into a separate thread, or maybe just
> > + pending on
> > + * hardware interrupt as reset dependency ready or start a timely
> > + alarm
> > + * to poll register status as a background daemon. PMD is also
> > + responsible
> > + * to raise the RTE_ETH_EVENT_RESET_COMPLETE event to notify the
> > + application
> > + * when reset is complete.
> > + *
> > + * Application should not assume device reset is finished after
> > + * rte_eth_dev_reset 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.
> > + *
> 
> If you intend to make this change for 18.11, 

It's not for 18.11, it's for 19.02, 

then I think you need to
> document it as part of 18.08. 

Yes I will send the API change notification in 18.11.

Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Stephen Hemminger]

On Wed,  8 Aug 2018 15:00:42 +0800
Qi Zhang <qi.z.zhang@intel.com> wrote:

> rte_eth_dev_reset should be implemented in an async way since it is
> possible 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, all these time consuming task will block the
> the interrupt thread.
> The patch claims rte_eth_dev_reset is an async function and introduce
> a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this event
> when finish reset in background. The applicaiton should always wait
> for this event before continue to configure and restart the device.


If you have to change every driver to spawn a thread, then this doesn't
seem that useful.  If you have to have a thread, then the base layer
code in EAL should do it.

Lots of DPDK changes seem to require every driver to change (a nuisance),
and then every driver changes in the same boilerplate way (indicates
poor design choice).

Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Andrew Rybchenko]

On 08.08.2018 18:13, Stephen Hemminger wrote:
> On Wed,  8 Aug 2018 15:00:42 +0800
> Qi Zhang <qi.z.zhang@intel.com> wrote:
>
>> rte_eth_dev_reset should be implemented in an async way since it is
>> possible 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, all these time consuming task will block the
>> the interrupt thread.
>> The patch claims rte_eth_dev_reset is an async function and introduce
>> a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this event
>> when finish reset in background. The applicaiton should always wait
>> for this event before continue to configure and restart the device.
>
> If you have to change every driver to spawn a thread, then this doesn't
> seem that useful.  If you have to have a thread, then the base layer
> code in EAL should do it.
>
> Lots of DPDK changes seem to require every driver to change (a nuisance),
> and then every driver changes in the same boilerplate way (indicates
> poor design choice).

+1

Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async

[Zhang, Qi Z]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, August 8, 2018 11:13 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: thomas@monjalon.net; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> dev@dpdk.org; Shelton, Benjamin H <benjamin.h.shelton@intel.com>;
> Vangati, Narender <narender.vangati@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> 0000-cover-letter.patch@dpdk.org
> Subject: Re: [dpdk-dev] [RFC 1/4] ethdev: claim device reset as async
> 
> On Wed,  8 Aug 2018 15:00:42 +0800
> Qi Zhang <qi.z.zhang@intel.com> wrote:
> 
> > rte_eth_dev_reset should be implemented in an async way since it is
> > possible 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, all these time consuming task will block the the
> > interrupt thread.
> > The patch claims rte_eth_dev_reset is an async function and introduce
> > a new event RTE_ETH_EVENT_RESET_COMPLETE. PMD should raise this
> event
> > when finish reset in background. The applicaiton should always wait
> > for this event before continue to configure and restart the device.
> 
> 
> If you have to change every driver to spawn a thread, then this doesn't seem
> that useful.  If you have to have a thread, then the base layer code in EAL
> should do it.

It may not necessary for PMD which can do device reset quickly to spawn a thread, in that case, it just need to raise RTE_ETH_EVENT_RESET_COMPLETE
in the same thread. But I agree it is better to move thread spawn and event raise into ether layer as a standard way.

> 
> Lots of DPDK changes seem to require every driver to change (a nuisance),
> and then every driver changes in the same boilerplate way (indicates poor
> design choice).