On Thu, 23 Mar 2023 08:28:01 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> + if (fseek(fp, 0, SEEK_END) == 0) {
> + file_size = ftell(fp);
> + if (file_size == -1) {
> + ret = -EIO;
> + goto error;
> + }
> +
> + file_buffer = rte_malloc(NULL, file_size, RTE_CACHE_LINE_SIZE);
> + if (file_buffer == NULL) {
> + ml_err("Failed to allocate memory: %s\n", file);
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + if (fseek(fp, 0, SEEK_SET) != 0) {
> + ret = -EIO;
> + goto error;
> + }
> +
> + if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
> + ml_err("Failed to read file : %s\n", file);
> + ret = -EIO;
> + goto error;
> + }
> + fclose(fp);
> + } else {
> + ret = -EIO;
> + goto error;
> + }
> +
> + *buffer = file_buffer;
> + *size = file_size;
> +
> + return 0;
Granted this only test code, but is the slowest way to do this.
Stdio is buffered (in 4K chunks). And using rte_malloc comes from hugepages.
Three levels of improvement are possible:
1. don't use rte_malloc() use malloc() instead.
2. use direct system call for I/O
3. use mmap() to directly map in the file instead read
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 28 March 2023 21:22
> To: Srikanth Yalavarthi <syalavarthi@marvell.com>
> Cc: Anup Prabhu <aprabhu@marvell.com>; dev@dpdk.org; Shivah Shankar
> Shankar Narayan Rao <sshankarnara@marvell.com>; Prince Takkar
> <ptakkar@marvell.com>; Srikanth Yalavarthi <syalavarthi@marvell.com>
> Subject: [EXT] Re: [PATCH 1/1] app/mldev: add internal function for file read
>
> External Email
>
> ----------------------------------------------------------------------
> On Thu, 23 Mar 2023 08:28:01 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
>
> > + if (fseek(fp, 0, SEEK_END) == 0) {
> > + file_size = ftell(fp);
> > + if (file_size == -1) {
> > + ret = -EIO;
> > + goto error;
> > + }
> > +
> > + file_buffer = rte_malloc(NULL, file_size,
> RTE_CACHE_LINE_SIZE);
> > + if (file_buffer == NULL) {
> > + ml_err("Failed to allocate memory: %s\n", file);
> > + ret = -ENOMEM;
> > + goto error;
> > + }
> > +
> > + if (fseek(fp, 0, SEEK_SET) != 0) {
> > + ret = -EIO;
> > + goto error;
> > + }
> > +
> > + if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned
> long)file_size) {
> > + ml_err("Failed to read file : %s\n", file);
> > + ret = -EIO;
> > + goto error;
> > + }
> > + fclose(fp);
> > + } else {
> > + ret = -EIO;
> > + goto error;
> > + }
> > +
> > + *buffer = file_buffer;
> > + *size = file_size;
> > +
> > + return 0;
>
> Granted this only test code, but is the slowest way to do this.
> Stdio is buffered (in 4K chunks). And using rte_malloc comes from
> hugepages.
>
> Three levels of improvement are possible:
> 1. don't use rte_malloc() use malloc() instead.
Agree on this. Will update in next version.
> 2. use direct system call for I/O
> 3. use mmap() to directly map in the file instead read
Agree on the improvements.
But, considering that this is a test code and these operations are done in slow-path, I would prefer to have the implementation based on C library calls rather than using system calls.
Also, using system calls may not make this code portable? Though we are not supporting this app on platforms other than Linux, as of now.
Pls let me know what you think.
@@ -5,12 +5,71 @@
#include <errno.h>
#include <rte_common.h>
+#include <rte_malloc.h>
#include <rte_memory.h>
#include <rte_mldev.h>
#include "ml_common.h"
#include "test_common.h"
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+ char *file_buffer = NULL;
+ long file_size = 0;
+ int ret = 0;
+ FILE *fp;
+
+ fp = fopen(file, "r");
+ if (fp == NULL) {
+ ml_err("Failed to open file: %s\n", file);
+ return -EIO;
+ }
+
+ if (fseek(fp, 0, SEEK_END) == 0) {
+ file_size = ftell(fp);
+ if (file_size == -1) {
+ ret = -EIO;
+ goto error;
+ }
+
+ file_buffer = rte_malloc(NULL, file_size, RTE_CACHE_LINE_SIZE);
+ if (file_buffer == NULL) {
+ ml_err("Failed to allocate memory: %s\n", file);
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ if (fseek(fp, 0, SEEK_SET) != 0) {
+ ret = -EIO;
+ goto error;
+ }
+
+ if (fread(file_buffer, sizeof(char), file_size, fp) != (unsigned long)file_size) {
+ ml_err("Failed to read file : %s\n", file);
+ ret = -EIO;
+ goto error;
+ }
+ fclose(fp);
+ } else {
+ ret = -EIO;
+ goto error;
+ }
+
+ *buffer = file_buffer;
+ *size = file_size;
+
+ return 0;
+
+error:
+ rte_free(file_buffer);
+
+ if (fp != NULL)
+ fclose(fp);
+
+ return ret;
+}
+
bool
ml_test_cap_check(struct ml_options *opt)
{
@@ -24,4 +24,6 @@ int ml_test_device_close(struct ml_test *test, struct ml_options *opt);
int ml_test_device_start(struct ml_test *test, struct ml_options *opt);
int ml_test_device_stop(struct ml_test *test, struct ml_options *opt);
+int ml_read_file(char *file, size_t *size, char **buffer);
+
#endif /* TEST_COMMON_H */
@@ -604,10 +604,10 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
char mp_name[RTE_MEMPOOL_NAMESIZE];
const struct rte_memzone *mz;
uint64_t nb_buffers;
+ char *buffer = NULL;
uint32_t buff_size;
uint32_t mz_size;
- uint32_t fsize;
- FILE *fp;
+ size_t fsize;
int ret;
/* get input buffer size */
@@ -647,51 +647,35 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
t->model[fid].reference = NULL;
/* load input file */
- fp = fopen(opt->filelist[fid].input, "r");
- if (fp == NULL) {
- ml_err("Failed to open input file : %s\n", opt->filelist[fid].input);
- ret = -errno;
+ ret = ml_read_file(opt->filelist[fid].input, &fsize, &buffer);
+ if (ret != 0)
goto error;
- }
- fseek(fp, 0, SEEK_END);
- fsize = ftell(fp);
- fseek(fp, 0, SEEK_SET);
- if (fsize != t->model[fid].inp_dsize) {
- ml_err("Invalid input file, size = %u (expected size = %" PRIu64 ")\n", fsize,
+ if (fsize == t->model[fid].inp_dsize) {
+ rte_memcpy(t->model[fid].input, buffer, fsize);
+ rte_free(buffer);
+ } else {
+ ml_err("Invalid input file, size = %zu (expected size = %" PRIu64 ")\n", fsize,
t->model[fid].inp_dsize);
ret = -EINVAL;
- fclose(fp);
- goto error;
- }
-
- if (fread(t->model[fid].input, 1, t->model[fid].inp_dsize, fp) != t->model[fid].inp_dsize) {
- ml_err("Failed to read input file : %s\n", opt->filelist[fid].input);
- ret = -errno;
- fclose(fp);
goto error;
}
- fclose(fp);
/* load reference file */
if (t->model[fid].reference != NULL) {
- fp = fopen(opt->filelist[fid].reference, "r");
- if (fp == NULL) {
- ml_err("Failed to open reference file : %s\n",
- opt->filelist[fid].reference);
- ret = -errno;
+ ret = ml_read_file(opt->filelist[fid].reference, &fsize, &buffer);
+ if (ret != 0)
goto error;
- }
- if (fread(t->model[fid].reference, 1, t->model[fid].out_dsize, fp) !=
- t->model[fid].out_dsize) {
- ml_err("Failed to read reference file : %s\n",
- opt->filelist[fid].reference);
- ret = -errno;
- fclose(fp);
+ if (fsize == t->model[fid].out_dsize) {
+ rte_memcpy(t->model[fid].reference, buffer, fsize);
+ rte_free(buffer);
+ } else {
+ ml_err("Invalid reference file, size = %zu (expected size = %" PRIu64 ")\n",
+ fsize, t->model[fid].out_dsize);
+ ret = -EINVAL;
goto error;
}
- fclose(fp);
}
/* create mempool for quantized input and output buffers. ml_request_initialize is
@@ -723,6 +707,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
t->model[fid].io_pool = NULL;
}
+ rte_free(buffer);
+
return ret;
}
@@ -14,11 +14,11 @@
int
ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *model, uint16_t fid)
{
- struct test_common *t = ml_test_priv(test);
struct rte_ml_model_params model_params;
- FILE *fp;
int ret;
+ RTE_SET_USED(test);
+
if (model->state == MODEL_LOADED)
return 0;
@@ -26,31 +26,10 @@ ml_model_load(struct ml_test *test, struct ml_options *opt, struct ml_model *mod
return -EINVAL;
/* read model binary */
- fp = fopen(opt->filelist[fid].model, "r");
- if (fp == NULL) {
- ml_err("Failed to open model file : %s\n", opt->filelist[fid].model);
- return -1;
- }
-
- fseek(fp, 0, SEEK_END);
- model_params.size = ftell(fp);
- fseek(fp, 0, SEEK_SET);
-
- model_params.addr = rte_malloc_socket("ml_model", model_params.size,
- t->dev_info.min_align_size, opt->socket_id);
- if (model_params.addr == NULL) {
- ml_err("Failed to allocate memory for model: %s\n", opt->filelist[fid].model);
- fclose(fp);
- return -ENOMEM;
- }
-
- if (fread(model_params.addr, 1, model_params.size, fp) != model_params.size) {
- ml_err("Failed to read model file : %s\n", opt->filelist[fid].model);
- rte_free(model_params.addr);
- fclose(fp);
- return -1;
- }
- fclose(fp);
+ ret = ml_read_file(opt->filelist[fid].model, &model_params.size,
+ (char **)&model_params.addr);
+ if (ret != 0)
+ return ret;
/* load model to device */
ret = rte_ml_model_load(opt->dev_id, &model_params, &model->id);