RE: [EXT] Re: [PATCH 1/1] app/mldev: add internal function for file read

[Srikanth Yalavarthi <syalavarthi@marvell.com>]

> -----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.