DPDK patches and discussions
 help / color / mirror / Atom feed
From: Zhou Guoyang <zhouguoyang@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>, <dev@dpdk.org>
Cc: <bluca@debian.org>, <cloud.wangxiaoyun@huawei.com>,
	<luoxianjun@huawei.com>, <yin.yinshi@huawei.com>,
	<luojiachen@huawei.com>, <chenlizhong@huawei.com>,
	<zengweiliang.zengweiliang@huawei.com>, <liqingqing3@huawei.com>,
	<stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v1 1/1] net/hinic: fix coredump when PMD used by fstack
Date: Mon, 29 Mar 2021 19:05:24 +0800
Message-ID: <ec518b51-6768-0612-c55d-484b8808c16e@huawei.com> (raw)
In-Reply-To: <c3ceb0c3-4d35-c14e-ea6b-cd4b311657a4@intel.com>

在 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 <zhouguoyang@huawei.com>
>> ---
>>   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



      reply	other threads:[~2021-03-29 11:05 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-15  8:14 Guoyang Zhou
2021-03-22 17:17 ` Ferruh Yigit
2021-03-29 11:05   ` Zhou Guoyang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ec518b51-6768-0612-c55d-484b8808c16e@huawei.com \
    --to=zhouguoyang@huawei.com \
    --cc=bluca@debian.org \
    --cc=chenlizhong@huawei.com \
    --cc=cloud.wangxiaoyun@huawei.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=liqingqing3@huawei.com \
    --cc=luojiachen@huawei.com \
    --cc=luoxianjun@huawei.com \
    --cc=stable@dpdk.org \
    --cc=yin.yinshi@huawei.com \
    --cc=zengweiliang.zengweiliang@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git