search

intel / idxd-config

Accel-config / libaccel-config
Other
56 stars 34 forks source link

Compilation fails on i586 #3

Closed lpechacek closed 3 years ago

lpechacek commented 3 years ago

On a 32-bit system, the compilation fails with the below error. The root cause is the difference in the int size between 32-bit and 64-bit systems.

[   11s] gcc -DHAVE_CONFIG_H -I. -I..  -include ../config.h -DSYSCONFDIR=\""/etc"\" -DLIBEXECDIR=\""/usr/libexec"\" -DPREFIX=\""/usr"\" -DACCFG_MAN_PATH=\""/usr/share/man"\" -I../accfg/lib -I../accfg -I../ -I/usr/include/kmod   -I/usr/include/uuid  -I/usr/include/json-c   -Wall -Wchar-subscripts -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wshadow -Wsign-compare -Wstrict-prototypes -Wtype-limits -Wmaybe-uninitialized -Wdeclaration-after-statement -Wunused-result -D_FORTIFY_SOURCE=2 -O2 -fvisibility=hidden -ffunction-sections -fdata-sections -fomit-frame-pointer -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -flto=auto -g -c -o config_attr.o config_attr.c
[   11s] In file included from config_attr.c:14:
[   11s] config_attr.c: In function 'cmd_config_wq':
[   11s] ../util/util.h:73:45: error: negative width in bit-field '<anonymous>'
[   11s]    73 | #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
[   11s]       |                                             ^
[   11s] ../util/parse-options.h:120:32: note: in expansion of macro 'BUILD_BUG_ON_ZERO'
[   11s]   120 | #define check_vtype(v, type) ( BUILD_BUG_ON_ZERO(!__builtin_types_compatible_p(typeof(v), type)) + v )
[   11s]       |                                ^~~~~~~~~~~~~~~~~
[   11s] ../util/parse-options.h:137:105: note: in expansion of macro 'check_vtype'
[   11s]   137 | #define OPT_U64(s, l, v, h)         { .type = OPTION_U64, .short_name = (s), .long_name = (l), .value = check_vtype(v, u64 *), .help = (h) }
[   11s]       |                                                                                                         ^~~~~~~~~~~
[   11s] config_attr.c:438:3: note: in expansion of macro 'OPT_U64'
[   11s]   438 |   OPT_U64('x', "max-transfer-size", &wq_param.max_transfer_size,
[   11s]       |   ^~~~~~~
[   11s] make[3]: *** [Makefile:528: config_attr.o] Error 1

Although DSA and IAX are embedded in 64-bit CPUs, I think that it's fair to assume that the operating system might run in 32-bit mode, and therefore the configuration tools should also compile for 32-bit targets. The proposed patch addresses only the immediate build failure but there are more places that require fixing with regard to the assumed CPU word length.

--- idxd-config-accel-config-v3.0.1.orig/accfg/libaccel_config.h
+++ idxd-config-accel-config-v3.0.1/accfg/libaccel_config.h
@@ -102,7 +102,7 @@ struct wq_parameters {
    unsigned int priority;
    int block_on_fault;
    unsigned int max_batch_size;
-   unsigned long max_transfer_size;
+   uint64_t max_transfer_size;
    const char *mode;
    const char *type;
    const char *name;
ramesh-thomas commented 3 years ago

Thanks for the patch. We ignored 32 bit compile errors because my understanding is driver doesn't support 32 bit target. @davejiang can correct me.

The max_transfer_size field is 64 bits. To be fully correct for 32 bit targets, we need to also make sure there is no loss of data due to use of shorter data types at all other places where it gets processed. You are welcome to provide additional patches addressing them if any.

djbw commented 3 years ago

It doesn't matter if the driver is 32-bit only. Userspace packages that a distro ships might be built for any number of architectures, and it is a pain to maintain some packages as architecture specific. Every effort should be made to not make Linux distro packager lives harder. Notice all the architectures that ndctl (which idxd-config was derived) builds on:

https://kojipkgs.fedoraproject.org/packages/ndctl/71/1.fc34/

...and note that many of those architectures did not have a persistent memory driver when the ndctl project was started.

ramesh-thomas commented 3 years ago

Besides this issue, I also notice other warnings related to 32 bit compiler incompatibilities that can break the build. The project uses "unsigned long" everywhere which is problematic especially if the field is actually 64 bits. I will include the current patch along with other patches I am working on to address those issues to the accel-config mailing list https://lists.01.org/postorius/lists/accel-config.lists.01.org/

lpechacek commented 3 years ago

We ignored 32 bit compile errors because my understanding is driver doesn't support 32 bit target.

Understood. However, 64-bit kernel can host 32-bit userspace. I don't say that I would see it in the field but the possibility is there.

From my side is the bug report an open question about your target platform support and the patch is a mere illustration that use of portable data types may be the way to go. The final decision is yours and I'm happy to see that you decided in favor of the broader target set. Thanks!

ramesh-thomas commented 3 years ago

Posted patch https://lists.01.org/hyperkitty/list/accel-config@lists.01.org/thread/U7Q2I6WYKCHVPCXOJADE34R65ABV3TXB/

ramesh-thomas commented 3 years ago

Committed fixes https://github.com/intel/idxd-config/commit/2d3236066d9b6018491b1316a7fe015eb394834c