The lock sh->txpp.mutex was not correctly released on one path of cleanup function return, potentially causing the deadlock. Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing") Cc: stable@dpdk.org Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk> --- drivers/net/mlx5/mlx5_txpp.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c index 4f6da9f2d1..0ece788a84 100644 --- a/drivers/net/mlx5/mlx5_txpp.c +++ b/drivers/net/mlx5/mlx5_txpp.c @@ -961,8 +961,12 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) MLX5_ASSERT(!ret); RTE_SET_USED(ret); MLX5_ASSERT(sh->txpp.refcnt); - if (!sh->txpp.refcnt || --sh->txpp.refcnt) + if (!sh->txpp.refcnt || --sh->txpp.refcnt) { + ret = pthread_mutex_unlock(&sh->txpp.mutex); + MLX5_ASSERT(!ret); + RTE_SET_USED(ret); return; + } /* No references any more, do actual destroy. */ mlx5_txpp_destroy(sh); ret = pthread_mutex_unlock(&sh->txpp.mutex); -- 2.17.1
> -----Original Message-----
> From: Chengfeng Ye <cyeaa@connect.ust.hk>
> Sent: Tuesday, October 12, 2021 13:02
> To: david.marchand@redhat.com; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Matan
> Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Chengfeng Ye <cyeaa@connect.ust.hk>;
> stable@dpdk.org
> Subject: [PATCH v5] net/mlx5: fix mutex unlock in txpp cleanup
>
> The lock sh->txpp.mutex was not correctly released on one path of cleanup
> function return, potentially causing the deadlock.
>
> Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
Hi,
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Chengfeng Ye
> Sent: Tuesday, October 12, 2021 1:02 PM
> To: david.marchand@redhat.com; Slava Ovsiienko
> <viacheslavo@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Matan
> Azrad <matan@nvidia.com>
> Cc: dev@dpdk.org; Chengfeng Ye <cyeaa@connect.ust.hk>;
> stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v5] net/mlx5: fix mutex unlock in txpp cleanup
>
> The lock sh->txpp.mutex was not correctly released on one path of cleanup
> function return, potentially causing the deadlock.
>
> Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
Patch applied to next-net-mlx,
Kindest regards
Raslan Darawsheh
On 10/12/2021 11:02 AM, Chengfeng Ye wrote: > The lock sh->txpp.mutex was not correctly released on one path > of cleanup function return, potentially causing the deadlock. > > Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing") > Cc: stable@dpdk.org > > Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk> > --- > drivers/net/mlx5/mlx5_txpp.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c > index 4f6da9f2d1..0ece788a84 100644 > --- a/drivers/net/mlx5/mlx5_txpp.c > +++ b/drivers/net/mlx5/mlx5_txpp.c > @@ -961,8 +961,12 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) > MLX5_ASSERT(!ret); > RTE_SET_USED(ret); > MLX5_ASSERT(sh->txpp.refcnt); > - if (!sh->txpp.refcnt || --sh->txpp.refcnt) > + if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > + ret = pthread_mutex_unlock(&sh->txpp.mutex); > + MLX5_ASSERT(!ret); > + RTE_SET_USED(ret); Is this 'RTE_SET_USED()' need to be used multiple times for same variable? This usage looks ugly, I can see why it is used but I wonder if this can be solved differently, what about something like following: #ifdef RTE_LIBRTE_MLX5_DEBUG #define MLX5_ASSERT(exp) RTE_VERIFY(exp) #else #ifdef RTE_ENABLE_ASSERT #define MLX5_ASSERT(exp) RTE_ASSERT(exp) #else #define MLX5_ASSERT(exp) RTE_SET_USED(exp) #endif #endif > return; > + } > /* No references any more, do actual destroy. */ > mlx5_txpp_destroy(sh); > ret = pthread_mutex_unlock(&sh->txpp.mutex); >
Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Wednesday, November 10, 2021 18:57 > To: Chengfeng Ye <cyeaa@connect.ust.hk>; david.marchand@redhat.com; > Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf Shuler > <shahafs@nvidia.com>; Matan Azrad <matan@nvidia.com> > Cc: dev@dpdk.org; stable@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5] net/mlx5: fix mutex unlock in txpp > cleanup > > On 10/12/2021 11:02 AM, Chengfeng Ye wrote: > > The lock sh->txpp.mutex was not correctly released on one path of > > cleanup function return, potentially causing the deadlock. > > > > Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing") > > Cc: stable@dpdk.org > > > > Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk> > > --- > > drivers/net/mlx5/mlx5_txpp.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/mlx5/mlx5_txpp.c > > b/drivers/net/mlx5/mlx5_txpp.c index 4f6da9f2d1..0ece788a84 100644 > > --- a/drivers/net/mlx5/mlx5_txpp.c > > +++ b/drivers/net/mlx5/mlx5_txpp.c > > @@ -961,8 +961,12 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) > > MLX5_ASSERT(!ret); > > RTE_SET_USED(ret); > > MLX5_ASSERT(sh->txpp.refcnt); > > - if (!sh->txpp.refcnt || --sh->txpp.refcnt) > > + if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > > + ret = pthread_mutex_unlock(&sh->txpp.mutex); > > + MLX5_ASSERT(!ret); > > + RTE_SET_USED(ret); > > Is this 'RTE_SET_USED()' need to be used multiple times for same variable? mmm, It seems "claim_zero()" macro would be better here: claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); I will provide the cleanup patch, thank you for noticing that > This usage looks ugly, I can see why it is used but I wonder if this can be > solved differently, what about something like following: > > #ifdef RTE_LIBRTE_MLX5_DEBUG > #define MLX5_ASSERT(exp) RTE_VERIFY(exp) > #else > #ifdef RTE_ENABLE_ASSERT > #define MLX5_ASSERT(exp) RTE_ASSERT(exp) > #else > #define MLX5_ASSERT(exp) RTE_SET_USED(exp) > #endif > #endif It would directly replace MLX5_ASSERT(exp) with RTE_SET_USED(exp) if there is neither RTE_ENABLE_ASSERT nor RTE_LIBRTE_MLX5_DEBUG. We would not like to drop the "not used" check functionality at all , right? With best regards, Slava
The patch just refines the code and replaces the pairs of MLX5_ASSERT() and RTE_SET_USED() with equivalent claim_zero(). Cc: stable@dpdk.org Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com> --- drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 --- a/drivers/net/mlx5/mlx5_txpp.c +++ b/drivers/net/mlx5/mlx5_txpp.c @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_dev_ctx_shared *sh = priv->sh; int err = 0; - int ret; if (!priv->config.tx_pp) { /* Packet pacing is not requested for the device. */ @@ -903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) return 0; } if (priv->config.tx_pp > 0) { - ret = rte_mbuf_dynflag_lookup - (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); - if (ret < 0) + err = rte_mbuf_dynflag_lookup + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); + /* No flag registered means no service needed. */ + if (err < 0) return 0; + err = 0; } - ret = pthread_mutex_lock(&sh->txpp.mutex); - MLX5_ASSERT(!ret); - RTE_SET_USED(ret); + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); if (sh->txpp.refcnt) { priv->txpp_en = 1; ++sh->txpp.refcnt; @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) rte_errno = -err; } } - ret = pthread_mutex_unlock(&sh->txpp.mutex); - MLX5_ASSERT(!ret); - RTE_SET_USED(ret); + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); return err; } @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { struct mlx5_priv *priv = dev->data->dev_private; struct mlx5_dev_ctx_shared *sh = priv->sh; - int ret; if (!priv->txpp_en) { /* Packet pacing is already disabled for the device. */ return; } priv->txpp_en = 0; - ret = pthread_mutex_lock(&sh->txpp.mutex); - MLX5_ASSERT(!ret); - RTE_SET_USED(ret); + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); MLX5_ASSERT(sh->txpp.refcnt); if (!sh->txpp.refcnt || --sh->txpp.refcnt) { - ret = pthread_mutex_unlock(&sh->txpp.mutex); - MLX5_ASSERT(!ret); - RTE_SET_USED(ret); + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); return; } /* No references any more, do actual destroy. */ mlx5_txpp_destroy(sh); - ret = pthread_mutex_unlock(&sh->txpp.mutex); - MLX5_ASSERT(!ret); - RTE_SET_USED(ret); + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); } /* -- 2.18.1
Hi, Ferruh I've also inspected the mlx5 PMD code for RTE_SET_USED() for the similar issues related to the MLX5_ASSERT(). The patch http://patches.dpdk.org/project/dpdk/patch/20211111084751.26721-1-viacheslavo@nvidia.com/ should refine the few found ones. I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp cleanup" After getting this code in Upstream will care about the version for LTS. With best regards, Slava > -----Original Message----- > From: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > Sent: Thursday, November 11, 2021 10:48 > To: dev@dpdk.org > Cc: ferruh.yigit@intel.com; Raslan Darawsheh <rasland@nvidia.com>; Matan > Azrad <matan@nvidia.com>; stable@dpdk.org > Subject: [PATCH] net/mlx5: remove redundant "set used" > > The patch just refines the code and replaces the pairs of MLX5_ASSERT() and > RTE_SET_USED() with equivalent claim_zero(). > > Cc: stable@dpdk.org > > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > --- > drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > 1 file changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c > index 73626f0e8f..af77e91e4c 100644 > --- a/drivers/net/mlx5/mlx5_txpp.c > +++ b/drivers/net/mlx5/mlx5_txpp.c > @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_dev_ctx_shared *sh = priv->sh; > int err = 0; > - int ret; > > if (!priv->config.tx_pp) { > /* Packet pacing is not requested for the device. */ @@ - > 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > return 0; > } > if (priv->config.tx_pp > 0) { > - ret = rte_mbuf_dynflag_lookup > - > (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > - if (ret < 0) > + err = rte_mbuf_dynflag_lookup > + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > NULL); > + /* No flag registered means no service needed. */ > + if (err < 0) > return 0; > + err = 0; > } > - ret = pthread_mutex_lock(&sh->txpp.mutex); > - MLX5_ASSERT(!ret); > - RTE_SET_USED(ret); > + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > if (sh->txpp.refcnt) { > priv->txpp_en = 1; > ++sh->txpp.refcnt; > @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > rte_errno = -err; > } > } > - ret = pthread_mutex_unlock(&sh->txpp.mutex); > - MLX5_ASSERT(!ret); > - RTE_SET_USED(ret); > + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > return err; > } > > @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > struct mlx5_priv *priv = dev->data->dev_private; > struct mlx5_dev_ctx_shared *sh = priv->sh; > - int ret; > > if (!priv->txpp_en) { > /* Packet pacing is already disabled for the device. */ > return; > } > priv->txpp_en = 0; > - ret = pthread_mutex_lock(&sh->txpp.mutex); > - MLX5_ASSERT(!ret); > - RTE_SET_USED(ret); > + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > MLX5_ASSERT(sh->txpp.refcnt); > if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > - ret = pthread_mutex_unlock(&sh->txpp.mutex); > - MLX5_ASSERT(!ret); > - RTE_SET_USED(ret); > + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > return; > } > /* No references any more, do actual destroy. */ > mlx5_txpp_destroy(sh); > - ret = pthread_mutex_unlock(&sh->txpp.mutex); > - MLX5_ASSERT(!ret); > - RTE_SET_USED(ret); > + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > } > > /* > -- > 2.18.1
On 11/11/2021 7:06 AM, Slava Ovsiienko wrote:
> Hi, Ferruh
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Wednesday, November 10, 2021 18:57
>> To: Chengfeng Ye <cyeaa@connect.ust.hk>; david.marchand@redhat.com;
>> Slava Ovsiienko <viacheslavo@nvidia.com>; Shahaf Shuler
>> <shahafs@nvidia.com>; Matan Azrad <matan@nvidia.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v5] net/mlx5: fix mutex unlock in txpp
>> cleanup
>>
>> On 10/12/2021 11:02 AM, Chengfeng Ye wrote:
>>> The lock sh->txpp.mutex was not correctly released on one path of
>>> cleanup function return, potentially causing the deadlock.
>>>
>>> Fixes: d133f4cdb7 ("net/mlx5: create clock queue for packet pacing")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Chengfeng Ye <cyeaa@connect.ust.hk>
>>> ---
>>> drivers/net/mlx5/mlx5_txpp.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/mlx5/mlx5_txpp.c
>>> b/drivers/net/mlx5/mlx5_txpp.c index 4f6da9f2d1..0ece788a84 100644
>>> --- a/drivers/net/mlx5/mlx5_txpp.c
>>> +++ b/drivers/net/mlx5/mlx5_txpp.c
>>> @@ -961,8 +961,12 @@ mlx5_txpp_stop(struct rte_eth_dev *dev)
>>> MLX5_ASSERT(!ret);
>>> RTE_SET_USED(ret);
>>> MLX5_ASSERT(sh->txpp.refcnt);
>>> - if (!sh->txpp.refcnt || --sh->txpp.refcnt)
>>> + if (!sh->txpp.refcnt || --sh->txpp.refcnt) {
>>> + ret = pthread_mutex_unlock(&sh->txpp.mutex);
>>> + MLX5_ASSERT(!ret);
>>> + RTE_SET_USED(ret);
>>
>> Is this 'RTE_SET_USED()' need to be used multiple times for same variable?
> mmm, It seems "claim_zero()" macro would be better here:
>
> claim_zero(pthread_mutex_lock(&sh->txpp.mutex));
>
> I will provide the cleanup patch, thank you for noticing that
>
>> This usage looks ugly, I can see why it is used but I wonder if this can be
>> solved differently, what about something like following:
>>
>> #ifdef RTE_LIBRTE_MLX5_DEBUG
>> #define MLX5_ASSERT(exp) RTE_VERIFY(exp)
>> #else
>> #ifdef RTE_ENABLE_ASSERT
>> #define MLX5_ASSERT(exp) RTE_ASSERT(exp)
>> #else
>> #define MLX5_ASSERT(exp) RTE_SET_USED(exp)
>> #endif
>> #endif
> It would directly replace MLX5_ASSERT(exp) with RTE_SET_USED(exp)
> if there is neither RTE_ENABLE_ASSERT nor RTE_LIBRTE_MLX5_DEBUG.
> We would not like to drop the "not used" check functionality at all , right?
>
The suggestion was to prevent following kind of usage:
MLX5_ASSERT(!ret);
RTE_SET_USED(ret);
I assume you need above usage when a variable is used only in the 'MLX5_ASSERT',
if there is a way to prevent warning in that case without 'RTE_SET_USED' that
may be better.
On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > Hi, Ferruh > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the similar > issues related to the MLX5_ASSERT(). > > The patch http://patches.dpdk.org/project/dpdk/patch/20211111084751.26721-1-viacheslavo@nvidia.com/ > should refine the few found ones. > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp cleanup" > After getting this code in Upstream will care about the version for LTS. > It will cause additional complexity for the LTS, since a small part of the below fix will be originated from Chengfeng's change. To help LTS, what do you think - First get your fix on top of current task - Have a new version from Chengfeng on top of latest head, with 'claim_zero' usage? So only your update need to be merged to LTS releases. > With best regards, > Slava > >> -----Original Message----- >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com> >> Sent: Thursday, November 11, 2021 10:48 >> To: dev@dpdk.org >> Cc: ferruh.yigit@intel.com; Raslan Darawsheh <rasland@nvidia.com>; Matan >> Azrad <matan@nvidia.com>; stable@dpdk.org >> Subject: [PATCH] net/mlx5: remove redundant "set used" >> >> The patch just refines the code and replaces the pairs of MLX5_ASSERT() and >> RTE_SET_USED() with equivalent claim_zero(). >> >> Cc: stable@dpdk.org >> >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com> >> --- >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- >> 1 file changed, 10 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c >> index 73626f0e8f..af77e91e4c 100644 >> --- a/drivers/net/mlx5/mlx5_txpp.c >> +++ b/drivers/net/mlx5/mlx5_txpp.c >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) >> struct mlx5_priv *priv = dev->data->dev_private; >> struct mlx5_dev_ctx_shared *sh = priv->sh; >> int err = 0; >> - int ret; >> >> if (!priv->config.tx_pp) { >> /* Packet pacing is not requested for the device. */ @@ - >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) >> return 0; >> } >> if (priv->config.tx_pp > 0) { >> - ret = rte_mbuf_dynflag_lookup >> - >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); >> - if (ret < 0) >> + err = rte_mbuf_dynflag_lookup >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, >> NULL); >> + /* No flag registered means no service needed. */ >> + if (err < 0) >> return 0; >> + err = 0; >> } >> - ret = pthread_mutex_lock(&sh->txpp.mutex); >> - MLX5_ASSERT(!ret); >> - RTE_SET_USED(ret); >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); >> if (sh->txpp.refcnt) { >> priv->txpp_en = 1; >> ++sh->txpp.refcnt; >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) >> rte_errno = -err; >> } >> } >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); >> - MLX5_ASSERT(!ret); >> - RTE_SET_USED(ret); >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); >> return err; >> } >> >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { >> struct mlx5_priv *priv = dev->data->dev_private; >> struct mlx5_dev_ctx_shared *sh = priv->sh; >> - int ret; >> >> if (!priv->txpp_en) { >> /* Packet pacing is already disabled for the device. */ >> return; >> } >> priv->txpp_en = 0; >> - ret = pthread_mutex_lock(&sh->txpp.mutex); >> - MLX5_ASSERT(!ret); >> - RTE_SET_USED(ret); >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); >> MLX5_ASSERT(sh->txpp.refcnt); >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); >> - MLX5_ASSERT(!ret); >> - RTE_SET_USED(ret); >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); >> return; >> } >> /* No references any more, do actual destroy. */ >> mlx5_txpp_destroy(sh); >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); >> - MLX5_ASSERT(!ret); >> - RTE_SET_USED(ret); >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); >> } >> >> /* >> -- >> 2.18.1 >
Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Thursday, November 11, 2021 14:08 > To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org > Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad > <matan@nvidia.com>; stable@dpdk.org; Chengfeng Ye > <cyeaa@connect.ust.hk> > Subject: Re: [PATCH] net/mlx5: remove redundant "set used" > > On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > > Hi, Ferruh > > > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the > > similar issues related to the MLX5_ASSERT(). > > > > The patch > > http://patches.dpdk.org/project/dpdk/patch/20211111084751.26721-1- > viac > > heslavo@nvidia.com/ > > should refine the few found ones. > > > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp > cleanup" > > After getting this code in Upstream will care about the version for LTS. > > > > It will cause additional complexity for the LTS, since a small part of the below > fix will be originated from Chengfeng's change. To help LTS, what do you think > - First get your fix on top of current task > - Have a new version from Chengfeng on top of latest head, with 'claim_zero' > usage? Would be nice, I have no any objections. Chengfeng, could you please, squash (or write by yourself) my proposed updates and send the next version of your patch with "claim_zero()"? > So only your update need to be merged to LTS releases. Yes, agree, it is even better than my proposal. With best regards, Slava > > > >> -----Original Message----- > >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > >> Sent: Thursday, November 11, 2021 10:48 > >> To: dev@dpdk.org > >> Cc: ferruh.yigit@intel.com; Raslan Darawsheh <rasland@nvidia.com>; > >> Matan Azrad <matan@nvidia.com>; stable@dpdk.org > >> Subject: [PATCH] net/mlx5: remove redundant "set used" > >> > >> The patch just refines the code and replaces the pairs of > >> MLX5_ASSERT() and > >> RTE_SET_USED() with equivalent claim_zero(). > >> > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > >> --- > >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > >> 1 file changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/net/mlx5/mlx5_txpp.c > >> b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 > >> --- a/drivers/net/mlx5/mlx5_txpp.c > >> +++ b/drivers/net/mlx5/mlx5_txpp.c > >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> int err = 0; > >> - int ret; > >> > >> if (!priv->config.tx_pp) { > >> /* Packet pacing is not requested for the device. */ @@ - > >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> return 0; > >> } > >> if (priv->config.tx_pp > 0) { > >> - ret = rte_mbuf_dynflag_lookup > >> - > >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > >> - if (ret < 0) > >> + err = rte_mbuf_dynflag_lookup > >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > >> NULL); > >> + /* No flag registered means no service needed. */ > >> + if (err < 0) > >> return 0; > >> + err = 0; > >> } > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> if (sh->txpp.refcnt) { > >> priv->txpp_en = 1; > >> ++sh->txpp.refcnt; > >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> rte_errno = -err; > >> } > >> } > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return err; > >> } > >> > >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> - int ret; > >> > >> if (!priv->txpp_en) { > >> /* Packet pacing is already disabled for the device. */ > >> return; > >> } > >> priv->txpp_en = 0; > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> MLX5_ASSERT(sh->txpp.refcnt); > >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return; > >> } > >> /* No references any more, do actual destroy. */ > >> mlx5_txpp_destroy(sh); > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> } > >> > >> /* > >> -- > >> 2.18.1 > >
[-- Attachment #1: Type: text/plain, Size: 6259 bytes --] No problem, I will send the new patch. But what about the commit message and title, should I use the previous one? net/mlx5: fix mutex unlock in txpp cleanup 获取 Outlook for iOS<https://aka.ms/o0ukef> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com> 发送时间: Thursday, November 11, 2021 8:27:30 PM 收件人: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org <dev@dpdk.org> 抄送: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org <stable@dpdk.org>; YE Chengfeng <cyeaa@connect.ust.hk> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Thursday, November 11, 2021 14:08 > To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org > Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad > <matan@nvidia.com>; stable@dpdk.org; Chengfeng Ye > <cyeaa@connect.ust.hk> > Subject: Re: [PATCH] net/mlx5: remove redundant "set used" > > On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > > Hi, Ferruh > > > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the > > similar issues related to the MLX5_ASSERT(). > > > > The patch > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7C87553a5cd43647db708008d9a50ea321%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722304566828185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=woTMghsE0S7ckoo0AI01YbdWrJeLtbMCu4GeE8JDiUc%3D&reserved=0 > viac > > heslavo@nvidia.com/ > > should refine the few found ones. > > > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp > cleanup" > > After getting this code in Upstream will care about the version for LTS. > > > > It will cause additional complexity for the LTS, since a small part of the below > fix will be originated from Chengfeng's change. To help LTS, what do you think > - First get your fix on top of current task > - Have a new version from Chengfeng on top of latest head, with 'claim_zero' > usage? Would be nice, I have no any objections. Chengfeng, could you please, squash (or write by yourself) my proposed updates and send the next version of your patch with "claim_zero()"? > So only your update need to be merged to LTS releases. Yes, agree, it is even better than my proposal. With best regards, Slava > > > >> -----Original Message----- > >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > >> Sent: Thursday, November 11, 2021 10:48 > >> To: dev@dpdk.org > >> Cc: ferruh.yigit@intel.com; Raslan Darawsheh <rasland@nvidia.com>; > >> Matan Azrad <matan@nvidia.com>; stable@dpdk.org > >> Subject: [PATCH] net/mlx5: remove redundant "set used" > >> > >> The patch just refines the code and replaces the pairs of > >> MLX5_ASSERT() and > >> RTE_SET_USED() with equivalent claim_zero(). > >> > >> Cc: stable@dpdk.org > >> > >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com> > >> --- > >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > >> 1 file changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/net/mlx5/mlx5_txpp.c > >> b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 > >> --- a/drivers/net/mlx5/mlx5_txpp.c > >> +++ b/drivers/net/mlx5/mlx5_txpp.c > >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> int err = 0; > >> - int ret; > >> > >> if (!priv->config.tx_pp) { > >> /* Packet pacing is not requested for the device. */ @@ - > >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> return 0; > >> } > >> if (priv->config.tx_pp > 0) { > >> - ret = rte_mbuf_dynflag_lookup > >> - > >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > >> - if (ret < 0) > >> + err = rte_mbuf_dynflag_lookup > >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > >> NULL); > >> + /* No flag registered means no service needed. */ > >> + if (err < 0) > >> return 0; > >> + err = 0; > >> } > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> if (sh->txpp.refcnt) { > >> priv->txpp_en = 1; > >> ++sh->txpp.refcnt; > >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> rte_errno = -err; > >> } > >> } > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return err; > >> } > >> > >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> - int ret; > >> > >> if (!priv->txpp_en) { > >> /* Packet pacing is already disabled for the device. */ > >> return; > >> } > >> priv->txpp_en = 0; > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> MLX5_ASSERT(sh->txpp.refcnt); > >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return; > >> } > >> /* No references any more, do actual destroy. */ > >> mlx5_txpp_destroy(sh); > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> } > >> > >> /* > >> -- > >> 2.18.1 > > [-- Attachment #2: Type: text/html, Size: 12092 bytes --]
[-- Attachment #1: Type: text/plain, Size: 8313 bytes --] From: YE Chengfeng <cyeaa@connect.ust.hk> Sent: Thursday, November 11, 2021 18:08 To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org Subject: Re: [PATCH] net/mlx5: remove redundant "set used" No problem, I will send the new patch. [SO] Thank you. But what about the commit message and title, should I use the previous one? net/mlx5: fix mutex unlock in txpp cleanup [SO] Please, send as the next version (v6?) of your patch. Just squash my proposals and keep your title (“net/mlx5: fix mutex unlock in txpp cleanup”) and commit message. With best regards, Slava 获取 Outlook for iOS<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Faka.ms%2Fo0ukef&data=04%7C01%7Cviacheslavo%40nvidia.com%7Ce021aa65a82246fea44808d9a52d64bb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637722436716123715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gLoebXXI%2BFE8l6sE0pmNxjvPNVk34vBxWTeVlTOmJJ0%3D&reserved=0> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> 发送时间: Thursday, November 11, 2021 8:27:30 PM 收件人: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>> 抄送: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> <stable@dpdk.org<mailto:stable@dpdk.org>>; YE Chengfeng <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>> > Sent: Thursday, November 11, 2021 14:08 > To: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>>; dev@dpdk.org<mailto:dev@dpdk.org> > Cc: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad > <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org>; Chengfeng Ye > <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> > Subject: Re: [PATCH] net/mlx5: remove redundant "set used" > > On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > > Hi, Ferruh > > > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the > > similar issues related to the MLX5_ASSERT(). > > > > The patch > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7C87553a5cd43647db708008d9a50ea321%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722304566828185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=woTMghsE0S7ckoo0AI01YbdWrJeLtbMCu4GeE8JDiUc%3D&reserved=0<https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Cviacheslavo%40nvidia.com%7Ce021aa65a82246fea44808d9a52d64bb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637722436716123715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=IjK8W%2F8CMJljP%2Fi482yR7ndXZwov4JB6GuHpWlck%2BG8%3D&reserved=0> > viac > > heslavo@nvidia.com/<mailto:heslavo@nvidia.com/> > > should refine the few found ones. > > > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp > cleanup" > > After getting this code in Upstream will care about the version for LTS. > > > > It will cause additional complexity for the LTS, since a small part of the below > fix will be originated from Chengfeng's change. To help LTS, what do you think > - First get your fix on top of current task > - Have a new version from Chengfeng on top of latest head, with 'claim_zero' > usage? Would be nice, I have no any objections. Chengfeng, could you please, squash (or write by yourself) my proposed updates and send the next version of your patch with "claim_zero()"? > So only your update need to be merged to LTS releases. Yes, agree, it is even better than my proposal. With best regards, Slava > > > >> -----Original Message----- > >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> Sent: Thursday, November 11, 2021 10:48 > >> To: dev@dpdk.org<mailto:dev@dpdk.org> > >> Cc: ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; > >> Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> > >> Subject: [PATCH] net/mlx5: remove redundant "set used" > >> > >> The patch just refines the code and replaces the pairs of > >> MLX5_ASSERT() and > >> RTE_SET_USED() with equivalent claim_zero(). > >> > >> Cc: stable@dpdk.org<mailto:stable@dpdk.org> > >> > >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> --- > >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > >> 1 file changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/net/mlx5/mlx5_txpp.c > >> b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 > >> --- a/drivers/net/mlx5/mlx5_txpp.c > >> +++ b/drivers/net/mlx5/mlx5_txpp.c > >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> int err = 0; > >> - int ret; > >> > >> if (!priv->config.tx_pp) { > >> /* Packet pacing is not requested for the device. */ @@ - > >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> return 0; > >> } > >> if (priv->config.tx_pp > 0) { > >> - ret = rte_mbuf_dynflag_lookup > >> - > >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > >> - if (ret < 0) > >> + err = rte_mbuf_dynflag_lookup > >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > >> NULL); > >> + /* No flag registered means no service needed. */ > >> + if (err < 0) > >> return 0; > >> + err = 0; > >> } > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> if (sh->txpp.refcnt) { > >> priv->txpp_en = 1; > >> ++sh->txpp.refcnt; > >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> rte_errno = -err; > >> } > >> } > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return err; > >> } > >> > >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> - int ret; > >> > >> if (!priv->txpp_en) { > >> /* Packet pacing is already disabled for the device. */ > >> return; > >> } > >> priv->txpp_en = 0; > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> MLX5_ASSERT(sh->txpp.refcnt); > >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return; > >> } > >> /* No references any more, do actual destroy. */ > >> mlx5_txpp_destroy(sh); > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> } > >> > >> /* > >> -- > >> 2.18.1 > > [-- Attachment #2: Type: text/html, Size: 18202 bytes --]
[-- Attachment #1: Type: text/plain, Size: 8786 bytes --] Got it. 获取 Outlook for iOS<https://aka.ms/o0ukef> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com> 发送时间: Friday, November 12, 2021 12:47:04 AM 收件人: YE Chengfeng <cyeaa@connect.ust.hk>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org <dev@dpdk.org> 抄送: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org <stable@dpdk.org> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" From: YE Chengfeng <cyeaa@connect.ust.hk> Sent: Thursday, November 11, 2021 18:08 To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org Subject: Re: [PATCH] net/mlx5: remove redundant "set used" No problem, I will send the new patch. [SO] Thank you. But what about the commit message and title, should I use the previous one? net/mlx5: fix mutex unlock in txpp cleanup [SO] Please, send as the next version (v6?) of your patch. Just squash my proposals and keep your title (“net/mlx5: fix mutex unlock in txpp cleanup”) and commit message. With best regards, Slava 获取 Outlook for iOS<https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Faka.ms%2Fo0ukef&data=04%7C01%7Ccyeaa%40connect.ust.hk%7Cb054bd8f8f3c42e1f0bb08d9a532e5d5%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722460307163598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3v%2BIAEdHSfsB%2BEoAmKldFqybeHCC3ihLZ6HSnWzkTD0%3D&reserved=0> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> 发送时间: Thursday, November 11, 2021 8:27:30 PM 收件人: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>> 抄送: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> <stable@dpdk.org<mailto:stable@dpdk.org>>; YE Chengfeng <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>> > Sent: Thursday, November 11, 2021 14:08 > To: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>>; dev@dpdk.org<mailto:dev@dpdk.org> > Cc: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad > <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org>; Chengfeng Ye > <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> > Subject: Re: [PATCH] net/mlx5: remove redundant "set used" > > On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > > Hi, Ferruh > > > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the > > similar issues related to the MLX5_ASSERT(). > > > > The patch > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7C87553a5cd43647db708008d9a50ea321%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722304566828185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=woTMghsE0S7ckoo0AI01YbdWrJeLtbMCu4GeE8JDiUc%3D&reserved=0<https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7Cb054bd8f8f3c42e1f0bb08d9a532e5d5%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722460307173555%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=FH1BEOdaDn4vgLHJ%2BjV8DrH9kZIVCVpoVUEnPB2GHWw%3D&reserved=0> > viac > > heslavo@nvidia.com/<mailto:heslavo@nvidia.com/> > > should refine the few found ones. > > > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp > cleanup" > > After getting this code in Upstream will care about the version for LTS. > > > > It will cause additional complexity for the LTS, since a small part of the below > fix will be originated from Chengfeng's change. To help LTS, what do you think > - First get your fix on top of current task > - Have a new version from Chengfeng on top of latest head, with 'claim_zero' > usage? Would be nice, I have no any objections. Chengfeng, could you please, squash (or write by yourself) my proposed updates and send the next version of your patch with "claim_zero()"? > So only your update need to be merged to LTS releases. Yes, agree, it is even better than my proposal. With best regards, Slava > > > >> -----Original Message----- > >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> Sent: Thursday, November 11, 2021 10:48 > >> To: dev@dpdk.org<mailto:dev@dpdk.org> > >> Cc: ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; > >> Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> > >> Subject: [PATCH] net/mlx5: remove redundant "set used" > >> > >> The patch just refines the code and replaces the pairs of > >> MLX5_ASSERT() and > >> RTE_SET_USED() with equivalent claim_zero(). > >> > >> Cc: stable@dpdk.org<mailto:stable@dpdk.org> > >> > >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> --- > >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > >> 1 file changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/net/mlx5/mlx5_txpp.c > >> b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 > >> --- a/drivers/net/mlx5/mlx5_txpp.c > >> +++ b/drivers/net/mlx5/mlx5_txpp.c > >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> int err = 0; > >> - int ret; > >> > >> if (!priv->config.tx_pp) { > >> /* Packet pacing is not requested for the device. */ @@ - > >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> return 0; > >> } > >> if (priv->config.tx_pp > 0) { > >> - ret = rte_mbuf_dynflag_lookup > >> - > >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > >> - if (ret < 0) > >> + err = rte_mbuf_dynflag_lookup > >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > >> NULL); > >> + /* No flag registered means no service needed. */ > >> + if (err < 0) > >> return 0; > >> + err = 0; > >> } > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> if (sh->txpp.refcnt) { > >> priv->txpp_en = 1; > >> ++sh->txpp.refcnt; > >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> rte_errno = -err; > >> } > >> } > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return err; > >> } > >> > >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> - int ret; > >> > >> if (!priv->txpp_en) { > >> /* Packet pacing is already disabled for the device. */ > >> return; > >> } > >> priv->txpp_en = 0; > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> MLX5_ASSERT(sh->txpp.refcnt); > >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return; > >> } > >> /* No references any more, do actual destroy. */ > >> mlx5_txpp_destroy(sh); > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> } > >> > >> /* > >> -- > >> 2.18.1 > > [-- Attachment #2: Type: text/html, Size: 18346 bytes --]
[-- Attachment #1: Type: text/plain, Size: 9242 bytes --] Hi, I send the v6 patch. Best regards, Chengfeng ________________________________ 发件人: YE Chengfeng <cyeaa@connect.ust.hk> 发送时间: 2021年11月12日 2:31 收件人: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org <dev@dpdk.org> 抄送: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org <stable@dpdk.org> 主题: Re: [PATCH] net/mlx5: remove redundant "set used" Got it. 获取 Outlook for iOS<https://aka.ms/o0ukef> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com> 发送时间: Friday, November 12, 2021 12:47:04 AM 收件人: YE Chengfeng <cyeaa@connect.ust.hk>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org <dev@dpdk.org> 抄送: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org <stable@dpdk.org> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" From: YE Chengfeng <cyeaa@connect.ust.hk> Sent: Thursday, November 11, 2021 18:08 To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org Cc: Raslan Darawsheh <rasland@nvidia.com>; Matan Azrad <matan@nvidia.com>; stable@dpdk.org Subject: Re: [PATCH] net/mlx5: remove redundant "set used" No problem, I will send the new patch. [SO] Thank you. But what about the commit message and title, should I use the previous one? net/mlx5: fix mutex unlock in txpp cleanup [SO] Please, send as the next version (v6?) of your patch. Just squash my proposals and keep your title (“net/mlx5: fix mutex unlock in txpp cleanup”) and commit message. With best regards, Slava 获取 Outlook for iOS<https://apc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Faka.ms%2Fo0ukef&data=04%7C01%7Ccyeaa%40connect.ust.hk%7Cb054bd8f8f3c42e1f0bb08d9a532e5d5%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722460307163598%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=3v%2BIAEdHSfsB%2BEoAmKldFqybeHCC3ihLZ6HSnWzkTD0%3D&reserved=0> ________________________________ 发件人: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> 发送时间: Thursday, November 11, 2021 8:27:30 PM 收件人: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>>; dev@dpdk.org<mailto:dev@dpdk.org> <dev@dpdk.org<mailto:dev@dpdk.org>> 抄送: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> <stable@dpdk.org<mailto:stable@dpdk.org>>; YE Chengfeng <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> 主题: RE: [PATCH] net/mlx5: remove redundant "set used" Hi, Ferruh > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>> > Sent: Thursday, November 11, 2021 14:08 > To: Slava Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>>; dev@dpdk.org<mailto:dev@dpdk.org> > Cc: Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; Matan Azrad > <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org>; Chengfeng Ye > <cyeaa@connect.ust.hk<mailto:cyeaa@connect.ust.hk>> > Subject: Re: [PATCH] net/mlx5: remove redundant "set used" > > On 11/11/2021 8:59 AM, Slava Ovsiienko wrote: > > Hi, Ferruh > > > > I've also inspected the mlx5 PMD code for RTE_SET_USED() for the > > similar issues related to the MLX5_ASSERT(). > > > > The patch > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7C87553a5cd43647db708008d9a50ea321%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722304566828185%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=woTMghsE0S7ckoo0AI01YbdWrJeLtbMCu4GeE8JDiUc%3D&reserved=0<https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211111084751.26721-1-&data=04%7C01%7Ccyeaa%40connect.ust.hk%7Cb054bd8f8f3c42e1f0bb08d9a532e5d5%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C637722460307173555%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=FH1BEOdaDn4vgLHJ%2BjV8DrH9kZIVCVpoVUEnPB2GHWw%3D&reserved=0> > viac > > heslavo@nvidia.com/<mailto:heslavo@nvidia.com/> > > should refine the few found ones. > > > > I do not mind about squashing with "net/mlx5: fix mutex unlock in txpp > cleanup" > > After getting this code in Upstream will care about the version for LTS. > > > > It will cause additional complexity for the LTS, since a small part of the below > fix will be originated from Chengfeng's change. To help LTS, what do you think > - First get your fix on top of current task > - Have a new version from Chengfeng on top of latest head, with 'claim_zero' > usage? Would be nice, I have no any objections. Chengfeng, could you please, squash (or write by yourself) my proposed updates and send the next version of your patch with "claim_zero()"? > So only your update need to be merged to LTS releases. Yes, agree, it is even better than my proposal. With best regards, Slava > > > >> -----Original Message----- > >> From: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> Sent: Thursday, November 11, 2021 10:48 > >> To: dev@dpdk.org<mailto:dev@dpdk.org> > >> Cc: ferruh.yigit@intel.com<mailto:ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@nvidia.com<mailto:rasland@nvidia.com>>; > >> Matan Azrad <matan@nvidia.com<mailto:matan@nvidia.com>>; stable@dpdk.org<mailto:stable@dpdk.org> > >> Subject: [PATCH] net/mlx5: remove redundant "set used" > >> > >> The patch just refines the code and replaces the pairs of > >> MLX5_ASSERT() and > >> RTE_SET_USED() with equivalent claim_zero(). > >> > >> Cc: stable@dpdk.org<mailto:stable@dpdk.org> > >> > >> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com<mailto:viacheslavo@nvidia.com>> > >> --- > >> drivers/net/mlx5/mlx5_txpp.c | 30 ++++++++++-------------------- > >> 1 file changed, 10 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/net/mlx5/mlx5_txpp.c > >> b/drivers/net/mlx5/mlx5_txpp.c index 73626f0e8f..af77e91e4c 100644 > >> --- a/drivers/net/mlx5/mlx5_txpp.c > >> +++ b/drivers/net/mlx5/mlx5_txpp.c > >> @@ -890,7 +890,6 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> int err = 0; > >> - int ret; > >> > >> if (!priv->config.tx_pp) { > >> /* Packet pacing is not requested for the device. */ @@ - > >> 903,14 +902,14 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> return 0; > >> } > >> if (priv->config.tx_pp > 0) { > >> - ret = rte_mbuf_dynflag_lookup > >> - > >> (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, NULL); > >> - if (ret < 0) > >> + err = rte_mbuf_dynflag_lookup > >> + (RTE_MBUF_DYNFLAG_TX_TIMESTAMP_NAME, > >> NULL); > >> + /* No flag registered means no service needed. */ > >> + if (err < 0) > >> return 0; > >> + err = 0; > >> } > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> if (sh->txpp.refcnt) { > >> priv->txpp_en = 1; > >> ++sh->txpp.refcnt; > >> @@ -924,9 +923,7 @@ mlx5_txpp_start(struct rte_eth_dev *dev) > >> rte_errno = -err; > >> } > >> } > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return err; > >> } > >> > >> @@ -944,28 +941,21 @@ mlx5_txpp_stop(struct rte_eth_dev *dev) { > >> struct mlx5_priv *priv = dev->data->dev_private; > >> struct mlx5_dev_ctx_shared *sh = priv->sh; > >> - int ret; > >> > >> if (!priv->txpp_en) { > >> /* Packet pacing is already disabled for the device. */ > >> return; > >> } > >> priv->txpp_en = 0; > >> - ret = pthread_mutex_lock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_lock(&sh->txpp.mutex)); > >> MLX5_ASSERT(sh->txpp.refcnt); > >> if (!sh->txpp.refcnt || --sh->txpp.refcnt) { > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> return; > >> } > >> /* No references any more, do actual destroy. */ > >> mlx5_txpp_destroy(sh); > >> - ret = pthread_mutex_unlock(&sh->txpp.mutex); > >> - MLX5_ASSERT(!ret); > >> - RTE_SET_USED(ret); > >> + claim_zero(pthread_mutex_unlock(&sh->txpp.mutex)); > >> } > >> > >> /* > >> -- > >> 2.18.1 > > [-- Attachment #2: Type: text/html, Size: 19863 bytes --]