On Fri, May 9, 2025 at 11:46 AM Song Liu <s...@kernel.org> wrote:
>
> 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().
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);
}
