From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C174F45AF9; Thu, 10 Oct 2024 02:47:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 92D454025E; Thu, 10 Oct 2024 02:47:02 +0200 (CEST) Received: from mail-pl1-f169.google.com (mail-pl1-f169.google.com [209.85.214.169]) by mails.dpdk.org (Postfix) with ESMTP id 9F5D240156 for ; Thu, 10 Oct 2024 02:47:00 +0200 (CEST) Received: by mail-pl1-f169.google.com with SMTP id d9443c01a7336-20c56b816faso3149115ad.2 for ; Wed, 09 Oct 2024 17:47:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1728521220; x=1729126020; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=QbHvh5NJNmY0yzY2vtBRLqT7+lHxg6HKJ4Mh2yXFAM4=; b=AMWGo3V++SeYgRhtZkI9AvHsHKh+C1Er5dIjtCNfRCmIa8Sn3oxBHZS4QnBVh6bcfL ZD7Eb3QGGB90r4fq6mBsDPXmEfo63S4k8a2WrznRySp+Y4pDgTb4pfh6RZEYKx1g/8uB 0DmY1M8TSfoIs/1uLpJL8YK96h7hTym/m23tNpvH956b5UGORQpbwqbG3Tfbfflhjiif KELfWrPm0yKYy+mLraE3AkVp/xli2brwnYLznoWnEsZyjbO679CyBUOZbitagNCYq0xV 41zWZZVOzmcIqjev2b9Cp+3NCOY4oKb6VVQtjVt7yTyH2GoiwCfuu/DN083Y37n/vUpx cRwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728521220; x=1729126020; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QbHvh5NJNmY0yzY2vtBRLqT7+lHxg6HKJ4Mh2yXFAM4=; b=RLZ73vR/pRV3OSJ8Rno4mSrAaQO4pE99ivhWfUoE/3yEbOUClKiBQxH2lr338GmCbE xZt2Kd+D1OpOov1flD9qfmNLDFn2JbfPgq81s11aYTczNkJoJHVcQWqSUiYbkP7yNu0v SOiXaTU3+OvGAa4ojXFBbXbBIM1kS97eBn9LNHoSWhMCnjxg8SbryFws+hHEFDPexwoR vV+VSsHgCKGOcAXEM+OX1ro1b4Lth7FHhS/Y8l/5Br9u6lDgQjBrvvv0eqURnm32QfaC JHCTSXJDRGVLH7TK1Mm++gvpPdyBL5Gl2nA1qVbMFLeE1rCl77Iyc8EZK5r2wURbpDWd 9/tg== X-Forwarded-Encrypted: i=1; AJvYcCVvw3c6Q/zNL1QT9+l3TXV+5CS2oUIYW2YD2ww41ORe46yyf3WSuA+/ThXeTCnKVVQJytA=@dpdk.org X-Gm-Message-State: AOJu0YzVRmaLQ9CsO87XvP2DGBZ8MJrCyPyDKQfNFcHU45SF+2eNynYI 7MR1kG0KX9bMFOa6Q//CLct8vENj5VOpQGrp7+4PP9X7UjzGkGCXTvjmx+7zULM= X-Google-Smtp-Source: AGHT+IGb3p4I0WIsQkKjvk4dxOUrXNaHEchDLegXdIQqd467XowOjfns1IaTMwJqWJhqGc54E0n6TA== X-Received: by 2002:a17:902:d491:b0:20c:8907:902 with SMTP id d9443c01a7336-20c8907660amr6069495ad.49.1728521219740; Wed, 09 Oct 2024 17:46:59 -0700 (PDT) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-7ea4495a450sm38680a12.70.2024.10.09.17.46.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 09 Oct 2024 17:46:59 -0700 (PDT) Date: Wed, 9 Oct 2024 17:46:57 -0700 From: Stephen Hemminger To: Chengwen Feng Cc: , , , , Andrew Rybchenko , Somnath Kotur , Kalesh AP , , Subject: Re: [PATCH v4 1/7] ethdev: fix race-condition of proactive error handling mode Message-ID: <20241009174657.59491f20@hermes.local> In-Reply-To: <20240905092504.10725-2-fengchengwen@huawei.com> References: <20230301030610.49468-1-fengchengwen@huawei.com> <20240905092504.10725-1-fengchengwen@huawei.com> <20240905092504.10725-2-fengchengwen@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, 5 Sep 2024 09:24:58 +0000 Chengwen Feng 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 > Acked-by: Konstantin Ananyev > Acked-by: Huisong Li ... > 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.