[dpdk-dev,2/2] test/memzone: handle previously allocated memzones
Checks
Commit Message
Currently, memzone autotest expects there to be no memzones
present by the time the test is run. Some hardware drivers
will allocate memzones for internal use during initialization,
resulting in tests failing due to unexpected memzones being
allocated before the test was run.
Fix this by making callback increment a counter instead. This
also doubles as a test for correct operation of memzone_walk().
Fixes: 71330483a193 ("test/memzone: fix memory leak")
Cc: radoslaw.biernacki@linaro.org
Cc: stable@dpdk.org
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
test/test/test_memzone.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
Comments
Thanks, looks OK for me.
Reviewed-by: Radoslaw Biernacki <r <ferruh.yigit@intel.com>
adoslaw.biernacki@linaro.com>
On 26 January 2018 at 18:40, Anatoly Burakov <anatoly.burakov@intel.com>
wrote:
> Currently, memzone autotest expects there to be no memzones
> present by the time the test is run. Some hardware drivers
> will allocate memzones for internal use during initialization,
> resulting in tests failing due to unexpected memzones being
> allocated before the test was run.
>
> Fix this by making callback increment a counter instead. This
> also doubles as a test for correct operation of memzone_walk().
>
> Fixes: 71330483a193 ("test/memzone: fix memory leak")
> Cc: radoslaw.biernacki@linaro.org
> Cc: stable@dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> test/test/test_memzone.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/test/test/test_memzone.c b/test/test/test_memzone.c
> index 00d340f..5428b35 100644
> --- a/test/test/test_memzone.c
> +++ b/test/test/test_memzone.c
> @@ -953,16 +953,19 @@ test_memzone_basic(void)
> return 0;
> }
>
> -static int memzone_walk_called;
> +static int memzone_walk_cnt;
> static void memzone_walk_clb(const struct rte_memzone *mz __rte_unused,
> void *arg __rte_unused)
> {
> - memzone_walk_called = 1;
> + memzone_walk_cnt++;
> }
>
> static int
> test_memzone(void)
> {
> + /* take note of how many memzones were allocated before running */
> + int memzone_cnt = rte_eal_get_configuration()->
> mem_config->memzone_cnt;
> +
> printf("test basic memzone API\n");
> if (test_memzone_basic() < 0)
> return -1;
> @@ -1000,8 +1003,9 @@ test_memzone(void)
> return -1;
>
> printf("check memzone cleanup\n");
> + memzone_walk_cnt = 0;
> rte_memzone_walk(memzone_walk_clb, NULL);
> - if (memzone_walk_called) {
> + if (memzone_walk_cnt != memzone_cnt) {
> printf("there are some memzones left after test\n");
> rte_memzone_dump(stdout);
> return -1;
> --
> 2.7.4
>
Hi Anatoly,
I think your fix is elegant, however you can't grantee it doesn't have dirty memzone remained after memzone autotest.
What if some existed initial memzone released during the test and some dirty memzone remained. The counter cannot illustrate this state.
My fix just care about the memzone used in memzone autotest. It is rough but it seems more reliable. 😊
Thanks,
Phil Yang
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
> Sent: Saturday, January 27, 2018 1:41 AM
> To: dev@dpdk.org
> Cc: radoslaw.biernacki@linaro.org; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 2/2] test/memzone: handle previously allocated
> memzones
>
> Currently, memzone autotest expects there to be no memzones present by the
> time the test is run. Some hardware drivers will allocate memzones for internal
> use during initialization, resulting in tests failing due to unexpected memzones
> being allocated before the test was run.
>
> Fix this by making callback increment a counter instead. This also doubles as a
> test for correct operation of memzone_walk().
>
> Fixes: 71330483a193 ("test/memzone: fix memory leak")
> Cc: radoslaw.biernacki@linaro.org
> Cc: stable@dpdk.org
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> test/test/test_memzone.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/test/test/test_memzone.c b/test/test/test_memzone.c index
> 00d340f..5428b35 100644
> --- a/test/test/test_memzone.c
> +++ b/test/test/test_memzone.c
> @@ -953,16 +953,19 @@ test_memzone_basic(void)
> return 0;
> }
>
> -static int memzone_walk_called;
> +static int memzone_walk_cnt;
> static void memzone_walk_clb(const struct rte_memzone *mz __rte_unused,
> void *arg __rte_unused)
> {
> - memzone_walk_called = 1;
> + memzone_walk_cnt++;
> }
>
> static int
> test_memzone(void)
> {
> + /* take note of how many memzones were allocated before running */
> + int memzone_cnt =
> +rte_eal_get_configuration()->mem_config->memzone_cnt;
> +
> printf("test basic memzone API\n");
> if (test_memzone_basic() < 0)
> return -1;
> @@ -1000,8 +1003,9 @@ test_memzone(void)
> return -1;
>
> printf("check memzone cleanup\n");
> + memzone_walk_cnt = 0;
> rte_memzone_walk(memzone_walk_clb, NULL);
> - if (memzone_walk_called) {
> + if (memzone_walk_cnt != memzone_cnt) {
> printf("there are some memzones left after test\n");
> rte_memzone_dump(stdout);
> return -1;
> --
> 2.7.4
On 31-Jan-18 7:51 AM, Phil Yang wrote:
> Hi Anatoly,
>
> I think your fix is elegant, however you can't grantee it doesn't have dirty memzone remained after memzone autotest.
> What if some existed initial memzone released during the test and some dirty memzone remained. The counter cannot illustrate this state.
>
> My fix just care about the memzone used in memzone autotest. It is rough but it seems more reliable. 😊
>
> Thanks,
> Phil Yang
We could combine the approaches. That way, we both ensure that no
memzones were left in that should've been freed, and that total number
of memzones didn't change as well (i.e. we didn't allocate/free any
memzones we weren't supposed to allocate/free).
As i side note, i think making a #define with memzone prefix in your
patch will work better and will be less copypaste-error-prone in the
long run.
I will prepare a v2 combining both approaches. Is that OK with you?
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
>> Sent: Saturday, January 27, 2018 1:41 AM
>> To: dev@dpdk.org
>> Cc: radoslaw.biernacki@linaro.org; stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH 2/2] test/memzone: handle previously allocated
>> memzones
>>
>> Currently, memzone autotest expects there to be no memzones present by the
>> time the test is run. Some hardware drivers will allocate memzones for internal
>> use during initialization, resulting in tests failing due to unexpected memzones
>> being allocated before the test was run.
>>
>> Fix this by making callback increment a counter instead. This also doubles as a
>> test for correct operation of memzone_walk().
>>
>> Fixes: 71330483a193 ("test/memzone: fix memory leak")
>> Cc: radoslaw.biernacki@linaro.org
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
That is OK for me. Thanks for your comments.
Thanks,
Phil Yang
> -----Original Message-----
> From: Burakov, Anatoly [mailto:anatoly.burakov@intel.com]
> Sent: Wednesday, January 31, 2018 6:05 PM
> To: Phil Yang <Phil.Yang@arm.com>; dev@dpdk.org
> Cc: radoslaw.biernacki@linaro.org; stable@dpdk.org; nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 2/2] test/memzone: handle previously allocated
> memzones
>
> On 31-Jan-18 7:51 AM, Phil Yang wrote:
> > Hi Anatoly,
> >
> > I think your fix is elegant, however you can't grantee it doesn't have dirty
> memzone remained after memzone autotest.
> > What if some existed initial memzone released during the test and some dirty
> memzone remained. The counter cannot illustrate this state.
> >
> > My fix just care about the memzone used in memzone autotest. It is
> > rough but it seems more reliable. 😊
> >
> > Thanks,
> > Phil Yang
>
> We could combine the approaches. That way, we both ensure that no
> memzones were left in that should've been freed, and that total number of
> memzones didn't change as well (i.e. we didn't allocate/free any memzones we
> weren't supposed to allocate/free).
>
> As i side note, i think making a #define with memzone prefix in your patch will
> work better and will be less copypaste-error-prone in the long run.
>
> I will prepare a v2 combining both approaches. Is that OK with you?
>
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anatoly Burakov
> >> Sent: Saturday, January 27, 2018 1:41 AM
> >> To: dev@dpdk.org
> >> Cc: radoslaw.biernacki@linaro.org; stable@dpdk.org
> >> Subject: [dpdk-dev] [PATCH 2/2] test/memzone: handle previously
> >> allocated memzones
> >>
> >> Currently, memzone autotest expects there to be no memzones present
> >> by the time the test is run. Some hardware drivers will allocate
> >> memzones for internal use during initialization, resulting in tests
> >> failing due to unexpected memzones being allocated before the test was run.
> >>
> >> Fix this by making callback increment a counter instead. This also
> >> doubles as a test for correct operation of memzone_walk().
> >>
> >> Fixes: 71330483a193 ("test/memzone: fix memory leak")
> >> Cc: radoslaw.biernacki@linaro.org
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
>
> --
> Thanks,
> Anatoly
@@ -953,16 +953,19 @@ test_memzone_basic(void)
return 0;
}
-static int memzone_walk_called;
+static int memzone_walk_cnt;
static void memzone_walk_clb(const struct rte_memzone *mz __rte_unused,
void *arg __rte_unused)
{
- memzone_walk_called = 1;
+ memzone_walk_cnt++;
}
static int
test_memzone(void)
{
+ /* take note of how many memzones were allocated before running */
+ int memzone_cnt = rte_eal_get_configuration()->mem_config->memzone_cnt;
+
printf("test basic memzone API\n");
if (test_memzone_basic() < 0)
return -1;
@@ -1000,8 +1003,9 @@ test_memzone(void)
return -1;
printf("check memzone cleanup\n");
+ memzone_walk_cnt = 0;
rte_memzone_walk(memzone_walk_clb, NULL);
- if (memzone_walk_called) {
+ if (memzone_walk_cnt != memzone_cnt) {
printf("there are some memzones left after test\n");
rte_memzone_dump(stdout);
return -1;