[RFC,1/4] ethdev: claim device reset as async
Checks
Commit Message
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(-)
Comments
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 */
>
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).
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
> -----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).
> -----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.
@@ -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 */