Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon <thomas@monjalon.net>]

03/02/2022 21:28, Ajit Khaparde:
> On Tue, Feb 1, 2022 at 5:20 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > On 2/1/2022 1:09 PM, Kalesh Anakkur Purayil wrote:
> > > On Tue, Feb 1, 2022 at 5:41 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> > >     On 1/28/2022 12:48 PM, Kalesh A P wrote:
> > >      > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
> > >      > --- a/doc/guides/prog_guide/poll_mode_drv.rst
> > >      > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> > >      > +Error recovery support
> > >      > +~~~~~~~~~~~~~~~~~~~~~~
> > >      > +
> > >      > +When the PMD detects a FW reset or error condition, it may try to recover
> > >      > +from the error without needing the application intervention. In such cases,

Wrong, the application needs to react on the event.

> > >      > +PMD would need events to notify the application that it is undergoing
> > >      > +an error recovery.
> > >      > +
> > >      > +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> > >      > +application that PMD detected a FW reset or FW error condition. PMD may
> > >      > +try to recover from the error by itself. Data path may be quiesced and

PMD may try to recover by itself but the application must take action.

> > >      > +control path operations may fail during the recovery period. The application
> > >      > +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.

The application should stop polling and doing anything else with the device,
otherwise it will receive an error code during the recovery,
meaning until the RECOVERED event is received.
Said like that, it is less vague, you see.

> > >     Between the time FW error occurred and the application receive the RECOVERING event,
> > >     datapath will continue to poll and application may call control APIs, so the event
> > >     really is not solving the issue and driver somehow should be sure this won't crash
> > >     the application, in that case not sure about the benefit of this event.
> > >
> > > [Kalesh]: As soon as the driver detects a FW dead or reset condition, it sets the fastpath pointers to dummy functions. This will prevent the crash. All control path operations would fail with -EBUSY. This change is already there in bnxt PMD. This event is a notification to the application that the PMD is recovering from a FW error condition so that it can stop polling and issue control path operations.
> >
> > This was my point indeed. PMD can't rely on this event and should take actions (for both
> > datapath and control path) to prevent possible crash/error. If that is the case event
> > doesn't have that much benefit since PMD already has to protect itself.
> 
> This is an attempt to prevent PMD and applications from encountering
> unexpected situations because of an error in hardware or firmware.
> The unexpected scenarios can include lockups to segfaults apart from
> other situations.
> 
> When hardware or firmware encounters and error, propagating it to the
> application for it to react can be slow, time consuming and could be
> vendor specific.
> 
> Instead in the case of Broadcom devices we are trying to contain the
> detection and recovery within the hardware, firmware mostly and to a
> small extent driver framework.
> In the case of kernel drivers, this allows avoiding a system reset as
> much as possible while in the case of DPDK PMD, it can prevent
> application reloads or ungraceful exits.
> 
> The PMD generated notification to the application will be more like a
> "For Your Information" for most of the applications.

The most important for the application is to be tolerant to the errors
returned in the datapath and in control API.

> But applications can decide to act on the notification and take
> specific steps - like flushing or deleting all the existing flow
> rules, meters etc.. without waiting for success or failure indication.
> Applications can also re-offload the flow rules, recreate meters based
> on the notification allowing them to sync with the fresh hardware,
> firmware state.

We need to know what has to be recreated.
I think we should keep the same rule as for a restart.

> We think other PMD can also take advantage of these notifications to
> provide better reliability, accessibility and serviceability in
> general.

Indeed others can benefit of some mechanisms if they are common
*and well explained*.

[...]
> > >      > +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application

s/should/must/

> > >      > +that the it has recovered from the error condition. PMD re-configures the port
> > >      > +to the state prior to the error condition. Control path and data path are up now.
> > >      > +Since the device has undergone a reset, flow rules offloaded prior to reset
> > >      > +may be lost and the application should recreate the rules again.

Please refer to the restart situation, including these capabilities:

/** Device supports keeping flow rules across restart. */
#define RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP         RTE_BIT64(3)
/** Device supports keeping shared flow objects across restart. */
#define RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP RTE_BIT64(4)

> > >     I think the most difficult part here is clarify what application should do
> > >     when this event received consistent for all devices, "flow rules may be lost"
> > >     looks very vague to me.

This is the rule for a restart:

 * Please note that some configuration is not stored between calls to
 * rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will
 * be retained:
 *
 *     - MTU
 *     - flow control settings
 *     - receive mode configuration (promiscuous mode, all-multicast mode,
 *       hardware checksum mode, RSS/VMDq settings etc.)
 *     - VLAN filtering configuration
 *     - default MAC address
 *     - MAC addresses supplied to MAC address array
 *     - flow director filtering mode (but not filtering rules)
 *     - NIC queue statistics mappings
 *
 * The following configuration may be retained or not
 * depending on the device capabilities:
 *
 *     - flow rules
 *     - flow-related shared objects, e.g. indirect actions
 *
 * Any other configuration will not be stored and will need to be re-entered
 * before a call to rte_eth_dev_start().

> > >     Unless it is not clear for application what to do when this event is received,
> > >     it is not that useful or it will be specific to some PMDs. And I can see it is
> > >     hard to clarify this but perhaps we can define a set of common behavior.
> > >     Not sure what others are thinking.

+1

> > > [Kalesh]: Sure, let's wait for others' opinions as well.

Nothing new, we already did such comment in 2020.

> > >      > +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
> > >      > +that it has failed to recover from the error condition. The device may not be
> > >      > +usable anymore.

Yes good idea to use RMV event in case of failure.
You should update the comment of RTE_ETH_EVENT_INTR_RMV
to list cases when it is fired:
	- device unplugged
	- recovery failure
	- reset failure?

> > >      > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > >      > index 147cc1c..a46819f 100644
> > >      > --- a/lib/ethdev/rte_ethdev.h
> > >      > +++ b/lib/ethdev/rte_ethdev.h
> > >      > @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> > >      >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> > >      >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> > >      >       RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> > >      > +     RTE_ETH_EVENT_ERR_RECOVERING,
> > >      > +                     /**< port recovering from an error
> > >      > +                      *
> > >      > +                      * PMD detected a FW reset or error condition.
> > >      > +                      * PMD will try to recover from the error.
> > >      > +                      * Data path may be quiesced and Control path operations
> > >      > +                      * may fail at this time.
> > >      > +                      */
> > >
> > >     Please put multi line comments before enum, Andrew did a set of cleanups for these.
> > >
> > > [Kalesh]: Sure, will do.
> > >
> > >
> > >      > +     RTE_ETH_EVENT_RECOVERED,
> > >      > +                     /**< port recovered from an error
> > >      > +                      *
> > >      > +                      * PMD has recovered from the error condition.
> > >      > +                      * Control path and Data path are up now.
> > >      > +                      * PMD re-configures the port to the state prior to the error.
> > >      > +                      * Since the device has undergone a reset, flow rules
> > >      > +                      * offloaded prior to reset may be lost and
> > >      > +                      * the application should recreate the rules again.
> > >      > +                      */

Same as for the RST, the comments need to be very clear,
while being concise here.

I think there are a lot of tips in this thread to make this mechanism
work in a generic way. Please work on a precise new version, thanks.