diff mbox series

lib/mempool : rte_mempool_avail_count, fixing return bigger than mempool size

Message ID 20230516134146.480047-1-yasinncaner@gmail.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series lib/mempool : rte_mempool_avail_count, fixing return bigger than mempool size |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS

Commit Message

Yasin CANER May 16, 2023, 1:41 p.m. UTC 
  From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>

after a while working rte_mempool_avail_count function returns bigger
than mempool size that cause miscalculation rte_mempool_in_use_count.

it helps to avoid miscalculation rte_mempool_in_use_count.

Bugzilla ID: 1229

Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
---
 lib/mempool/rte_mempool.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger May 16, 2023, 3:23 p.m. UTC | #1 
On Tue, 16 May 2023 13:41:46 +0000
Yasin CANER <yasinncaner@gmail.com> wrote:

> From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> 
> after a while working rte_mempool_avail_count function returns bigger
> than mempool size that cause miscalculation rte_mempool_in_use_count.
> 
> it helps to avoid miscalculation rte_mempool_in_use_count.
> 
> Bugzilla ID: 1229
> 
> Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>

An alternative that avoids some code duplication.

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index cf5dea2304a7..2406b112e7b0 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool *mp)
        count = rte_mempool_ops_get_count(mp);
 
        if (mp->cache_size == 0)
-               return count;
+               goto exit;
 
        for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
                count += mp->local_cache[lcore_id].len;
@@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool *mp)
         * due to race condition (access to len is not locked), the
         * total can be greater than size... so fix the result
         */
+exit:
        if (count > mp->size)
                return mp->size;
        return count;
Morten Brørup May 16, 2023, 4:03 p.m. UTC | #2 
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Tuesday, 16 May 2023 17.24
> 
> On Tue, 16 May 2023 13:41:46 +0000
> Yasin CANER <yasinncaner@gmail.com> wrote:
> 
> > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> >
> > after a while working rte_mempool_avail_count function returns bigger
> > than mempool size that cause miscalculation rte_mempool_in_use_count.
> >
> > it helps to avoid miscalculation rte_mempool_in_use_count.
> >
> > Bugzilla ID: 1229
> >
> > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> 
> An alternative that avoids some code duplication.
> 
> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> index cf5dea2304a7..2406b112e7b0 100644
> --- a/lib/mempool/rte_mempool.c
> +++ b/lib/mempool/rte_mempool.c
> @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
> *mp)
>         count = rte_mempool_ops_get_count(mp);
> 
>         if (mp->cache_size == 0)
> -               return count;
> +               goto exit;

This bug can only occur here (i.e. with cache_size==0) if rte_mempool_ops_get_count() returns an incorrect value. The bug should be fixed there instead.

> 
>         for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
>                 count += mp->local_cache[lcore_id].len;
> @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
> *mp)
>          * due to race condition (access to len is not locked), the
>          * total can be greater than size... so fix the result
>          */
> +exit:
>         if (count > mp->size)
>                 return mp->size;
>         return count;
Yasin CANER May 17, 2023, 8:01 a.m. UTC | #3 
Hello,

I don't have full knowledge of how to work rte_mempool_ops_get_count() but
there is another comment about it. Maybe it relates.
/*
 * due to race condition (access to len is not locked), the
 * total can be greater than size... so fix the result
 */

Best regards.

Morten Brørup <mb@smartsharesystems.com>, 16 May 2023 Sal, 19:04 tarihinde
şunu yazdı:

> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, 16 May 2023 17.24
> >
> > On Tue, 16 May 2023 13:41:46 +0000
> > Yasin CANER <yasinncaner@gmail.com> wrote:
> >
> > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > >
> > > after a while working rte_mempool_avail_count function returns bigger
> > > than mempool size that cause miscalculation rte_mempool_in_use_count.
> > >
> > > it helps to avoid miscalculation rte_mempool_in_use_count.
> > >
> > > Bugzilla ID: 1229
> > >
> > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> >
> > An alternative that avoids some code duplication.
> >
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index cf5dea2304a7..2406b112e7b0 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > *mp)
> >         count = rte_mempool_ops_get_count(mp);
> >
> >         if (mp->cache_size == 0)
> > -               return count;
> > +               goto exit;
>
> This bug can only occur here (i.e. with cache_size==0) if
> rte_mempool_ops_get_count() returns an incorrect value. The bug should be
> fixed there instead.
>
> >
> >         for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> >                 count += mp->local_cache[lcore_id].len;
> > @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > *mp)
> >          * due to race condition (access to len is not locked), the
> >          * total can be greater than size... so fix the result
> >          */
> > +exit:
> >         if (count > mp->size)
> >                 return mp->size;
> >         return count;
>
Morten Brørup May 17, 2023, 8:38 a.m. UTC | #4 
From: Yasin CANER [mailto:yasinncaner@gmail.com] 
Sent: Wednesday, 17 May 2023 10.01
To: Morten Brørup
Cc: Stephen Hemminger; dev@dpdk.org; Yasin CANER; Olivier Matz; Andrew Rybchenko
Subject: Re: [PATCH] lib/mempool : rte_mempool_avail_count, fixing return bigger than mempool size

 

