* Re: [PATCH 1/1] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
@ 2023-03-28 15:52 ` Stephen Hemminger
2023-04-12 8:48 ` [EXT] " Srikanth Yalavarthi
2023-04-23 4:55 ` [PATCH v2] " Srikanth Yalavarthi
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2023-03-28 15:52 UTC (permalink / raw)
To: Srikanth Yalavarthi; +Cc: Anup Prabhu, dev, sshankarnara, ptakkar
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
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [EXT] Re: [PATCH 1/1] app/mldev: add internal function for file read
2023-03-28 15:52 ` Stephen Hemminger
@ 2023-04-12 8:48 ` Srikanth Yalavarthi
0 siblings, 0 replies; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-04-12 8:48 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Anup Prabhu, dev, Shivah Shankar Shankar Narayan Rao,
Prince Takkar, Srikanth Yalavarthi
> -----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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
2023-03-28 15:52 ` Stephen Hemminger
@ 2023-04-23 4:55 ` Srikanth Yalavarthi
2023-05-03 8:56 ` [PATCH v3] " Srikanth Yalavarthi
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-04-23 4:55 UTC (permalink / raw)
To: Srikanth Yalavarthi, Anup Prabhu; +Cc: dev, sshankarnara, ptakkar
Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.
Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")
Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
app/test-mldev/test_common.c | 59 ++++++++++++++++++++++++++
app/test-mldev/test_common.h | 2 +
app/test-mldev/test_inference_common.c | 54 +++++++++--------------
app/test-mldev/test_model_common.c | 33 +++-----------
4 files changed, 87 insertions(+), 61 deletions(-)
diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..be67ea487c 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -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 = malloc(file_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)
{
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index a7b2ea652a..7e3634b0c6 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -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 */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index 29c18bbc85..96213413a2 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -610,10 +610,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 */
@@ -653,51 +653,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
@@ -729,6 +713,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;
}
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..7a8c284d13 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -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);
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v3] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
2023-03-28 15:52 ` Stephen Hemminger
2023-04-23 4:55 ` [PATCH v2] " Srikanth Yalavarthi
@ 2023-05-03 8:56 ` Srikanth Yalavarthi
2023-05-03 14:54 ` Stephen Hemminger
2023-06-07 11:35 ` [PATCH v4] " Srikanth Yalavarthi
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-05-03 8:56 UTC (permalink / raw)
To: Srikanth Yalavarthi, Anup Prabhu; +Cc: dev, sshankarnara, ptakkar
Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.
Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")
Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v3:
* Fix incorrect use of rte_free with free
v2:
* Replace rte_malloc in ml_read_file with malloc
v1:
* Initial patch
app/test-mldev/test_common.c | 59 ++++++++++++++++++++++++++
app/test-mldev/test_common.h | 2 +
app/test-mldev/test_inference_common.c | 54 +++++++++--------------
app/test-mldev/test_model_common.c | 39 ++++-------------
4 files changed, 90 insertions(+), 64 deletions(-)
diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..d8a8e8a448 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -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 = malloc(file_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:
+ free(file_buffer);
+
+ if (fp != NULL)
+ fclose(fp);
+
+ return ret;
+}
+
bool
ml_test_cap_check(struct ml_options *opt)
{
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index a7b2ea652a..7e3634b0c6 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -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 */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index af831fc1bf..9a1c706e11 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -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);
+ 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);
+ 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;
}
+ free(buffer);
+
return ret;
}
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -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,43 +26,22 @@ 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);
if (ret != 0) {
ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
model->state = MODEL_ERROR;
- rte_free(model_params.addr);
+ free(model_params.addr);
return ret;
}
- /* release mz */
- rte_free(model_params.addr);
+ /* release buffer */
+ free(model_params.addr);
/* get model info */
ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3] app/mldev: add internal function for file read
2023-05-03 8:56 ` [PATCH v3] " Srikanth Yalavarthi
@ 2023-05-03 14:54 ` Stephen Hemminger
2023-05-03 14:59 ` [EXT] " Srikanth Yalavarthi
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2023-05-03 14:54 UTC (permalink / raw)
To: Srikanth Yalavarthi; +Cc: Anup Prabhu, dev, sshankarnara, ptakkar
On Wed, 3 May 2023 01:56:41 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> Added internal function to read model, input and reference
> files with required error checks. This change fixes the
> unchecked return value and improper use of negative value
> issues reported by coverity scan for file read operations.
>
> Coverity issue: 383742, 383743
> Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> Fixes: da6793390596 ("app/mldev: support inference validation")
>
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> ---
> v3:
> * Fix incorrect use of rte_free with free
>
> v2:
> * Replace rte_malloc in ml_read_file with malloc
>
> v1:
> * Initial patch
>
> app/test-mldev/test_common.c | 59 ++++++++++++++++++++++++++
> app/test-mldev/test_common.h | 2 +
> app/test-mldev/test_inference_common.c | 54 +++++++++--------------
> app/test-mldev/test_model_common.c | 39 ++++-------------
> 4 files changed, 90 insertions(+), 64 deletions(-)
>
> diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
> index 016b31c6ba..d8a8e8a448 100644
> --- a/app/test-mldev/test_common.c
> +++ b/app/test-mldev/test_common.c
> @@ -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 = malloc(file_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 {
Granted this is a test program. But why did you ignore my feedback that this
is the slowest way to read a file. Stdio requires extra buffering, use regular read() or
better yet mmap().
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read
2023-05-03 14:54 ` Stephen Hemminger
@ 2023-05-03 14:59 ` Srikanth Yalavarthi
2023-05-03 18:28 ` Stephen Hemminger
0 siblings, 1 reply; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-05-03 14:59 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Anup Prabhu, dev, Shivah Shankar Shankar Narayan Rao,
Prince Takkar, Srikanth Yalavarthi
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 03 May 2023 20:24
> 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>
> Subject: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read
>
> External Email
>
> ----------------------------------------------------------------------
> On Wed, 3 May 2023 01:56:41 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
>
> > Added internal function to read model, input and reference files with
> > required error checks. This change fixes the unchecked return value
> > and improper use of negative value issues reported by coverity scan
> > for file read operations.
> >
> > Coverity issue: 383742, 383743
> > Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> > Fixes: da6793390596 ("app/mldev: support inference validation")
> >
> > Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
> > ---
> > v3:
> > * Fix incorrect use of rte_free with free
> >
> > v2:
> > * Replace rte_malloc in ml_read_file with malloc
> >
> > v1:
> > * Initial patch
> >
> > app/test-mldev/test_common.c | 59 ++++++++++++++++++++++++++
> > app/test-mldev/test_common.h | 2 +
> > app/test-mldev/test_inference_common.c | 54 +++++++++--------------
> > app/test-mldev/test_model_common.c | 39 ++++-------------
> > 4 files changed, 90 insertions(+), 64 deletions(-)
> >
> > diff --git a/app/test-mldev/test_common.c
> > b/app/test-mldev/test_common.c index 016b31c6ba..d8a8e8a448 100644
> > --- a/app/test-mldev/test_common.c
> > +++ b/app/test-mldev/test_common.c
> > @@ -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 = malloc(file_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 {
>
>
> Granted this is a test program. But why did you ignore my feedback that this
> is the slowest way to read a file. Stdio requires extra buffering, use regular
> read() or better yet mmap().
Agree on the improvement, 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.
I had shared my additional comments on v2 patch.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read
2023-05-03 14:59 ` [EXT] " Srikanth Yalavarthi
@ 2023-05-03 18:28 ` Stephen Hemminger
2023-05-03 23:04 ` Tyler Retzlaff
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2023-05-03 18:28 UTC (permalink / raw)
To: Srikanth Yalavarthi
Cc: Anup Prabhu, dev, Shivah Shankar Shankar Narayan Rao, Prince Takkar
On Wed, 3 May 2023 14:59:40 +0000
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> >
> > Granted this is a test program. But why did you ignore my feedback that this
> > is the slowest way to read a file. Stdio requires extra buffering, use regular
> > read() or better yet mmap().
>
> Agree on the improvement, 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.
>
> I had shared my additional comments on v2 patch.
Using system calls read/write is used lots of places in DPDK already and is portable
to all the supported platforms.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [EXT] Re: [PATCH v3] app/mldev: add internal function for file read
2023-05-03 18:28 ` Stephen Hemminger
@ 2023-05-03 23:04 ` Tyler Retzlaff
0 siblings, 0 replies; 17+ messages in thread
From: Tyler Retzlaff @ 2023-05-03 23:04 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Srikanth Yalavarthi, Anup Prabhu, dev,
Shivah Shankar Shankar Narayan Rao, Prince Takkar
On Wed, May 03, 2023 at 11:28:26AM -0700, Stephen Hemminger wrote:
> On Wed, 3 May 2023 14:59:40 +0000
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
>
> > >
> > > Granted this is a test program. But why did you ignore my feedback that this
> > > is the slowest way to read a file. Stdio requires extra buffering, use regular
> > > read() or better yet mmap().
> >
> > Agree on the improvement, 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.
> >
> > I had shared my additional comments on v2 patch.
>
> Using system calls read/write is used lots of places in DPDK already and is portable
> to all the supported platforms.
well almost, the windows standard c library implements a subset of
POSIX.1 (ISO/IEC 9945-1:1996) and there should be a strong emphasis on
`a subset' as in it is not fully conformant to any specific POSIX standard.
also because they aren't technically part of the standard C library
(again POSIX is not standard C) they are exposed with different names on
windows by prepending a leading `_' to the names. so you get `_read' instead
of `read' for example.
you can force exposure of the non-conforming names (i.e. the POSIX
names) with the _CRT_DECLARE_NONSTDC_NAMES define but if you do and you use
them you may then get deprecation warnings.
anyway, i read above nobody cares if this code ever runs on anything but
Linux ~forever so i won't make it my business to comment further unless
there is a desire to include windows.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v4] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
` (2 preceding siblings ...)
2023-05-03 8:56 ` [PATCH v3] " Srikanth Yalavarthi
@ 2023-06-07 11:35 ` Srikanth Yalavarthi
2023-06-07 15:02 ` Stephen Hemminger
2023-06-07 16:20 ` [PATCH v5] " Srikanth Yalavarthi
2023-06-07 17:24 ` [PATCH v6] " Srikanth Yalavarthi
5 siblings, 1 reply; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-06-07 11:35 UTC (permalink / raw)
To: Srikanth Yalavarthi, Anup Prabhu; +Cc: dev, sshankarnara, ptakkar
Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.
Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")
Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v4:
* Drop use of fread and replace with systme calls
v3:
* Fix incorrect use of rte_free with free
v2:
* Replace rte_malloc in ml_read_file with malloc
app/test-mldev/test_common.c | 57 ++++++++++++++++++++++++++
app/test-mldev/test_common.h | 2 +
app/test-mldev/test_inference_common.c | 56 ++++++++++---------------
app/test-mldev/test_model_common.c | 39 ++++--------------
4 files changed, 90 insertions(+), 64 deletions(-)
diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..2449de0da9 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,69 @@
#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"
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+ char *file_buffer = NULL;
+ struct stat file_stat;
+ char *file_map;
+ int ret;
+ int fd;
+
+ fd = open(file, O_RDONLY);
+ if (fd == -1) {
+ ml_err("Failed to open file: %s\n", file);
+ return -errno;
+ }
+
+ if (fstat(fd, &file_stat) != 0) {
+ ml_err("fstat failed for file: %s\n", file);
+ return -errno;
+ }
+
+ file_buffer = malloc(file_stat.st_size);
+ if (file_buffer == NULL) {
+ ml_err("Failed to allocate memory: %s\n", file);
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (file_map == MAP_FAILED) {
+ ml_err("Failed to map file: %s\n", file);
+ ret = -errno;
+ goto error;
+ }
+
+ rte_memcpy(file_buffer, file_map, file_stat.st_size);
+ munmap(file_map, file_stat.st_size);
+
+ *size = file_stat.st_size;
+ *buffer = file_buffer;
+
+ return 0;
+
+error:
+ if (file_buffer)
+ free(file_buffer);
+
+ close(fd);
+
+ return ret;
+}
+
bool
ml_test_cap_check(struct ml_options *opt)
{
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,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 */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index 469ed35f6c..aa59066ff9 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -593,10 +593,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 */
@@ -636,51 +636,36 @@ 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);
+ 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 */
+ buffer = NULL;
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);
+ 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
@@ -712,6 +697,9 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
t->model[fid].io_pool = NULL;
}
+ if (buffer)
+ free(buffer);
+
return ret;
}
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -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,43 +26,22 @@ 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);
if (ret != 0) {
ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
model->state = MODEL_ERROR;
- rte_free(model_params.addr);
+ free(model_params.addr);
return ret;
}
- /* release mz */
- rte_free(model_params.addr);
+ /* release buffer */
+ free(model_params.addr);
/* get model info */
ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4] app/mldev: add internal function for file read
2023-06-07 11:35 ` [PATCH v4] " Srikanth Yalavarthi
@ 2023-06-07 15:02 ` Stephen Hemminger
2023-06-07 16:21 ` [EXT] " Srikanth Yalavarthi
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2023-06-07 15:02 UTC (permalink / raw)
To: Srikanth Yalavarthi; +Cc: Anup Prabhu, dev, sshankarnara, ptakkar
On Wed, 7 Jun 2023 04:35:56 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> + if (file_buffer)
> + free(file_buffer);
Unnecessary if() since free(NULL) is ok.
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [EXT] Re: [PATCH v4] app/mldev: add internal function for file read
2023-06-07 15:02 ` Stephen Hemminger
@ 2023-06-07 16:21 ` Srikanth Yalavarthi
0 siblings, 0 replies; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-06-07 16:21 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Anup Prabhu, dev, Shivah Shankar Shankar Narayan Rao,
Prince Takkar, Srikanth Yalavarthi
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 07 June 2023 20:32
> 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>
> Subject: [EXT] Re: [PATCH v4] app/mldev: add internal function for file read
>
> External Email
>
> ----------------------------------------------------------------------
> On Wed, 7 Jun 2023 04:35:56 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
>
> > + if (file_buffer)
> > + free(file_buffer);
>
> Unnecessary if() since free(NULL) is ok.
Dropped if(). Updated in Version 5.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v5] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
` (3 preceding siblings ...)
2023-06-07 11:35 ` [PATCH v4] " Srikanth Yalavarthi
@ 2023-06-07 16:20 ` Srikanth Yalavarthi
2023-06-07 16:49 ` Stephen Hemminger
2023-06-07 17:24 ` [PATCH v6] " Srikanth Yalavarthi
5 siblings, 1 reply; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-06-07 16:20 UTC (permalink / raw)
To: Srikanth Yalavarthi, Anup Prabhu; +Cc: dev, sshankarnara, ptakkar
Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.
Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")
Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v5:
* Updated as per review comments
v4:
* Drop use of fread and replace with system calls
v3:
* Fix incorrect use of rte_free with free
v2:
* Replace rte_malloc in ml_read_file with malloc
app/test-mldev/test_common.c | 56 ++++++++++++++++++++++++++
app/test-mldev/test_common.h | 2 +
app/test-mldev/test_inference_common.c | 56 ++++++++++----------------
app/test-mldev/test_model_common.c | 39 +++++-------------
4 files changed, 89 insertions(+), 64 deletions(-)
diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..cc38e8af05 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,68 @@
#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"
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+ char *file_buffer = NULL;
+ struct stat file_stat;
+ char *file_map;
+ int ret;
+ int fd;
+
+ fd = open(file, O_RDONLY);
+ if (fd == -1) {
+ ml_err("Failed to open file: %s\n", file);
+ return -errno;
+ }
+
+ if (fstat(fd, &file_stat) != 0) {
+ ml_err("fstat failed for file: %s\n", file);
+ return -errno;
+ }
+
+ file_buffer = malloc(file_stat.st_size);
+ if (file_buffer == NULL) {
+ ml_err("Failed to allocate memory: %s\n", file);
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (file_map == MAP_FAILED) {
+ ml_err("Failed to map file: %s\n", file);
+ ret = -errno;
+ goto error;
+ }
+
+ rte_memcpy(file_buffer, file_map, file_stat.st_size);
+ munmap(file_map, file_stat.st_size);
+
+ *size = file_stat.st_size;
+ *buffer = file_buffer;
+
+ return 0;
+
+error:
+ free(file_buffer);
+
+ close(fd);
+
+ return ret;
+}
+
bool
ml_test_cap_check(struct ml_options *opt)
{
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,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 */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index d929ff6e61..8a6e4725df 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -599,10 +599,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 */
@@ -642,51 +642,36 @@ 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);
+ 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 */
+ buffer = NULL;
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);
+ 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
@@ -718,6 +703,9 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
t->model[fid].io_pool = NULL;
}
+ if (buffer)
+ free(buffer);
+
return ret;
}
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -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,43 +26,22 @@ 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);
if (ret != 0) {
ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
model->state = MODEL_ERROR;
- rte_free(model_params.addr);
+ free(model_params.addr);
return ret;
}
- /* release mz */
- rte_free(model_params.addr);
+ /* release buffer */
+ free(model_params.addr);
/* get model info */
ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v5] app/mldev: add internal function for file read
2023-06-07 16:20 ` [PATCH v5] " Srikanth Yalavarthi
@ 2023-06-07 16:49 ` Stephen Hemminger
2023-06-07 17:24 ` [EXT] " Srikanth Yalavarthi
0 siblings, 1 reply; 17+ messages in thread
From: Stephen Hemminger @ 2023-06-07 16:49 UTC (permalink / raw)
To: Srikanth Yalavarthi; +Cc: Anup Prabhu, dev, sshankarnara, ptakkar
On Wed, 7 Jun 2023 09:20:30 -0700
Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
The normal case leaks the open file descriptor
+
+ file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (file_map == MAP_FAILED) {
+ ml_err("Failed to map file: %s\n", file);
+ ret = -errno;
+ goto error;
+ }
+
+ rte_memcpy(file_buffer, file_map, file_stat.st_size);
+ munmap(file_map, file_stat.st_size);
+
+ *size = file_stat.st_size;
+ *buffer = file_buffer;
+
+ return 0;
Missing close()
> + if (buffer)
> + free(buffer);
> +
Another needless if()
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [EXT] Re: [PATCH v5] app/mldev: add internal function for file read
2023-06-07 16:49 ` Stephen Hemminger
@ 2023-06-07 17:24 ` Srikanth Yalavarthi
0 siblings, 0 replies; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-06-07 17:24 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Anup Prabhu, dev, Shivah Shankar Shankar Narayan Rao,
Prince Takkar, Srikanth Yalavarthi
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 07 June 2023 22:19
> 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>
> Subject: [EXT] Re: [PATCH v5] app/mldev: add internal function for file read
>
> External Email
>
> ----------------------------------------------------------------------
> On Wed, 7 Jun 2023 09:20:30 -0700
> Srikanth Yalavarthi <syalavarthi@marvell.com> wrote:
> The normal case leaks the open file descriptor
>
> +
> + file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd,
> 0);
> + if (file_map == MAP_FAILED) {
> + ml_err("Failed to map file: %s\n", file);
> + ret = -errno;
> + goto error;
> + }
> +
> + rte_memcpy(file_buffer, file_map, file_stat.st_size);
> + munmap(file_map, file_stat.st_size);
> +
> + *size = file_stat.st_size;
> + *buffer = file_buffer;
> +
> + return 0;
>
> Missing close()
>
> > + if (buffer)
> > + free(buffer);
> > +
>
> Another needless if()
Updated in v6.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6] app/mldev: add internal function for file read
2023-03-23 15:28 [PATCH 1/1] app/mldev: add internal function for file read Srikanth Yalavarthi
` (4 preceding siblings ...)
2023-06-07 16:20 ` [PATCH v5] " Srikanth Yalavarthi
@ 2023-06-07 17:24 ` Srikanth Yalavarthi
2023-07-07 8:06 ` Thomas Monjalon
5 siblings, 1 reply; 17+ messages in thread
From: Srikanth Yalavarthi @ 2023-06-07 17:24 UTC (permalink / raw)
To: Srikanth Yalavarthi, Anup Prabhu; +Cc: dev, sshankarnara, ptakkar
Added internal function to read model, input and reference
files with required error checks. This change fixes the
unchecked return value and improper use of negative value
issues reported by coverity scan for file read operations.
Coverity issue: 383742, 383743
Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
Fixes: da6793390596 ("app/mldev: support inference validation")
Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
---
v6:
* Addressed additional review comments
v5:
* Updated as per review comments
v4:
* Drop use of fread and replace with system calls
v3:
* Fix incorrect use of rte_free with free
v2:
* Replace rte_malloc in ml_read_file with malloc
app/test-mldev/test_common.c | 56 ++++++++++++++++++++++++++
app/test-mldev/test_common.h | 2 +
app/test-mldev/test_inference_common.c | 55 ++++++++++---------------
app/test-mldev/test_model_common.c | 39 +++++-------------
4 files changed, 88 insertions(+), 64 deletions(-)
diff --git a/app/test-mldev/test_common.c b/app/test-mldev/test_common.c
index 016b31c6ba..891e8d0ca8 100644
--- a/app/test-mldev/test_common.c
+++ b/app/test-mldev/test_common.c
@@ -5,12 +5,68 @@
#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"
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+int
+ml_read_file(char *file, size_t *size, char **buffer)
+{
+ char *file_buffer = NULL;
+ struct stat file_stat;
+ char *file_map;
+ int ret;
+ int fd;
+
+ fd = open(file, O_RDONLY);
+ if (fd == -1) {
+ ml_err("Failed to open file: %s\n", file);
+ return -errno;
+ }
+
+ if (fstat(fd, &file_stat) != 0) {
+ ml_err("fstat failed for file: %s\n", file);
+ return -errno;
+ }
+
+ file_buffer = malloc(file_stat.st_size);
+ if (file_buffer == NULL) {
+ ml_err("Failed to allocate memory: %s\n", file);
+ ret = -ENOMEM;
+ goto error;
+ }
+
+ file_map = mmap(0, file_stat.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (file_map == MAP_FAILED) {
+ ml_err("Failed to map file: %s\n", file);
+ ret = -errno;
+ goto error;
+ }
+
+ rte_memcpy(file_buffer, file_map, file_stat.st_size);
+ munmap(file_map, file_stat.st_size);
+ close(fd);
+
+ *size = file_stat.st_size;
+ *buffer = file_buffer;
+
+ return 0;
+
+error:
+ free(file_buffer);
+ close(fd);
+
+ return ret;
+}
+
bool
ml_test_cap_check(struct ml_options *opt)
{
diff --git a/app/test-mldev/test_common.h b/app/test-mldev/test_common.h
index def108d5b2..793841a917 100644
--- a/app/test-mldev/test_common.h
+++ b/app/test-mldev/test_common.h
@@ -27,4 +27,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 */
diff --git a/app/test-mldev/test_inference_common.c b/app/test-mldev/test_inference_common.c
index d929ff6e61..231ef2defa 100644
--- a/app/test-mldev/test_inference_common.c
+++ b/app/test-mldev/test_inference_common.c
@@ -599,10 +599,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 */
@@ -642,51 +642,36 @@ 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);
+ 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 */
+ buffer = NULL;
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);
+ 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
@@ -718,6 +703,8 @@ ml_inference_iomem_setup(struct ml_test *test, struct ml_options *opt, uint16_t
t->model[fid].io_pool = NULL;
}
+ free(buffer);
+
return ret;
}
diff --git a/app/test-mldev/test_model_common.c b/app/test-mldev/test_model_common.c
index c28e452f29..8dbb0ff89f 100644
--- a/app/test-mldev/test_model_common.c
+++ b/app/test-mldev/test_model_common.c
@@ -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,43 +26,22 @@ 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);
if (ret != 0) {
ml_err("Failed to load model : %s\n", opt->filelist[fid].model);
model->state = MODEL_ERROR;
- rte_free(model_params.addr);
+ free(model_params.addr);
return ret;
}
- /* release mz */
- rte_free(model_params.addr);
+ /* release buffer */
+ free(model_params.addr);
/* get model info */
ret = rte_ml_model_info_get(opt->dev_id, model->id, &model->info);
--
2.17.1
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v6] app/mldev: add internal function for file read
2023-06-07 17:24 ` [PATCH v6] " Srikanth Yalavarthi
@ 2023-07-07 8:06 ` Thomas Monjalon
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2023-07-07 8:06 UTC (permalink / raw)
To: Srikanth Yalavarthi; +Cc: Anup Prabhu, dev, sshankarnara, ptakkar
07/06/2023 19:24, Srikanth Yalavarthi:
> Added internal function to read model, input and reference
> files with required error checks. This change fixes the
> unchecked return value and improper use of negative value
> issues reported by coverity scan for file read operations.
>
> Coverity issue: 383742, 383743
> Fixes: f6661e6d9a3a ("app/mldev: validate model operations")
> Fixes: da6793390596 ("app/mldev: support inference validation")
>
> Signed-off-by: Srikanth Yalavarthi <syalavarthi@marvell.com>
Applied, thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread