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 41AA0427FB; Tue, 21 Mar 2023 17:24:28 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CE78D40A7A; Tue, 21 Mar 2023 17:24:27 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id E049640697; Tue, 21 Mar 2023 17:24:25 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 30B1E20FB43F; Tue, 21 Mar 2023 09:24:25 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 30B1E20FB43F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1679415865; bh=JPkX6FvP7Vhvv8kqNGYq9SixX6ZwpuR9+0bnfCZ6iVM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qnFWmIOReCbXR/657NC32CsLg2eKZonlnsibPllFId61op93RQzqsrEK+7z7o/To2 CfHAtMiKfnjR/PgmJ92GOLghDL1D2yV07FVuYPgyWwByZ0Oimwe2+nc/kZD26uLwtb cL1tM9J4z2dsL5G7D+j1ejPJ2FaQsVGIk2nNE8x8= Date: Tue, 21 Mar 2023 09:24:25 -0700 From: Tyler Retzlaff To: "Zhang, Qi Z" Cc: "Ye, MingjinX" , "dev@dpdk.org" , "Yang, Qiming" , "stable@dpdk.org" , "Zhou, YidingX" , "Zhang, Ke1X" Subject: Re: [PATCH v5] net/ice: fix ice dcf control thread crash Message-ID: <20230321162425.GA5043@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20230317050936.5513-1-mingjinx.ye@intel.com> <20230320094030.18949-1-mingjinx.ye@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 Tue, Mar 21, 2023 at 11:55:19AM +0000, Zhang, Qi Z wrote: > > > > -----Original Message----- > > From: Ye, MingjinX > > Sent: Tuesday, March 21, 2023 10:08 AM > > To: Zhang, Qi Z ; dev@dpdk.org > > Cc: Yang, Qiming ; stable@dpdk.org; Zhou, YidingX > > ; Zhang, Ke1X > > Subject: RE: [PATCH v5] net/ice: fix ice dcf control thread crash > > > > Hi Qi, here is my new solution, can you give me some good suggestions. > > 1. remove the 'vc_event_msg_cb == NULL' related processing and let each > > 'ice-rest' thread, end normally. > > 2. Define vsi_update_thread_num as rte_atomic32_t. > > > > > -----Original Message----- > > > From: Zhang, Qi Z > > > Sent: 2023年3月20日 20:53 > > > To: Ye, MingjinX ; dev@dpdk.org > > > Cc: Yang, Qiming ; stable@dpdk.org; Zhou, > > > YidingX ; Zhang, Ke1X > > > Subject: RE: [PATCH v5] net/ice: fix ice dcf control thread crash > > > > > > for (;;) { > > > > + if (hw->vc_event_msg_cb == NULL) > > > > + break; > > > Can you explain why this is required, seems it not related with your > > > commit log > > The purpose of this is to bring all 'ice-reset' threads to a quick end when hw > > is released. > > I don't understand, the vc_event_msg_cb was initialized in ice_dcf_dev_init and never be reset why we need this check, anything I missed? > > > > > > > > > > - rte_intr_enable(pci_dev->intr_handle); > > > > - ice_dcf_enable_irq0(hw); > > > > + if (hw->vc_event_msg_cb != NULL) { > > > > + rte_intr_enable(pci_dev->intr_handle); > > > > + ice_dcf_enable_irq0(hw); > > > > > > Same question as above > > These are called when HW releases the resource. Therefore, there is no need > > to call. > > > > > > + rte_spinlock_lock(&dcf_hw->vsi_thread_lock); > > > > + dcf_hw->vsi_update_thread_num++; > > > > + rte_spinlock_unlock(&dcf_hw->vsi_thread_lock); > > > > > > I think you can define vsi_update_thread_num as rte_atomic32_t and use > > > rte_atomic32_add/sub > > At first I chose the rte_atomic32_t option, which is not very elegant using > > spinlock. > > The 'checkpatches.sh' script gives a warning ('Using rte_atomicNN_xxx') when > > it is executed. I saw the comment on line 89 of the script (# refrain from new > > additions of 16/32/64 bits rte_atomicNN_xxx()), so I went with the spinlock > > solution. > > You are right, rte_atomicNN_xxx will be deprecated, and should not be used > We can use the gcc build-in function __atomic_fetch_add/sub as an alternative, sp using __atomic_fetch_{add,sub} is appropriate. please be aware using __atomic_{add_fetch}_fetch is discouraged but it is easy to write the code using only the former. > > > > >