Hello,

 

I don't have full knowledge of how to work rte_mempool_ops_get_count() but there is another comment about it. Maybe it relates. 

/*
 * due to race condition (access to len is not locked), the
 * total can be greater than size... so fix the result
 */

 

MB: This comment relates to the race when accessing the per-lcore cache counters.

 

Best regards.

 

Morten Brørup <mb@smartsharesystems.com>, 16 May 2023 Sal, 19:04 tarihinde şunu yazdı:

	> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
	> Sent: Tuesday, 16 May 2023 17.24
	> 
	> On Tue, 16 May 2023 13:41:46 +0000
	> Yasin CANER <yasinncaner@gmail.com> wrote:
	> 
	> > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> >
	> > after a while working rte_mempool_avail_count function returns bigger
	> > than mempool size that cause miscalculation rte_mempool_in_use_count.
	> >
	> > it helps to avoid miscalculation rte_mempool_in_use_count.
	> >
	> > Bugzilla ID: 1229
	> >
	> > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> 
	> An alternative that avoids some code duplication.
	> 
	> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
	> index cf5dea2304a7..2406b112e7b0 100644
	> --- a/lib/mempool/rte_mempool.c
	> +++ b/lib/mempool/rte_mempool.c
	> @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
	> *mp)
	>         count = rte_mempool_ops_get_count(mp);
	> 
	>         if (mp->cache_size == 0)
	> -               return count;
	> +               goto exit;
	
	This bug can only occur here (i.e. with cache_size==0) if rte_mempool_ops_get_count() returns an incorrect value. The bug should be fixed there instead.
	
	> 
	>         for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
	>                 count += mp->local_cache[lcore_id].len;
	> @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
	> *mp)
	>          * due to race condition (access to len is not locked), the
	>          * total can be greater than size... so fix the result
	>          */
	> +exit:
	>         if (count > mp->size)
	>                 return mp->size;
	>         return count;
Morten Brørup May 17, 2023, 9:04 a.m. UTC | #5 
From: Morten Brørup [mailto:mb@smartsharesystems.com] 
Sent: Wednesday, 17 May 2023 10.38



From: Yasin CANER [mailto:yasinncaner@gmail.com] 
Sent: Wednesday, 17 May 2023 10.01



Hello,

 

I don't have full knowledge of how to work rte_mempool_ops_get_count() but there is another comment about it. Maybe it relates. 

/*
 * due to race condition (access to len is not locked), the
 * total can be greater than size... so fix the result
 */

 

MB: This comment relates to the race when accessing the per-lcore cache counters.

 

MB (continued): I have added more information, regarding mempool drivers, in Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=1229

 

 

Best regards.

 

