On Thu, May 8, 2025 at 11:21 AM T.J. Mercier <tjmerc...@google.com> wrote:
>
> Use the same test buffers as the traditional iterator and a new BPF map
> to verify the test buffers can be found with the open coded dmabuf
> iterator.
The way we split 4/5 and 5/5 makes the code tricker to follow. I guess
the motivation is to back port default iter along to older kernels. But I
think we can still make the code cleaner.
>
> Signed-off-by: T.J. Mercier <tjmerc...@google.com>
> ---
[...]
>
> -static int create_udmabuf(void)
> +static int create_udmabuf(int map_fd)
> {
> struct udmabuf_create create;
> int dev_udmabuf;
> + bool f = false;
>
> udmabuf_test_buffer_size = 10 * getpagesize();
>
> @@ -63,10 +64,10 @@ static int create_udmabuf(void)
> if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B,
> udmabuf_test_buffer_name), "name"))
> return 1;
>
> - return 0;
> + return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name, &f,
> BPF_ANY);
We don't really need this bpf_map_update_elem() inside
create_udmabuf(), right?
> }
>
> -static int create_sys_heap_dmabuf(void)
> +static int create_sys_heap_dmabuf(int map_fd)
> {
> sysheap_test_buffer_size = 20 * getpagesize();
>
> @@ -77,6 +78,7 @@ static int create_sys_heap_dmabuf(void)
> .heap_flags = 0,
> };
> int heap_fd, ret;
> + bool f = false;
>
> if (!ASSERT_LE(sizeof(sysheap_test_buffer_name), DMA_BUF_NAME_LEN,
> "NAMETOOLONG"))
> return 1;
> @@ -95,18 +97,18 @@ static int create_sys_heap_dmabuf(void)
> if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B,
> sysheap_test_buffer_name), "name"))
> return 1;
>
> - return 0;
> + return bpf_map_update_elem(map_fd, sysheap_test_buffer_name, &f,
> BPF_ANY);
Same for this bpf_map_update_elem(), we can call this directly from
create_test_buffers().
> }
>
> -static int create_test_buffers(void)
> +static int create_test_buffers(int map_fd)
> {
> int ret;
>
> - ret = create_udmabuf();
> + ret = create_udmabuf(map_fd);
> if (ret)
> return ret;
>
> - return create_sys_heap_dmabuf();
> + return create_sys_heap_dmabuf(map_fd);
Personally, I would prefer we just merge all the logic of
create_udmabuf() and create_sys_heap_dmabuf()
into create_test_buffers().
> }
>
[...]
> +
> void test_dmabuf_iter(void)
> {
> struct dmabuf_iter *skel = NULL;
> + int iter_fd, map_fd;
> char buf[256];
> - int iter_fd;
>
> skel = dmabuf_iter__open_and_load();
> if (!ASSERT_OK_PTR(skel, "dmabuf_iter__open_and_load"))
> return;
>
> - if (!ASSERT_OK(create_test_buffers(), "create_buffers"))
> + map_fd = bpf_map__fd(skel->maps.testbuf_hash);
> + if (!ASSERT_OK_FD(map_fd, "map_fd"))
> + goto destroy_skel;
> +
> + if (!ASSERT_OK(create_test_buffers(map_fd), "create_buffers"))
> goto destroy;
>
> if (!ASSERT_OK(dmabuf_iter__attach(skel), "skel_attach"))
> @@ -215,10 +246,13 @@ void test_dmabuf_iter(void)
>
> if (test__start_subtest("default_iter"))
> subtest_dmabuf_iter_check_default_iter(skel);
> + if (test__start_subtest("open_coded"))
> + subtest_dmabuf_iter_check_open_coded(skel, map_fd);
>
> close(iter_fd);
>
> destroy:
> destroy_test_buffers();
> +destroy_skel:
> dmabuf_iter__destroy(skel);
> }
[...]
