Re: [PATCH bpf-next v4 5/5] selftests/bpf: Add test for open coded dmabuf_iter - Linaro-mm-sig

9 May 2025


      On Fri, May 9, 2025 at 11:46 AM Song Liu song@kernel.org wrote:
...
On Thu, May 8, 2025 at 11:21 AM T.J. Mercier tjmercier@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 tjmercier@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().
That's a lot of different stuff to put in one place. How about
returning file descriptors from the buffer create functions while
having them clean up after themselves:
-static int memfd, udmabuf;
+static int udmabuf;
 static const char udmabuf_test_buffer_name[DMA_BUF_NAME_LEN] =
"udmabuf_test_buffer_for_iter";
 static size_t udmabuf_test_buffer_size;
 static int sysheap_dmabuf;
 static const char sysheap_test_buffer_name[DMA_BUF_NAME_LEN] =
"sysheap_test_buffer_for_iter";
 static size_t sysheap_test_buffer_size;
-static int create_udmabuf(int map_fd)
+static int create_udmabuf(void)
 {
        struct udmabuf_create create;
-       int dev_udmabuf;
-       bool f = false;
+       int dev_udmabuf, memfd, udmabuf;
udmabuf_test_buffer_size = 10 * getpagesize();
if (!ASSERT_LE(sizeof(udmabuf_test_buffer_name),
DMA_BUF_NAME_LEN, "NAMETOOLONG"))
-               return 1;
+               return -1;
memfd = memfd_create("memfd_test", MFD_ALLOW_SEALING);
        if (!ASSERT_OK_FD(memfd, "memfd_create"))
-               return 1;
+               return -1;
if (!ASSERT_OK(ftruncate(memfd, udmabuf_test_buffer_size), "ftruncate"))
-               return 1;
+               goto close_memfd;
if (!ASSERT_OK(fcntl(memfd, F_ADD_SEALS, F_SEAL_SHRINK), "seal"))
-               return 1;
+               goto close_memfd;
dev_udmabuf = open("/dev/udmabuf", O_RDONLY);
        if (!ASSERT_OK_FD(dev_udmabuf, "open udmabuf"))
-               return 1;
+               goto close_memfd;
create.memfd = memfd;
        create.flags = UDMABUF_FLAGS_CLOEXEC;
@@ -59,15 +58,21 @@ static int create_udmabuf(int map_fd)
        udmabuf = ioctl(dev_udmabuf, UDMABUF_CREATE, &create);
        close(dev_udmabuf);
        if (!ASSERT_OK_FD(udmabuf, "udmabuf_create"))
-               return 1;
+               goto close_memfd;
if (!ASSERT_OK(ioctl(udmabuf, DMA_BUF_SET_NAME_B,
udmabuf_test_buffer_name), "name"))
-               return 1;
+               goto close_udmabuf;
+
+       return udmabuf;
-       return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name,
&f, BPF_ANY);
+close_udmabuf:
+       close(udmabuf);
+close_memfd:
+       close(memfd);
+       return -1;
 }
-static int create_sys_heap_dmabuf(int map_fd)
+static int create_sys_heap_dmabuf(void)
 {
        sysheap_test_buffer_size = 20 * getpagesize();
@@ -78,43 +83,46 @@ static int create_sys_heap_dmabuf(int map_fd)
                .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;
+               return -1;
heap_fd = open("/dev/dma_heap/system", O_RDONLY);
        if (!ASSERT_OK_FD(heap_fd, "open dma heap"))
-               return 1;
+               return -1;
ret = ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &data);
        close(heap_fd);
        if (!ASSERT_OK(ret, "syheap alloc"))
-               return 1;
+               return -1;
-       sysheap_dmabuf = data.fd;
+       if (!ASSERT_OK(ioctl(data.fd, DMA_BUF_SET_NAME_B,
sysheap_test_buffer_name), "name"))
+               goto close_sysheap_dmabuf;
-       if (!ASSERT_OK(ioctl(sysheap_dmabuf, DMA_BUF_SET_NAME_B,
sysheap_test_buffer_name), "name"))
-               return 1;
+       return data.fd;
-       return bpf_map_update_elem(map_fd, sysheap_test_buffer_name,
&f, BPF_ANY);
+close_sysheap_dmabuf:
+       close(data.fd);
+       return -1;
 }
static int create_test_buffers(int map_fd)
 {
-       int ret;
+       bool f = false;
+
+       udmabuf = create_udmabuf();
+       sysheap_dmabuf = create_sys_heap_dmabuf();
-       ret = create_udmabuf(map_fd);
-       if (ret)
-               return ret;
+       if (udmabuf < 0 || sysheap_dmabuf < 0)
+               return -1;
-       return create_sys_heap_dmabuf(map_fd);
+       return bpf_map_update_elem(map_fd, udmabuf_test_buffer_name,
&f, BPF_ANY) ||
+              bpf_map_update_elem(map_fd, sysheap_test_buffer_name,
&f, BPF_ANY);
 }
static void destroy_test_buffers(void)
 {
        close(udmabuf);
-       close(memfd);
        close(sysheap_dmabuf);
 }

    