Morten Brørup <mb@smartsharesystems.com>, 16 May 2023 Sal, 19:04 tarihinde şunu yazdı:

	> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
	> Sent: Tuesday, 16 May 2023 17.24
	> 
	> On Tue, 16 May 2023 13:41:46 +0000
	> Yasin CANER <yasinncaner@gmail.com> wrote:
	> 
	> > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> >
	> > after a while working rte_mempool_avail_count function returns bigger
	> > than mempool size that cause miscalculation rte_mempool_in_use_count.
	> >
	> > it helps to avoid miscalculation rte_mempool_in_use_count.
	> >
	> > Bugzilla ID: 1229
	> >
	> > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> 
	> An alternative that avoids some code duplication.
	> 
	> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
	> index cf5dea2304a7..2406b112e7b0 100644
	> --- a/lib/mempool/rte_mempool.c
	> +++ b/lib/mempool/rte_mempool.c
	> @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
	> *mp)
	>         count = rte_mempool_ops_get_count(mp);
	> 
	>         if (mp->cache_size == 0)
	> -               return count;
	> +               goto exit;
	
	This bug can only occur here (i.e. with cache_size==0) if rte_mempool_ops_get_count() returns an incorrect value. The bug should be fixed there instead.

	 

	MB (continued): The bug must be in the underlying mempool driver. I took a look at the ring and stack drivers, and they seem fine.

	
	
	> 
	>         for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
	>                 count += mp->local_cache[lcore_id].len;
	> @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
	> *mp)
	>          * due to race condition (access to len is not locked), the
	>          * total can be greater than size... so fix the result
	>          */
	> +exit:
	>         if (count > mp->size)
	>                 return mp->size;
	>         return count;
Yasin CANER May 17, 2023, 11:37 a.m. UTC | #6 
Hello,

mempool drivers is listed here.
https://doc.dpdk.org/guides/mempool/index.html

my app loads *rte_mempool_ring* and also *rte_mempool_stack* . In doc,
rte_mempool_ring is default mempool driver.

"-d librte_mbuf.so -d librte_mempool.so -d librte_mempool_ring.so -d
librte_mempool_stack.so -d librte_mempool_bucket.so -d librte_kni.so"

EAL: open shared lib librte_mbuf.so
EAL: open shared lib librte_mempool.so
EAL: open shared lib librte_mempool_ring.so
EAL: open shared lib librte_mempool_stack.so
EAL: lib.stack log level changed from disabled to notice
EAL: open shared lib librte_mempool_bucket.so
EAL: open shared lib librte_kni.so
EAL: open shared lib
DPDK_LIBS/lib/x86_64-linux-gnu/dpdk/pmds-23.0/librte_mempool_octeontx.so
EAL: pmd.octeontx.mbox log level changed from disabled to notice
EAL: pmd.mempool.octeontx log level changed from disabled to notice



Morten Brørup <mb@smartsharesystems.com>, 17 May 2023 Çar, 12:04 tarihinde
şunu yazdı:

> *From:* Morten Brørup [mailto:mb@smartsharesystems.com]
> *Sent:* Wednesday, 17 May 2023 10.38
>
> *From:* Yasin CANER [mailto:yasinncaner@gmail.com]
> *Sent:* Wednesday, 17 May 2023 10.01
>
> Hello,
>
>
>
> I don't have full knowledge of how to work rte_mempool_ops_get_count() but
> there is another comment about it. Maybe it relates.
>
> /*
>  * due to race condition (access to len is not locked), the
>  * total can be greater than size... so fix the result
>  */
>
>
>
> MB: This comment relates to the race when accessing the per-lcore cache
> counters.
>
>
>
> MB (continued): I have added more information, regarding mempool drivers,
> in Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=1229
>
>
>
>
>
> Best regards.
>
>
>
> Morten Brørup <mb@smartsharesystems.com>, 16 May 2023 Sal, 19:04
> tarihinde şunu yazdı:
>
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Tuesday, 16 May 2023 17.24
> >
> > On Tue, 16 May 2023 13:41:46 +0000
> > Yasin CANER <yasinncaner@gmail.com> wrote:
> >
> > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > >
> > > after a while working rte_mempool_avail_count function returns bigger
> > > than mempool size that cause miscalculation rte_mempool_in_use_count.
> > >
> > > it helps to avoid miscalculation rte_mempool_in_use_count.
> > >
> > > Bugzilla ID: 1229
> > >
> > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> >
> > An alternative that avoids some code duplication.
> >
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index cf5dea2304a7..2406b112e7b0 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > *mp)
> >         count = rte_mempool_ops_get_count(mp);
> >
> >         if (mp->cache_size == 0)
> > -               return count;
> > +               goto exit;
>
> This bug can only occur here (i.e. with cache_size==0) if
> rte_mempool_ops_get_count() returns an incorrect value. The bug should be
> fixed there instead.
>
>
>
> MB (continued): The bug must be in the underlying mempool driver. I took a
> look at the ring and stack drivers, and they seem fine.
>
>
>
> >
> >         for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
> >                 count += mp->local_cache[lcore_id].len;
> > @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > *mp)
> >          * due to race condition (access to len is not locked), the
> >          * total can be greater than size... so fix the result
> >          */
> > +exit:
> >         if (count > mp->size)
> >                 return mp->size;
> >         return count;
>
>
David Marchand May 17, 2023, 11:53 a.m. UTC | #7 
On Wed, May 17, 2023 at 11:05 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > On Tue, 16 May 2023 13:41:46 +0000
> > Yasin CANER <yasinncaner@gmail.com> wrote:
> >
> > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > >
> > > after a while working rte_mempool_avail_count function returns bigger
> > > than mempool size that cause miscalculation rte_mempool_in_use_count.
> > >
> > > it helps to avoid miscalculation rte_mempool_in_use_count.

