Re: [PATCH 1/2] tools/dma: move dma_map_benchmark from selftests to tools/dma

[Qinxin Xia <xiaqinxin@huawei.com>]

On 2025/8/29 05:22:21, Barry Song <21cnbao@gmail.com> wrote:
> On Thu, Aug 28, 2025 at 12:07 AM Qinxin Xia <xiaqinxin@huawei.com> wrote:
>>
>>
>>
>> On 2025/8/22 09:12:07, Barry Song <21cnbao@gmail.com> wrote:
>>>>
>>>> Does usr/include have header files? Did you run make headers_install
>>>> before make?
>>>> [xiaqinxin@localhost linux]$ make headers_install
>>>>      HOSTCC  scripts/basic/fixdep
>>>>      HOSTCC  scripts/unifdef
>>>>      WRAP    arch/arm64/include/generated/uapi/asm/socket.h
>>>>      SYSHDR  arch/arm64/include/generated/uapi/asm/unistd_64.h
>>>>      HDRINST usr/include/asm-generic/mman.h
>>>>      HDRINST usr/include/asm-generic/stat.h
>>>>      HDRINST usr/include/asm-generic/ucontext.h
>>>>      HDRINST usr/include/asm-generic/int-ll64.h
>>>>      HDRINST usr/include/asm-generic/unistd.h
>>>>      HDRINST usr/include/asm-generic/kvm_para.h
>>>>      HDRINST usr/include/asm-generic/types.h
>>>>      HDRINST usr/include/asm-generic/ipcbuf.h
>>>>      HDRINST usr/include/asm-generic/termbits-common.h
>>>> ...
>>>> [xiaqinxin@localhost linux]$ cd tools/dma/
>>>> [xiaqinxin@localhost dma]$ make
>>>> cc -I../../usr/include -I../../include dma_map_benchmark.c -o
>>>> dma_map_benchmark
>>>
>>> This is really frustrating. Why do other parts not need this, but
>>> dma_map_benchmark does? It is also not acceptable to hardcode the
>>> path to usr/include.
>>>
>>> It is also not good practice to access a kernel header directly from a
>>> userspace tool - such as -I../../include.
>>>
>>> Shouldn't map_benchmark.h be a proper UAPI header that gets installed
>>> into the toolchain like the others?
>>>
>> Hello Barry :
>>
>> This include file is inherited from the original version, and there are
>> similar
>>
>> method in other parts ：
>>
>> pcmcia/Makefile:CFLAGS := -I../../usr/include
>> laptop/dslm/Makefile:CFLAGS := -I../../usr/include
>> accounting/Makefile:CFLAGS := -I../../usr/include
>>
>> During compilation, the system searches for header files from
>> ../../usr/include first.
>>
>> If no header file is found in ../../usr/include, the system attempts to
>> get header files
> 
> The difference is that, for them, the build still passes even without
> running `header_install` (and thus without `../../usr/include`).
> 
> tools/laptop/dslm$ make
> gcc -I../../usr/include    dslm.c   -o dslm
> 
> tools/pcmcia$ make
> gcc -I../../usr/include    crc32hash.c   -o crc32hash
> 
> For tools/accounting, the build fails mainly because the UAPI header
> files in the toolchain may be older than those in the latest kernel. so
> we need make headers_install to update.
> 
> But I don’t really understand why we need it. My guess is that the
> "-I../../include" option overrides the system header files, which then
> causes type conflicts.
> 
> cc -I../../include dma_map_benchmark.c -o dma_map_benchmark
> In file included from dma_map_benchmark.c:13:
> ../../include/linux/types.h:20:33: error: conflicting types for
> ‘fd_set’; have ‘__kernel_fd_set’
>     20 | typedef __kernel_fd_set         fd_set;
>        |                                 ^~~~~~
> In file included from /usr/include/x86_64-linux-gnu/sys/types.h:179,
>                   from /usr/include/stdlib.h:395,
>                   from dma_map_benchmark.c:8:
> /usr/include/x86_64-linux-gnu/sys/select.h:70:5: note: previous
> declaration of ‘fd_set’ with type ‘fd_set’
>     70 |   } fd_set;
>        |     ^~~~~~
> In file included from dma_map_benchmark.c:13:
> ../../include/linux/types.h:21:33: error: conflicting types for
> ‘dev_t’; have ‘__kernel_dev_t’ {aka ‘unsigned int’}
>     21 | typedef __kernel_dev_t          dev_t;
>        |                                 ^~~~~
> 
> You should be referring to the system types.h, but the -I../../include
> option makes you pick up the wrong kernel types.h. However, when you
> do have "../../usr/include", you end up with the correct types.h from UAPI.
> 
> I really dislike all this *mess*. You can fix it by doing the following:
> 
> diff --git a/tools/dma/Makefile b/tools/dma/Makefile
> index 841b54896288..cec09abf47cd 100644
> --- a/tools/dma/Makefile
> +++ b/tools/dma/Makefile
> @@ -1,7 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   bindir ?= /usr/bin
> 
> -CFLAGS += -I../../include -I../../usr/include
> +CFLAGS += -idirafter ../../include
> 
Hello, Barry:

Let me see... 'make headers_install' installs the UAPI header files to 
the usr/include directory in the kernel source tree by default, rather 
than directly to the system's /usr/include directory.

This means that when 'map_benchmark.h' is moved to uapi/include, 
compilation tool chain cannot get the header file from the system path. 
Users need to install the UAPI header file to the system directory or 
set environment variables to reference it from the environment variables.

Could this get a little complex?  Should we keep ' -I ../../usr/include 
' ?>>
>> from the system directory of the compilation environment. So maybe in
>> some compilation
>>
>> environments, compiling these modules might have the same problem...
>>
>> 'struct map_benchmark' is defined in map_benchmark.h which is used by
>> map_benchmark.c
>>
>> Do we need to define them separately in the kernel and uapi header files?>>
> 
> Ideally, yes—then the -I../../include option would no longer be necessary.
> 
> Thanks
> Barry