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 0FB0EA034F for ; Mon, 29 Mar 2021 13:05:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 790A9140DCA; Mon, 29 Mar 2021 13:05:41 +0200 (CEST) Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by mails.dpdk.org (Postfix) with ESMTP id 451E640042; Mon, 29 Mar 2021 13:05:37 +0200 (CEST) Received: from DGGEMS410-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4F88nH0vSmzmbfD; Mon, 29 Mar 2021 19:02:59 +0800 (CST) Received: from [10.174.61.249] (10.174.61.249) by DGGEMS410-HUB.china.huawei.com (10.3.19.210) with Microsoft SMTP Server id 14.3.498.0; Mon, 29 Mar 2021 19:05:25 +0800 To: Ferruh Yigit , CC: , , , , , , , , References: <1615796077-68790-1-git-send-email-zhouguoyang@huawei.com> From: Zhou Guoyang Message-ID: Date: Mon, 29 Mar 2021 19:05:24 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.61.249] X-CFilter-Loop: Reflected Subject: Re: [dpdk-stable] [PATCH v1 1/1] net/hinic: fix coredump when PMD used by fstack X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" 在 2021/3/23 1:17, Ferruh Yigit 写道: > On 3/15/2021 8:14 AM, Guoyang Zhou wrote: >> The fstack will use secondary process to access the memory of >> eth_dev_ops , and it wants to get the info of dev, but hinic >> driver does not initialized it when in secondary process. >> > > I guess the issue is not specific to the f-stack, perhaps can generalize the patch title. > >> Fixes: 66f64dd6dc86 ("net/hinic: fix secondary process") >> Cc: stable@dpdk.org >> Signed-off-by: Guoyang Zhou >> --- >>   drivers/net/hinic/base/hinic_compat.h | 25 ++++++++----------------- >>   drivers/net/hinic/hinic_pmd_ethdev.c  |  5 +++++ >>   2 files changed, 13 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/hinic/base/hinic_compat.h b/drivers/net/hinic/base/hinic_compat.h >> index 6dd210e..aea3320 100644 >> --- a/drivers/net/hinic/base/hinic_compat.h >> +++ b/drivers/net/hinic/base/hinic_compat.h >> @@ -171,6 +171,7 @@ static inline u32 readl(const volatile void *addr) >>   #else >>   #define CLOCK_TYPE CLOCK_MONOTONIC >>   #endif >> +#define HINIC_MUTEX_TIMEOUT  10 >>     static inline unsigned long clock_gettime_ms(void) >>   { >> @@ -225,24 +226,14 @@ static inline int hinic_mutex_destroy(pthread_mutex_t *pthreadmutex) >>   static inline int hinic_mutex_lock(pthread_mutex_t *pthreadmutex) >>   { >>       int err; >> +    struct timespec tout; >>   -    err = pthread_mutex_lock(pthreadmutex); >> -    if (!err) { >> -        return err; >> -    } else if (err == EOWNERDEAD) { >> -        PMD_DRV_LOG(ERR, "Mutex lock failed. (ErrorNo=%d)", errno); >> -#if defined(__GLIBC__) >> -#if __GLIBC_PREREQ(2, 12) >> -        (void)pthread_mutex_consistent(pthreadmutex); >> -#else >> -        (void)pthread_mutex_consistent_np(pthreadmutex); >> -#endif >> -#else >> -        (void)pthread_mutex_consistent(pthreadmutex); >> -#endif >> -    } else { >> -        PMD_DRV_LOG(ERR, "Mutex lock failed. (ErrorNo=%d)", errno); >> -    } >> +    (void)clock_gettime(CLOCK_TYPE, &tout); >> + >> +    tout.tv_sec += HINIC_MUTEX_TIMEOUT; >> +    err = pthread_mutex_timedlock(pthreadmutex, &tout); >> +    if (err) >> +        PMD_DRV_LOG(ERR, "Mutex lock failed. (ErrorNo=%d)", err); >>   > > Is above change related to the secondary process fix? > Yes, the hinic_mutex_lock function is related to the secondary process fix. Becasue I found if do not fix it, when a lot of secondary processes calls secondary ops (dev_infos_get), it will cause deadlock in low probability. >>       return err; >>   } >> diff --git a/drivers/net/hinic/hinic_pmd_ethdev.c b/drivers/net/hinic/hinic_pmd_ethdev.c >> index 1d6b710..057e7b1 100644 >> --- a/drivers/net/hinic/hinic_pmd_ethdev.c >> +++ b/drivers/net/hinic/hinic_pmd_ethdev.c >> @@ -3085,6 +3085,10 @@ static int hinic_dev_close(struct rte_eth_dev *dev) >>       .filter_ctrl                   = hinic_dev_filter_ctrl, >>   }; >>   +static const struct eth_dev_ops hinic_dev_sec_ops = { >> +    .dev_infos_get                 = hinic_dev_infos_get, >> +}; >> + >>   static int hinic_func_init(struct rte_eth_dev *eth_dev) >>   { >>       struct rte_pci_device *pci_dev; >> @@ -3099,6 +3103,7 @@ static int hinic_func_init(struct rte_eth_dev *eth_dev) >>         /* EAL is SECONDARY and eth_dev is already created */ >>       if (rte_eal_process_type() != RTE_PROC_PRIMARY) { >> +        eth_dev->dev_ops = &hinic_dev_sec_ops; > > Why not using existing dev_ops but creating a new one? > >>           PMD_DRV_LOG(INFO, "Initialize %s in secondary process", >>                   eth_dev->data->name); >>   > > . Because many interfaces do not support calling from the secondary process due to hardware reason, and from the current test situation, it does not be needed. Regards, Guoyang Zhou