Is this issue reproduced with an application of the reporter, or a
DPDK in-tree application?


> > >
> > > Bugzilla ID: 1229
> > >
> > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> >
> > An alternative that avoids some code duplication.
> >
> > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > index cf5dea2304a7..2406b112e7b0 100644
> > --- a/lib/mempool/rte_mempool.c
> > +++ b/lib/mempool/rte_mempool.c
> > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > *mp)
> >         count = rte_mempool_ops_get_count(mp);
> >
> >         if (mp->cache_size == 0)
> > -               return count;
> > +               goto exit;
>
> This bug can only occur here (i.e. with cache_size==0) if rte_mempool_ops_get_count() returns an incorrect value. The bug should be fixed there instead.
>
>
>
> MB (continued): The bug must be in the underlying mempool driver. I took a look at the ring and stack drivers, and they seem fine.

Or it could indicate a double free (or equivalent) issue from the
application (either through direct call to mempool API, or indirectly
like sending/freeing an already sent/freed packet for example).
Morten Brørup May 17, 2023, 12:23 p.m. UTC | #8 
> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 17 May 2023 13.53
> 
> On Wed, May 17, 2023 at 11:05 AM Morten Brørup <mb@smartsharesystems.com>
> wrote:
> > > On Tue, 16 May 2023 13:41:46 +0000
> > > Yasin CANER <yasinncaner@gmail.com> wrote:
> > >
> > > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > > >
> > > > after a while working rte_mempool_avail_count function returns bigger
> > > > than mempool size that cause miscalculation rte_mempool_in_use_count.
> > > >
> > > > it helps to avoid miscalculation rte_mempool_in_use_count.
> 
> Is this issue reproduced with an application of the reporter, or a
> DPDK in-tree application?
> 
> 
> > > >
> > > > Bugzilla ID: 1229
> > > >
> > > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > >
> > > An alternative that avoids some code duplication.
> > >
> > > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > > index cf5dea2304a7..2406b112e7b0 100644
> > > --- a/lib/mempool/rte_mempool.c
> > > +++ b/lib/mempool/rte_mempool.c
> > > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
> > > *mp)
> > >         count = rte_mempool_ops_get_count(mp);
> > >
> > >         if (mp->cache_size == 0)
> > > -               return count;
> > > +               goto exit;
> >
> > This bug can only occur here (i.e. with cache_size==0) if
> rte_mempool_ops_get_count() returns an incorrect value. The bug should be
> fixed there instead.
> >
> >
> >
> > MB (continued): The bug must be in the underlying mempool driver. I took a
> look at the ring and stack drivers, and they seem fine.
> 
> Or it could indicate a double free (or equivalent) issue from the
> application (either through direct call to mempool API, or indirectly
> like sending/freeing an already sent/freed packet for example).

Good point, David.

@Yasin, if you build DPDK and your application with RTE_LIBRTE_MEMPOOL_DEBUG set in config/rte_config.h, the mempool cookies should catch any double frees.
Yasin CANER May 18, 2023, 8:37 a.m. UTC | #9 
Hello,

I found a second free command in my code and removed it. David pointed to
the right .

On the other hand, do you think we need to avoid miscalculations? Is it
better to patch it or not?

or it needs to be aware of the second free command.

Sharing more information about env.

# ethtool -i mgmt
driver: virtio_net
version: 1.0.0
firmware-version:
expansion-rom-version:
bus-info: 0000:00:03.0
supports-statistics: yes
supports-test: no
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: no

NAME="Ubuntu"
VERSION="20.04.4 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.4 LTS"
VERSION_ID="20.04"

