Re: [PATCH v4 1/7] ethdev: fix race-condition of proactive error handling mode

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 5 Sep 2024 09:24:58 +0000
Chengwen Feng <fengchengwen@huawei.com> wrote:

> In the proactive error handling mode, the PMD will set the data path
> pointers to dummy functions and then try recovery, in this period the
> application may still invoking data path API. This will introduce a
> race-condition with data path which may lead to crash [1].
> 
> Although the PMD added delay after setting data path pointers to cover
> the above race-condition, it reduces the probability, but it doesn't
> solve the problem.
> 
> To solve the race-condition problem fundamentally, the following
> requirements are added:
> 1. The PMD should set the data path pointers to dummy functions after
>    report RTE_ETH_EVENT_ERR_RECOVERING event.
> 2. The application should stop data path API invocation when process
>    the RTE_ETH_EVENT_ERR_RECOVERING event.
> 3. The PMD should set the data path pointers to valid functions before
>    report RTE_ETH_EVENT_RECOVERY_SUCCESS event.
> 4. The application should enable data path API invocation when process
>    the RTE_ETH_EVENT_RECOVERY_SUCCESS event.
> 
> Also, this patch introduce a driver internal function
> rte_eth_fp_ops_setup which used as an help function for PMD.
> 
> [1] http://patchwork.dpdk.org/project/dpdk/patch/20230220060839.1267349-2-ashok.k.kaladi@intel.com/
> 
> Fixes: eb0d471a8941 ("ethdev: add proactive error handling mode")
> Cc: stable@dpdk.org

This is not material for stable release, because of the impact to PMD etc.

> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Acked-by: Huisong Li <lihuisong@huawei.com>

...

> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 548fada1c7..0aec5588e5 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -4041,25 +4041,28 @@ enum rte_eth_event_type {
>  	 */
>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
>  	/** Port recovering from a hardware or firmware error.
> -	 * If PMD supports proactive error recovery,
> -	 * it should trigger this event to notify application
> -	 * that it detected an error and the recovery is being started.
> -	 * Upon receiving the event, the application should not invoke any control path API
> -	 * (such as rte_eth_dev_configure/rte_eth_dev_stop...) until receiving
> -	 * RTE_ETH_EVENT_RECOVERY_SUCCESS or RTE_ETH_EVENT_RECOVERY_FAILED event.
> -	 * The PMD will set the data path pointers to dummy functions,
> -	 * and re-set the data path pointers to non-dummy functions
> -	 * before reporting RTE_ETH_EVENT_RECOVERY_SUCCESS event.
> -	 * It means that the application cannot send or receive any packets
> -	 * during this period.
> +	 *
> +	 * If PMD supports proactive error recovery, it should trigger this
> +	 * event to notify application that it detected an error and the
> +	 * recovery is about to start.
> +	 *
> +	 * Upon receiving the event, the application should not invoke any
> +	 * control and data path API until receiving
> +	 * RTE_ETH_EVENT_RECOVERY_SUCCESS or RTE_ETH_EVENT_RECOVERY_FAILED
> +	 * event.
> +	 *
> +	 * Once this event is reported, the PMD will set the data path pointers
> +	 * to dummy functions, and re-set the data path pointers to valid
> +	 * functions before reporting RTE_ETH_EVENT_RECOVERY_SUCCESS event.
> +	 *

Please use the IETF RFC conventions for wording here.
Use "should" only when it is optional. In these cases the word "must"
must be used.

	* If PMD supports proactive error recovery, it must trigger this
...


>  	 * @note Before the PMD reports the recovery result,
>  	 * the PMD may report the RTE_ETH_EVENT_ERR_RECOVERING event again,
>  	 * because a larger error may occur during the recovery.
>  	 */
>  	RTE_ETH_EVENT_ERR_RECOVERING,
>  	/** Port recovers successfully from the error.
> -	 * The PMD already re-configured the port,
> -	 * and the effect is the same as a restart operation.
> +	 *
> +	 * The PMD already re-configured the port:
>  	 * a) The following operation will be retained: (alphabetically)
>  	 *    - DCB configuration
>  	 *    - FEC configuration
> @@ -4086,6 +4089,9 @@ enum rte_eth_event_type {
>  	 *      (@see RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP)
>  	 * c) Any other configuration will not be stored
>  	 *    and will need to be re-configured.
> +	 *
> +	 * The application should restore some additional configuration
> +	 * (see above case b/c), and then enable data path API invocation.
>  	 */
>  	RTE_ETH_EVENT_RECOVERY_SUCCESS,
>  	/** Port recovery failed.
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 1669055ca5..da592b63bc 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -346,6 +346,7 @@ INTERNAL {
>  	rte_eth_devices;
>  	rte_eth_dma_zone_free;
>  	rte_eth_dma_zone_reserve;
> +	rte_eth_fp_ops_setup;
>  	rte_eth_hairpin_queue_peer_bind;
>  	rte_eth_hairpin_queue_peer_unbind;
>  	rte_eth_hairpin_queue_peer_update;

My other concern is that changing fp_ops on a running port is not safe.
No part of eth_dev_fp_ops_setup() is atomic.