On Mon, Jul 30, 2018 at 8:14 PM, Jens Axboe <axboe@xxxxxxxxx> wrote:
On 7/30/18 4:52 AM, dev.madaari@xxxxxxxxx wrote:
> diff --git a/README b/README
> index 38022bb..cbca933 100644
> --- a/README
> +++ b/README
> @@ -192,6 +192,35 @@ https://github.com/mintty/mintty/issues/56 and
> https://github.com/mintty/mintty/wiki/Tips#inputoutput- interaction-with-alien- programs
> for details).
>
> +RTEMS
> +~~~~~~~
> +
> +For RTEMS Toolchain generation::
> +
> +Toolchain, required for cross-compiling fio for desired architecture
> +(like arm in this example) can be generated by using RTEMS Source Builder(RSB).
> +
> +Please note that fio's build for RTEMS has been tested with RSB version 5(commit id:
> +25f4db09c85a52fb1640a29f9bdc2d Please break these at 80 chars, like the rest of the file.e8c2768988), and it may not work with older versions.
Noted.
> diff --git a/client.c b/client.c
> index 2a86ea9..356f9c0 100644
> --- a/client.c
> +++ b/client.c
> @@ -86,7 +86,7 @@ static void fio_client_remove_hash(struct fio_client *client)
> flist_del_init(&client->hash_list); So an idea for these, since I think the manual init calls end up being
> }
>
> -static void fio_init fio_client_hash_init(void)
> +void fio_init fio_client_hash_init(void)
> {
> int i;
very fragile. The problem is that the two are very far from each other.
Why not require that all fio_init users also call
fio_register_init_func() or something like that. On non-rtems, it would
do nothing. On rtems, it'd register this function as a constructor and
you could have an OS init func similarly to how we have an arch init.
Destructors could be handled the same.
If we do it that way, you don't have a separate table somewhere for init
functions. Nobody that adds a new init func will realize they need to
update rtems, and they should not have to.
Yes. that sounds to be a good,cleaner approach. So, let me just check if
I understood it correctly:
We can have a fio_init_register_func() right in the beginning of main() which
on RTEMS will initialize all the ioengines (just like arch_init) and a fio_exit_register_func()
at the end of main() which will call all destructors. and the most appropriate place
for the definitions of these function would be, init.c?
> diff --git a/crc/xxhash.h b/crc/xxhash.h
> index 8850d20..9019089 100644
> --- a/crc/xxhash.h
> +++ b/crc/xxhash.h
> @@ -107,9 +107,9 @@ XXH32() :
> // Advanced Hash Functions
> //****************************
>
> -void* XXH32_init (unsigned int seed);
> +void* XXH32_init (uint32_t seed);
> XXH_errorcode XXH32_update (void* state, const void* input, int len);
> -unsigned int XXH32_digest (void* state);
> +uint32_t XXH32_digest (void* state);
>
> /*
You still haven't explained why you are making this change? If it's
required, it should be a separate patch, it really has nothing to do
with your port.
during the declaration of, let's say XXH32_init(here) parameter type is
'unsigned int' while during the definition(here) it is 'uint32_t'. Now, on
most desktop os, they are are usually same, however on some embedded
platforms, for example RTEMS which uses newlib as it's standard c library,
Yes, i can quickly submit a separate patch for this.
This, and the above, should not be in the init.c function. See email on
> diff --git a/init.c b/init.c
> index af4cc6b..d69b644 100644
> --- a/init.c
> +++ b/init.c
> @@ -7,7 +7,6 @@
> #include <ctype.h>
> #include <string.h>
> #include <errno.h>
> -#include <sys/ipc.h>
> #include <sys/types.h>
> #include <dlfcn.h>
> #ifdef CONFIG_VALGRIND_DEV
> @@ -21,6 +20,10 @@
> #include <sys/shm.h>
> #endif
>
> +#ifndef __rtems__
> +#include <sys/ipc.h>
> +#endif /* __rtems__ */
> +
> #include "parse.h"
> #include "smalloc.h"
> #include "filehash.h"
> @@ -31,7 +34,13 @@
> #include "filelock.h"
> #include "steadystate.h"
>
> +/* RTEMS uses newlib's getopt_long_only_r() */
> +#ifdef __rtems__
> +#include <getopt.h>
> +#else /* __rtems__ */
> #include "oslib/getopt.h"
> +#endif /* __rtems__ */
> +
> #include "oslib/strcasestr.h"
>
> #include "crc/test.h"
your first patch for clarification.
Ditto for all of this. If you need to add ifdef rtems, then you are
> @@ -87,6 +96,10 @@ unsigned int *fio_warned = NULL;
> static char cmd_optstr[256];
> static bool did_arg;
>
> +#ifdef __rtems__
> + struct getopt_data getopt_reent;
> +#endif /* __rtems__ */
> +
> #define FIO_CLIENT_FLAG (1 << 16)
>
> /*
> @@ -2435,13 +2448,20 @@ int parse_cmd_line(int argc, char *argv[], int client_type)
> void *cur_client = NULL;
> int backend = 0;
>
> +#ifdef __rtems__
> + memset(&getopt_reent, 0, sizeof(getopt_data));
> +#endif /* __rtems__ */
> /*
> * Reset optind handling, since we may call this multiple times
> * for the backend.
> */
> optind = 1;
> -
> - while ((c = getopt_long_only(argc, argv, ostr, l_opts, &lidx)) != -1) {
> + while ((c =
> +#ifdef __rtems__ /* Using Newlib's reentrant version of getopt */
> + getopt_long_only_r(argc, argv, ostr, l_opts, &lidx, &getopt_reent))!= -1) {
> +#else /* __rtems__ */
> + getopt_long_only(argc, argv, ostr, l_opts, &lidx))!= -1) {
> +#endif /* __rtems__ */
> if ((c & FIO_CLIENT_FLAG) || client_flag_set(c)) {
> parse_cmd_client(cur_client, argv[optind - 1]);
> c &= ~FIO_CLIENT_FLAG;
doing it wrong. Fio supports lots of OS and archs, think about why the
code isn't already littered with ifdef this or that. If we had done it
that way, it would be a fragile unmaintaineable mess by now.
True, i'll further try to rectify this. Main issue i faced here was that newlib supports
both getopt_long_only and getopt_long_only_r and we need to use the later one(since
it doesn't use global variables to pass info). I'll see if with some way around i can remove
that ifndef too.
> diff --git a/os/os.h b/os/os.h
> index becc410..22bbb9b 100644
> --- a/os/os.h
> +++ b/os/os.h
> @@ -23,6 +23,7 @@ enum {
> os_windows,
> os_android,
> os_dragonfly,
> + os_rtems,
>
> os_nr,
> };
You need to update libfio.c:fio_os_strings[] as well.
Ok.
This seems totally random, just doing select init functions. This should> diff --git a/os/rtems/rtems-fio-wrap.h b/os/rtems/rtems-fio-wrap.h
> new file mode 100644
> index 0000000..54397c9
> --- /dev/null
> +++ b/os/rtems/rtems-fio-wrap.h
> @@ -0,0 +1,41 @@
> +#ifndef RTEMS_FIO_WRAP
> +#define RTEMS_FIO_WRAP
> +/* RTEMS specific wrapper for explicitly calling constructors and
> + * destructors */
> +
> +void fio_syncio_register(void);
> +void fio_filecreate_register(void);
> +void fio_null_register(void);
> +void fio_syncio_unregister(void);
> +void fio_filecreate_unregister(void);
> +void fio_null_unregister(void);
> +
> +static int
> +mainwrapper(int argc, char *argv[])
> +{
> + int err=0;
> +
> + /* Constructors */
> + fio_syncio_register();
> + fio_filecreate_register();
> + fio_null_register();
> +
> + err = main(argc, argv, NULL);
> +
> + /* Destructors */
> + fio_syncio_unregister();
> + fio_filecreate_unregister();
> + fio_null_unregister();
> +
> + return err;
> +}
also not be a main wrapper, please look into doing the init/destructor
type setup I mentioned earlier.
> diff --git a/os/rtems/rtems-init.c b/os/rtems/rtems-init.c
> new file mode 100644
> index 0000000..2cce2bb
> --- /dev/null
> +++ b/os/rtems/rtems-init.c
> +void
> +Init(rtems_task_argument arg)
> +{
> + rtems_status_code sc;
> +
> + puts("\n*** FIO - Flexible I/O tester ***\n\n");
> +
> + early_initialization();
> + rtems_bsd_initialize();
> +
> + /* Let the callout timer allocate its resources */
> + sc = rtems_task_wake_after(2);
> + assert(sc == RTEMS_SUCCESSFUL);
> +
> + sc = rtems_shell_init("SHLL", 16 * 1024, 1, CONSOLE_DEVICE_NAME,
> + false, true, NULL);
> + assert(sc == RTEMS_SUCCESSFUL);
> +
> + assert(0);
> +}
assert(0)? Also, don't add your own header with puts. A port should be a
straight port, nothing should change in output or otherwise.
Actually, the rtems-init.c file is the configuration file for the RTOS and this header will
appear during the boot-up.
Here's how fio is interfaced with RTEMS:
Here's how fio is interfaced with RTEMS:
- After cross-compiling the fio sources with RTEMS toolchain, user will have to directly
load the generated fio binary using an appropriate bootloader(like u-boot) on the target(like on beaglebone black).
- Then, RTEMS will bootup and the application specific header will appear(the one i have put above).
- RTEMS shell is then invoked (rtems_shell_init()), and user can enter the job config file and then invoke fio.
- In any case, rtems_shell_init() should not exit, and if it does we have assert(0) which will immediately stop the kernel.
> diff --git a/smalloc.c b/smalloc.c
> index a2ad25a..5cad99c 100644
> --- a/smalloc.c
> +++ b/smalloc.c
> @@ -19,7 +19,11 @@
> #define SMALLOC_BPL (SMALLOC_BPB * SMALLOC_BPI)
>
> #define INITIAL_SIZE 16*1024*1024 /* new pool size */
> +#ifndef FIO_LESS_POOLS
> #define INITIAL_POOLS 8 /* maximum number of pools to setup */
> +#else
> +#define INITIAL_POOLS 4
> +#endif
Why?
Maybe because, sometimes, on low memory targets we just don't have enough space to
allocate 8 pools of 16MB each. I understand the drawback of this setting(upper limit on the
number of jobs, like in my case it's 5 ) but probably i can live with that(until it doesn't start
influencing the statistics i want).
influencing the statistics i want).
> @@ -175,7 +179,12 @@ static bool add_pool(struct pool *pool, unsigned int alloc_size)
> #else
> mmap_flags |= MAP_SHARED;
> #endif
> +
> +#ifdef __rtems__
> + ptr = (void*)malloc(alloc_size);
> +#else
> ptr = mmap(NULL, alloc_size, PROT_READ|PROT_WRITE, mmap_flags, -1, 0);
> +#endif
This needs a comment.
I don't have a really good reason for doing this, but it's just that RTEMS mmap() implementation
is a bit buggy. sometimes, when asked to allocate a larger memory size it just silently fails. So,
instead, i though of using malloc() for better stability. Moreover, it doesn't support shared mapping too.
> diff --git a/stat.c b/stat.c
> index a308eb8..d909406 100644
> --- a/stat.c
> +++ b/stat.c
> @@ -38,10 +38,16 @@ void update_rusage_stat(struct thread_data *td)
> &td->ru_end.ru_utime);
> ts->sys_time += mtime_since_tv(&td->ru_start.ru_stime, Probably better to just set these to 0.
> &td->ru_end.ru_stime);
> +#ifndef FIO_NO_VIRTUAL_MEMORY
> ts->ctx += td->ru_end.ru_nvcsw + td->ru_end.ru_nivcsw
> - (td->ru_start.ru_nvcsw + td->ru_start.ru_nivcsw);
> ts->minf += td->ru_end.ru_minflt - td->ru_start.ru_minflt;
> ts->majf += td->ru_end.ru_majflt - td->ru_start.ru_majflt;
> +#else /* FIO_NO_VIRTUAL_MEMORY */
> + ts->ctx = ((uint64_t) -1);
> + ts->minf = ((uint64_t) -1);
> + ts->majf = ((uint64_t) -1);
> +#endif /* FIO_NO_VIRTUAL_MEMORY */
Ok
--
Jens Axboe
--