Linux spgw-dpdk 5.4.0-146-generic #163-Ubuntu SMP Fri Mar 17 18:26:02 UTC
2023 x86_64 x86_64 x86_64 GNU/Linux



Best regards.

Morten Brørup <mb@smartsharesystems.com>, 17 May 2023 Çar, 15:23 tarihinde
şunu yazdı:

> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Wednesday, 17 May 2023 13.53
> >
> > On Wed, May 17, 2023 at 11:05 AM Morten Brørup <mb@smartsharesystems.com
> >
> > wrote:
> > > > On Tue, 16 May 2023 13:41:46 +0000
> > > > Yasin CANER <yasinncaner@gmail.com> wrote:
> > > >
> > > > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > > > >
> > > > > after a while working rte_mempool_avail_count function returns
> bigger
> > > > > than mempool size that cause miscalculation
> rte_mempool_in_use_count.
> > > > >
> > > > > it helps to avoid miscalculation rte_mempool_in_use_count.
> >
> > Is this issue reproduced with an application of the reporter, or a
> > DPDK in-tree application?
> >
> >
> > > > >
> > > > > Bugzilla ID: 1229
> > > > >
> > > > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
> > > >
> > > > An alternative that avoids some code duplication.
> > > >
> > > > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
> > > > index cf5dea2304a7..2406b112e7b0 100644
> > > > --- a/lib/mempool/rte_mempool.c
> > > > +++ b/lib/mempool/rte_mempool.c
> > > > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct
> rte_mempool
> > > > *mp)
> > > >         count = rte_mempool_ops_get_count(mp);
> > > >
> > > >         if (mp->cache_size == 0)
> > > > -               return count;
> > > > +               goto exit;
> > >
> > > This bug can only occur here (i.e. with cache_size==0) if
> > rte_mempool_ops_get_count() returns an incorrect value. The bug should be
> > fixed there instead.
> > >
> > >
> > >
> > > MB (continued): The bug must be in the underlying mempool driver. I
> took a
> > look at the ring and stack drivers, and they seem fine.
> >
> > Or it could indicate a double free (or equivalent) issue from the
> > application (either through direct call to mempool API, or indirectly
> > like sending/freeing an already sent/freed packet for example).
>
> Good point, David.
>
> @Yasin, if you build DPDK and your application with
> RTE_LIBRTE_MEMPOOL_DEBUG set in config/rte_config.h, the mempool cookies
> should catch any double frees.
>
>
Morten Brørup May 18, 2023, 8:53 a.m. UTC | #10 
From: Yasin CANER [mailto:yasinncaner@gmail.com] 
Sent: Thursday, 18 May 2023 10.37



Hello, 

 

I found a second free command in my code and removed it. David pointed to the right .

 

MB: Good to hear.

 

On the other hand, do you think we need to avoid miscalculations? Is it better to patch it or not?

 

MB: There is no bug in the library, so no need to patch it. The returned value was a consequence of using the library incorrectly (freeing twice).

 

or it needs to be aware of the second free command. 

 

Sharing more information about env.

 

# ethtool -i mgmt
driver: virtio_net
version: 1.0.0
firmware-version:
expansion-rom-version:
bus-info: 0000:00:03.0
supports-statistics: yes
supports-test: no
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: no

 

NAME="Ubuntu"
VERSION="20.04.4 LTS (Focal Fossa)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 20.04.4 LTS"
VERSION_ID="20.04"

 

Linux spgw-dpdk 5.4.0-146-generic #163-Ubuntu SMP Fri Mar 17 18:26:02 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

 

 

 

Best regards.

 

