Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230329180502.1884307-1-kal.conley@dectris.com> <20230329180502.1884307-5-kal.conley@dectris.com>
In-Reply-To: <20230329180502.1884307-5-kal.conley@dectris.com>
From:   Magnus Karlsson <magnus.karlsson@gmail.com>
Date:   Mon, 3 Apr 2023 12:54:52 +0200
Message-ID: <CAJ8uoz1cGV1_3HQQddbkExVnm=wngP3ECJZNS5gOtQtfi=mPnA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 04/10] selftests: xsk: Deflakify
 STATS_RX_DROPPED test
To:     Kal Conley <kal.conley@dectris.com>
Cc:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        Magnus Karlsson <magnus.karlsson@intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Jonathan Lemon <jonathan.lemon@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Mykola Lysenko <mykolal@fb.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Shuah Khan <shuah@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, linux-kselftest@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 29 Mar 2023 at 20:11, Kal Conley <kal.conley@dectris.com> wrote:
>
> Fix flaky STATS_RX_DROPPED test. The receiver calls getsockopt after
> receiving the last (valid) packet which is not the final packet sent in
> the test (valid and invalid packets are sent in alternating fashion with
> the final packet being invalid). Since the last packet may or may not
> have been dropped already, both outcomes must be allowed.
>
> This issue could also be fixed by making sure the last packet sent is
> valid. This alternative is left as an exercise to the reader (or the
> benevolent maintainers of this file).
>
> This problem was quite visible on certain setups. On one machine this
> failure was observed 50% of the time.
>
> Also, remove a redundant assignment of pkt_stream->nb_pkts. This field
> is already initialized by __pkt_stream_alloc.

This has been bugging me for a while so thanks for fixing this. Please
break this commit out of this patch set and send it as a separate bug
fix.

Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>

> Fixes: 27e934bec35b ("selftests: xsk: make stat tests not spin on getsockopt")
> Signed-off-by: Kal Conley <kal.conley@dectris.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 34a1f32fe752..1a4bdd5aa78c 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -633,7 +633,6 @@ static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info *umem, u32 nb
>         if (!pkt_stream)
>                 exit_with_error(ENOMEM);
>
> -       pkt_stream->nb_pkts = nb_pkts;
>         for (i = 0; i < nb_pkts; i++) {
>                 pkt_set(umem, &pkt_stream->pkts[i], (i % umem->num_frames) * umem->frame_size,
>                         pkt_len);
> @@ -1141,7 +1140,14 @@ static int validate_rx_dropped(struct ifobject *ifobject)
>         if (err)
>                 return TEST_FAILURE;
>
> -       if (stats.rx_dropped == ifobject->pkt_stream->nb_pkts / 2)
> +       /* The receiver calls getsockopt after receiving the last (valid)
> +        * packet which is not the final packet sent in this test (valid and
> +        * invalid packets are sent in alternating fashion with the final
> +        * packet being invalid). Since the last packet may or may not have
> +        * been dropped already, both outcomes must be allowed.
> +        */
> +       if (stats.rx_dropped == ifobject->pkt_stream->nb_pkts / 2 ||
> +           stats.rx_dropped == ifobject->pkt_stream->nb_pkts / 2 - 1)
>                 return TEST_PASS;
>
>         return TEST_FAILURE;
> --
> 2.39.2
>