On Mon, Aug 16, 2021 at 4:45 PM sunyucong@xxxxxxxxx <sunyucong@xxxxxxxxx> wrote:
>
> On Mon, Aug 16, 2021 at 4:28 PM Song Liu <song@xxxxxxxxxx> wrote:
> >
> > On Mon, Aug 16, 2021 at 10:54 AM Yucong Sun <fallentree@xxxxxx> wrote:
> > >
> > > Using a fixed delay of 1ms is proven flaky in slow CPU environment, eg. github
> > > action CI system. This patch adds exponential backoff with a cap of 50ms, to
> > > reduce the flakiness of the test.
> >
> > Do we have data showing how flaky the test is before and after this change?
>
> Before the change, on 2 CPU KVM on my laptop the test is perfectly
> fine, on Github action (2 emulated CPU) , it appeared to fail roughly
> 1 in 10 runs or even more frequently.
> After the change, it appears pretty robust both on my laptop and on
> github action, I ran the github action a couple times and it succeeded
> every time.
Thanks for the data!
We should include this in the commit log. Maybe the maintainer could just
amend it when applying the patch.
>
> >
> > >
> > > Signed-off-by: Yucong Sun <fallentree@xxxxxx>
> > > ---
> > > tools/testing/selftests/bpf/test_maps.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
> > > index 14cea869235b..ed92d56c19cf 100644
> > > --- a/tools/testing/selftests/bpf/test_maps.c
> > > +++ b/tools/testing/selftests/bpf/test_maps.c
> > > @@ -1400,11 +1400,16 @@ static void test_map_stress(void)
> > > static int map_update_retriable(int map_fd, const void *key, const void *value,
> > > int flags, int attempts)
> > > {
> > > + int delay = 1;
> > > +
> > > while (bpf_map_update_elem(map_fd, key, value, flags)) {
> > > if (!attempts || (errno != EAGAIN && errno != EBUSY))
> > > return -errno;
> > >
> > > - usleep(1);
> > > + if (delay < 50)
> > > + delay *= 2;
> > > +
> > > + usleep(delay);
> >
> > It is a little weird that the delay times in microseconds are 2, 4, 8,
> > 16, 32, 64, 64, ...
> > Maybe just use rand()?
>
> map_update_retriable is called by test_map_update() , which is being
> parallel executed in 1024 threads, so the lock contention is
> intentional, I think if we introduce randomness in the delay it kind
> of defeats the purpose of the test.
> My original proposal is to just increase the attempts to 10X , Andrii
> proposed to use an exponential back-off, which is what I ended up
> implementing.
If that is what we agreed on, it works.
Acked-by: Song Liu <songliubraving@xxxxxx>