Morten Brørup <mb@smartsharesystems.com>, 17 May 2023 Çar, 15:23 tarihinde şunu yazdı:

	> From: David Marchand [mailto:david.marchand@redhat.com]
	> Sent: Wednesday, 17 May 2023 13.53
	> 
	> On Wed, May 17, 2023 at 11:05 AM Morten Brørup <mb@smartsharesystems.com>
	> wrote:
	> > > On Tue, 16 May 2023 13:41:46 +0000
	> > > Yasin CANER <yasinncaner@gmail.com> wrote:
	> > >
	> > > > From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> > > >
	> > > > after a while working rte_mempool_avail_count function returns bigger
	> > > > than mempool size that cause miscalculation rte_mempool_in_use_count.
	> > > >
	> > > > it helps to avoid miscalculation rte_mempool_in_use_count.
	> 
	> Is this issue reproduced with an application of the reporter, or a
	> DPDK in-tree application?
	> 
	> 
	> > > >
	> > > > Bugzilla ID: 1229
	> > > >
	> > > > Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
	> > >
	> > > An alternative that avoids some code duplication.
	> > >
	> > > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
	> > > index cf5dea2304a7..2406b112e7b0 100644
	> > > --- a/lib/mempool/rte_mempool.c
	> > > +++ b/lib/mempool/rte_mempool.c
	> > > @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
	> > > *mp)
	> > >         count = rte_mempool_ops_get_count(mp);
	> > >
	> > >         if (mp->cache_size == 0)
	> > > -               return count;
	> > > +               goto exit;
	> >
	> > This bug can only occur here (i.e. with cache_size==0) if
	> rte_mempool_ops_get_count() returns an incorrect value. The bug should be
	> fixed there instead.
	> >
	> >
	> >
	> > MB (continued): The bug must be in the underlying mempool driver. I took a
	> look at the ring and stack drivers, and they seem fine.
	> 
	> Or it could indicate a double free (or equivalent) issue from the
	> application (either through direct call to mempool API, or indirectly
	> like sending/freeing an already sent/freed packet for example).
	
	Good point, David.
	
	@Yasin, if you build DPDK and your application with RTE_LIBRTE_MEMPOOL_DEBUG set in config/rte_config.h, the mempool cookies should catch any double frees.
Mattias Rönnblom May 18, 2023, 3:40 p.m. UTC | #11 
On 2023-05-16 18:03, Morten Brørup wrote:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Tuesday, 16 May 2023 17.24
>>
>> On Tue, 16 May 2023 13:41:46 +0000
>> Yasin CANER <yasinncaner@gmail.com> wrote:
>>
>>> From: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
>>>
>>> after a while working rte_mempool_avail_count function returns bigger
>>> than mempool size that cause miscalculation rte_mempool_in_use_count.
>>>
>>> it helps to avoid miscalculation rte_mempool_in_use_count.
>>>
>>> Bugzilla ID: 1229
>>>
>>> Signed-off-by: Yasin CANER <yasin.caner@ulakhaberlesme.com.tr>
>>
>> An alternative that avoids some code duplication.
>>
>> diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
>> index cf5dea2304a7..2406b112e7b0 100644
>> --- a/lib/mempool/rte_mempool.c
>> +++ b/lib/mempool/rte_mempool.c
>> @@ -1010,7 +1010,7 @@ rte_mempool_avail_count(const struct rte_mempool
>> *mp)
>>          count = rte_mempool_ops_get_count(mp);
>>
>>          if (mp->cache_size == 0)
>> -               return count;
>> +               goto exit;
> 
> This bug can only occur here (i.e. with cache_size==0) if rte_mempool_ops_get_count() returns an incorrect value. The bug should be fixed there instead.
> 
>>
>>          for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
>>                  count += mp->local_cache[lcore_id].len;
>> @@ -1019,6 +1019,7 @@ rte_mempool_avail_count(const struct rte_mempool
>> *mp)

This loop may result in the same symptoms (i.e., count > mp->size). For 
example, the LF stack has a relaxed atomic load to retrieve the usage 
count, which may well be reordered (by the CPU and/or compiler) in 
relation to these non-atomic loads. The same issue may well exist on the 
writer side, with the order between the global and the cache counts not 
being ordered.

So while sanity checking the count without caching is not needed, with, 
you can argue it is.

>>           * due to race condition (access to len is not locked), the
>>           * total can be greater than size... so fix the result
>>           */
>> +exit:
>>          if (count > mp->size)
>>                  return mp->size;
>>          return count;
diff mbox series

Patch

diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index cf5dea2304..17ed1eb845 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -1009,8 +1009,11 @@  rte_mempool_avail_count(const struct rte_mempool *mp)
 
 	count = rte_mempool_ops_get_count(mp);
 
-	if (mp->cache_size == 0)
+	if (mp->cache_size == 0) {
+		if (count > mp->size)
+			return mp->size;
 		return count;
+	}
 
 	for (lcore_id = 0; lcore_id < RTE_MAX_LCORE; lcore_id++)
 		count += mp->local_cache[lcore_id].len;