On Fri, May 9, 2025 at 2:58 PM Song Liu song@kernel.org wrote:

On Fri, May 9, 2025 at 2:43 PM T.J. Mercier tjmercier@google.com wrote:

...

[...]

...

...

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:

I do like this version better. Some nitpicks though.

...

-static int memfd, udmabuf; +static int udmabuf;

About this, and ...

...

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;
  

.. here.

It is not ideal to have a global udmabuf and a local udmabuf. If we want the global version, let's rename the local one.

Ok let me change up the name of the aliasing variable to local_udmabuf.

[...]

...

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;
  

We also need destroy_test_buffers() on the error path here, or at the caller.

The caller does currently check to decide if it should bother running the tests or not, and calls destroy_test_buffers() if not.

...

•   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);
  

For the two global fds, let's reset them to -1 right after close().

Thanks, Song

Will do, thanks.