Re: [Devel] [PATCH vz10 v2 3/5] selftests: bpf: size the map in test_lru_sanity3 to whole refills

[Konstantin Khorenko <khorenko@virtuozzo.com>]

> The test fills a map of tgt_free * 2 elements and then reads back all
> but the last few, which fails on a machine with six cpus:
> 
>   test_lru_sanity3 (map_type:9 map_flags:0x0): test_lru_map.c:463:
>   test_lru_sanity3: Assertion `!bpf_map_lookup_elem_with_ref_bit(
>   lru_map_fd, key, value)' failed.
> 
> The elements are handed out in refills of lru->target_free, which the
> kernel derives from the map size as clamp((size / nr_cpus) / 2, 1,
> LOCAL_FREE_TARGET), 21 for a 256 element map and six cpus. A refill
> the global free list cannot satisfy in full does not stop there, it
> calls __bpf_lru_list_shrink() for the remainder, and that evicts
> elements which are still live. 256 is not a multiple of 21, so filling
> the map ends on a partial refill that drops 17 of the elements the test
> goes on to reference, and the lookup fails on the first of them.
> 
> Whether the size divides evenly depends on the cpu count alone, which
> is why this passes on two and on sixty four cpus and fails on six.
> 
> batch_size is already __tgt_size(tgt_free), the refill size of a map
> of __map_size(batch_size) elements, so size the map that way and the
> fill consumes whole refills and evicts nothing. Start the keys of the
> last insert at map_size + 1, they were placed just past the old size.
> 
> Fixes: 5e9388f7984a ("selftests/bpf: adapt one more case in test_lru_map to the new target_free")
> https://virtuozzo.atlassian.net/browse/VSTOR-139677
> Feature: fix selftests
> Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
>
> diff --git a/tools/testing/selftests/bpf/test_lru_map.c b/tools/testing/selftests/bpf/test_lru_map.c
> index 0921939532c6c..fc67a337d3c1b 100644
> --- a/tools/testing/selftests/bpf/test_lru_map.c
> +++ b/tools/testing/selftests/bpf/test_lru_map.c
> @@ -441,8 +441,18 @@ static void test_lru_sanity3(int map_type, int map_flags, unsigned int tgt_free)
>  	assert(sched_next_online(0, &next_cpu) != -1);
>  
>  	batch_size = __tgt_size(tgt_free);
> +	if (!batch_size)
> +		batch_size = 1;

Regression: the line

      if (!batch_size)
              batch_size = 1;

breaks the test for nr_cpus > 128. The old test degenerated there:
__tgt_size(128) is 0, nothing is inserted after the fill, map_equal()
passes trivially. The new test builds a map of 2 * nr_cpus elements with
target_free = 1. On the last insert the inactive list holds more than
nr_scans (128) referenced elements ahead of the single unreferenced one,
__bpf_lru_list_shrink_inactive() never reaches it, __bpf_lru_list_shrink()
falls through to the forced shrink and evicts a live key. The model fails
on 129, 144, 160, 192, 256, 384 and 512 cpus (256 cpus: key 257 is
gone, key 512 stays). Two-socket nodes and VMs with a large maxcpus have
that many possible cpus.

Suggested fix: when __tgt_size(tgt_free) is 0, print Skip and return
instead of forcing batch_size to 1. That keeps the old behaviour on large
machines and still fixes 3-64 cpus.

> +
> +	/* The local free list is refilled lru->target_free elements at a
> +	 * time, and a refill the global free list cannot satisfy in full
> +	 * shrinks the LRU list, which evicts elements that are still live.
> +	 * Size the map so that target_free divides it, otherwise filling it
> +	 * ends on a partial refill and evicts the elements referenced below.
> +	 */
> +	map_size = __map_size(batch_size);
> +	assert(__tgt_size(map_size) == batch_size);

assert(__tgt_size(map_size) == batch_size) is a tautology,
(b * n * 2 / n) / 2 == b holds for every b and n, so it checks nothing.

-- 
Konstantin Khorenko <khorenko@virtuozzo.com>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel