Re: [dpdk-dev] [PATCH v4] app/test: add tests for atomic exchanges

[David Marchand <david.marchand@redhat.com>]

On Fri, Oct 18, 2019 at 10:31 PM David Christensen
<drc@linux.vnet.ibm.com> wrote:
>
> The test works by creating a token comprised of random data
> and a CRC8 value, using the rte_atomicXX_exchange to exchange
> the new token for a previously generated token, and then
> verifying that the exchanged data is intact (i.e. the CRC8
> is still correct for the data).

Thanks, can you rebase this on master?

>
> Signed-off-by: David Christensen <drc@linux.vnet.ibm.com>
> ---
> v4:
> * Fix build error due to use of variable initialization
>   in "for" statement.
>
> v3:
> * Actually fixed build issue on all platforms caused by
>   misspelling of rte_atomic64_inc
>
> v2:
> * Fixed build issue on all platforms caused by misspelling
>   of rte_atomic64_inc
>
>  app/test/test_atomic.c | 176 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 174 insertions(+), 2 deletions(-)
>
> diff --git a/app/test/test_atomic.c b/app/test/test_atomic.c
> index 43be30ec0..858c6d7f9 100644
> --- a/app/test/test_atomic.c
> +++ b/app/test/test_atomic.c
> @@ -1,10 +1,12 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright(c) 2010-2014 Intel Corporation
> + * Copyright(c) 2019 Arm Limited
>   */
>
>  #include <stdio.h>
>  #include <stdint.h>
>  #include <unistd.h>
> +#include <inttypes.h>
>  #include <sys/queue.h>
>
>  #include <rte_memory.h>
> @@ -13,6 +15,8 @@
>  #include <rte_atomic.h>
>  #include <rte_eal.h>
>  #include <rte_lcore.h>
> +#include <rte_random.h>
> +#include <rte_hash_crc.h>
>
>  #include "test.h"
>
> @@ -20,7 +24,7 @@
>   * Atomic Variables
>   * ================
>   *
> - * - The main test function performs three subtests. The first test
> + * - The main test function performs four subtests. The first test

Let's drop this sentence.
No point in maintaining the number of subtests.

>   *   checks that the usual inc/dec/add/sub functions are working
>   *   correctly:
>   *
> @@ -61,11 +65,26 @@
>   *       atomic_sub(&count, tmp+1);
>   *
>   *   - At the end of the test, the *count* value must be 0.
> + *
> + * - Test "atomic exchange"
> + *
> + *   - Create a 64 bit token that can be tested for data integrity
> + *
> + *   - Invoke ``test_atomic_exchange`` on each lcore.  Before doing
> + *     anything else, the cores wait for a synchronization event.
> + *     Each core then does the follwoing for N iterations:
> + *
> + *       Generate a new token with a data integrity check
> + *       Exchange the new token for previously generated token
> + *       Increment a counter if a corrupt token was received
> + *
> + *   - At the end of the test, the number of corrupted tokens must be 0.
> + *
>   */
>
>  #define NUM_ATOMIC_TYPES 3
>
> -#define N 10000
> +#define N 1000000
>
>  static rte_atomic16_t a16;
>  static rte_atomic32_t a32;
> @@ -216,6 +235,127 @@ test_atomic_dec_and_test(__attribute__((unused)) void *arg)
>         return 0;
>  }
>
> +/*
> + * Helper definitions/variables/functions for
> + * atomic exchange tests
> + */
> +typedef union {
> +       uint16_t u16;
> +       uint8_t  u8[2];
> +} rte_u16_t;
> +
> +typedef union {
> +       uint32_t u32;
> +       uint16_t u16[2];
> +       uint8_t  u8[4];
> +} rte_u32_t;
> +
> +typedef union {
> +       uint64_t u64;
> +       uint32_t u32[2];
> +       uint16_t u16[4];
> +       uint8_t  u8[8];
> +} rte_u64_t;

Please, don't use such names for internal types.
It gives the impression those are EAL types.. but I don't feel like we
need them, so let's just avoid adding them in EAL, now.



-- 
David Marchand