Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Shreyansh Jain]

Hi Jan,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Viktorin
> Sent: Friday, May 06, 2016 7:18 PM
> To: dev@dpdk.org
> Cc: Jan Viktorin <viktorin@rehivetech.com>; David Marchand
> <david.marchand@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> Bruce Richardson <bruce.richardson@intel.com>; Declan Doherty
> <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>
> Subject: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> eal_parse_sysfs_valuef
> 
> The eal_parse_sysfs_value function accepts a filename however, such interface
> introduces race-conditions to the code. Introduce the variant of this
> function
> that accepts an already opened file instead of a filename.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> ---
>  lib/librte_eal/common/eal_filesystem.h |  5 +++++
>  lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++---------
> --
>  2 files changed, 30 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_filesystem.h
> b/lib/librte_eal/common/eal_filesystem.h
> index fdb4a70..7875454 100644
> --- a/lib/librte_eal/common/eal_filesystem.h
> +++ b/lib/librte_eal/common/eal_filesystem.h
> @@ -43,6 +43,7 @@
>  /** Path of rte config file. */
>  #define RUNTIME_CONFIG_FMT "%s/.%s_config"
> 
> +#include <stdio.h>
>  #include <stdint.h>
>  #include <limits.h>
>  #include <unistd.h>
> @@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t buflen,
> const char *hugedir, int
>   * Used to read information from files on /sys */
>  int eal_parse_sysfs_value(const char *filename, unsigned long *val);
> 
> +/** Function to read a single numeric value from a file on the filesystem.
> + * Used to read information from files on /sys */
> +int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
> +
>  #endif /* EAL_FILESYSTEM_H */
> diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> b/lib/librte_eal/linuxapp/eal/eal.c
> index 4b28197..e8fce6b 100644
> --- a/lib/librte_eal/linuxapp/eal/eal.c
> +++ b/lib/librte_eal/linuxapp/eal/eal.c
> @@ -126,13 +126,30 @@ rte_eal_get_configuration(void)
>  	return &rte_config;
>  }
> 
> +int
> +eal_parse_sysfs_valuef(FILE *f, unsigned long *val)

Trivial Comment: Maybe it is just me, but this function name is too close to its caller 'eal_parse_sysfs_value'. Probably, the name of the caller can be changed to 'eal_parse_sysfs' because anyways value parsing is being done in this ('eal_parse_sysfs_valuef()) function now. And, of course, dropping the '..f' in this name.

I almost skipped the '..f' in the name and wondered how two functions having same name exist :D

> +{
> +	char buf[BUFSIZ];
> +	char *end = NULL;
> +
> +	RTE_VERIFY(f != NULL);
> +
> +	if (fgets(buf, sizeof(buf), f) == NULL)
> +		return -1;
> +
> +	*val = strtoul(buf, &end, 0);
> +	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n'))
> +		return -2;
> +
> +	return 0;
> +}
> +
>  /* parse a sysfs (or other) file containing one integer value */
>  int
>  eal_parse_sysfs_value(const char *filename, unsigned long *val)
>  {
> +	int ret;
>  	FILE *f;
> -	char buf[BUFSIZ];
> -	char *end = NULL;
> 
>  	if ((f = fopen(filename, "r")) == NULL) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
> @@ -140,21 +157,18 @@ eal_parse_sysfs_value(const char *filename, unsigned
> long *val)
>  		return -1;
>  	}
> 
> -	if (fgets(buf, sizeof(buf), f) == NULL) {
> +	ret = eal_parse_sysfs_valuef(f, val);
> +	if (ret == -1) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
> -			__func__, filename);
> -		fclose(f);
> -		return -1;
> +				__func__, filename);
>  	}
> -	*val = strtoul(buf, &end, 0);
> -	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
> +	else if (ret < 0) {
>  		RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
>  				__func__, filename);
> -		fclose(f);
> -		return -1;
>  	}
> +
>  	fclose(f);
> -	return 0;
> +	return ret;
>  }
> 
> 
> --
> 2.8.0

-
Shreyansh

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Jan Viktorin]

On Mon, 13 Jun 2016 14:18:40 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> Hi Jan,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Viktorin
> > Sent: Friday, May 06, 2016 7:18 PM
> > To: dev@dpdk.org
> > Cc: Jan Viktorin <viktorin@rehivetech.com>; David Marchand
> > <david.marchand@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> > Bruce Richardson <bruce.richardson@intel.com>; Declan Doherty
> > <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>
> > Subject: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > eal_parse_sysfs_valuef
> > 
> > The eal_parse_sysfs_value function accepts a filename however, such interface
> > introduces race-conditions to the code. Introduce the variant of this
> > function
> > that accepts an already opened file instead of a filename.
> > 
> > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > ---
> >  lib/librte_eal/common/eal_filesystem.h |  5 +++++
> >  lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++---------
> > --
> >  2 files changed, 30 insertions(+), 11 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/eal_filesystem.h
> > b/lib/librte_eal/common/eal_filesystem.h
> > index fdb4a70..7875454 100644
> > --- a/lib/librte_eal/common/eal_filesystem.h
> > +++ b/lib/librte_eal/common/eal_filesystem.h
> > @@ -43,6 +43,7 @@
> >  /** Path of rte config file. */
> >  #define RUNTIME_CONFIG_FMT "%s/.%s_config"
> > 
> > +#include <stdio.h>
> >  #include <stdint.h>
> >  #include <limits.h>
> >  #include <unistd.h>
> > @@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t buflen,
> > const char *hugedir, int
> >   * Used to read information from files on /sys */
> >  int eal_parse_sysfs_value(const char *filename, unsigned long *val);
> > 
> > +/** Function to read a single numeric value from a file on the filesystem.
> > + * Used to read information from files on /sys */
> > +int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
> > +
> >  #endif /* EAL_FILESYSTEM_H */
> > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > b/lib/librte_eal/linuxapp/eal/eal.c
> > index 4b28197..e8fce6b 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > @@ -126,13 +126,30 @@ rte_eal_get_configuration(void)
> >  	return &rte_config;
> >  }
> > 
> > +int
> > +eal_parse_sysfs_valuef(FILE *f, unsigned long *val)  

Hi Shreyansh,

> 
> Trivial Comment: Maybe it is just me, but this function name is too close to its caller 'eal_parse_sysfs_value'. Probably, the name of the caller can be changed to 'eal_parse_sysfs' because anyways value parsing is being done in this ('eal_parse_sysfs_valuef()) function now. And, of course, dropping the '..f' in this name.

I don't like the idea of renaming the orignal function eal_parse_sysfs_value. The function
name is not related to its actual body but to its semantics. And the semantics are still
the same. This would introduce many other unneeded changes...

> 
> I almost skipped the '..f' in the name and wondered how two functions having same name exist :D

I agree that a better name would be nice here. This convention was based on the libc naming
(fopen, fclose) but the "f" letter could not be at the beginning.

What about one of those?

* eal_parse_sysfs_fd_value
* eal_parse_sysfs_file_value

Regards
Jan

[...]

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Shreyansh Jain]

Hi Jan,

> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> Sent: Monday, June 13, 2016 7:55 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> Cc: dev@dpdk.org; David Marchand <david.marchand@6wind.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Declan Doherty <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>
> Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> eal_parse_sysfs_valuef
> 
> On Mon, 13 Jun 2016 14:18:40 +0000
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
> > Hi Jan,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Viktorin
> > > Sent: Friday, May 06, 2016 7:18 PM
> > > To: dev@dpdk.org
> > > Cc: Jan Viktorin <viktorin@rehivetech.com>; David Marchand
> > > <david.marchand@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> > > Bruce Richardson <bruce.richardson@intel.com>; Declan Doherty
> > > <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>;
> Stephen
> > > Hemminger <stephen@networkplumber.org>
> > > Subject: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > > eal_parse_sysfs_valuef
> > >
> > > The eal_parse_sysfs_value function accepts a filename however, such
> interface
> > > introduces race-conditions to the code. Introduce the variant of this
> > > function
> > > that accepts an already opened file instead of a filename.
> > >
> > > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > > ---
> > >  lib/librte_eal/common/eal_filesystem.h |  5 +++++
> > >  lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++-----
> ----
> > > --
> > >  2 files changed, 30 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/lib/librte_eal/common/eal_filesystem.h
> > > b/lib/librte_eal/common/eal_filesystem.h
> > > index fdb4a70..7875454 100644
> > > --- a/lib/librte_eal/common/eal_filesystem.h
> > > +++ b/lib/librte_eal/common/eal_filesystem.h
> > > @@ -43,6 +43,7 @@
> > >  /** Path of rte config file. */
> > >  #define RUNTIME_CONFIG_FMT "%s/.%s_config"
> > >
> > > +#include <stdio.h>
> > >  #include <stdint.h>
> > >  #include <limits.h>
> > >  #include <unistd.h>
> > > @@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t
> buflen,
> > > const char *hugedir, int
> > >   * Used to read information from files on /sys */
> > >  int eal_parse_sysfs_value(const char *filename, unsigned long *val);
> > >
> > > +/** Function to read a single numeric value from a file on the
> filesystem.
> > > + * Used to read information from files on /sys */
> > > +int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
> > > +
> > >  #endif /* EAL_FILESYSTEM_H */
> > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > > b/lib/librte_eal/linuxapp/eal/eal.c
> > > index 4b28197..e8fce6b 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > > @@ -126,13 +126,30 @@ rte_eal_get_configuration(void)
> > >  	return &rte_config;
> > >  }
> > >
> > > +int
> > > +eal_parse_sysfs_valuef(FILE *f, unsigned long *val)
> 
> Hi Shreyansh,
> 
> >
> > Trivial Comment: Maybe it is just me, but this function name is too close
> to its caller 'eal_parse_sysfs_value'. Probably, the name of the caller can
> be changed to 'eal_parse_sysfs' because anyways value parsing is being done
> in this ('eal_parse_sysfs_valuef()) function now. And, of course, dropping
> the '..f' in this name.
> 
> I don't like the idea of renaming the orignal function eal_parse_sysfs_value.
> The function
> name is not related to its actual body but to its semantics. And the
> semantics are still
> the same. This would introduce many other unneeded changes...
> 

Agree. I overlooked that.

> >
> > I almost skipped the '..f' in the name and wondered how two functions
> having same name exist :D
> 
> I agree that a better name would be nice here. This convention was based on
> the libc naming
> (fopen, fclose) but the "f" letter could not be at the beginning.
> 
> What about one of those?
> 
> * eal_parse_sysfs_fd_value
> * eal_parse_sysfs_file_value

I don't have any better idea than above.

Though, I still feel that 'eal_parse_sysfs_value -> eal_parse_sysfs_file_value' would be slightly asymmetrical - but again, this is highly subjective argument.

Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be...

But, eal_parse_sysfs_file_value is still preferred than eal_parse_sysfs_fd_value, for me.

> 
> Regards
> Jan
> 
> [...]

-
Shreyansh

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Jan Viktorin]

On Tue, 14 Jun 2016 04:30:57 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> Hi Jan,
> 
> > -----Original Message-----
> > From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> > Sent: Monday, June 13, 2016 7:55 PM
> > To: Shreyansh Jain <shreyansh.jain@nxp.com>
> > Cc: dev@dpdk.org; David Marchand <david.marchand@6wind.com>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>; Bruce Richardson <bruce.richardson@intel.com>;
> > Declan Doherty <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>
> > Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > eal_parse_sysfs_valuef
> > 
> > On Mon, 13 Jun 2016 14:18:40 +0000
> > Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> >   
> > > Hi Jan,
> > >  
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jan Viktorin
> > > > Sent: Friday, May 06, 2016 7:18 PM
> > > > To: dev@dpdk.org
> > > > Cc: Jan Viktorin <viktorin@rehivetech.com>; David Marchand
> > > > <david.marchand@6wind.com>; Thomas Monjalon <thomas.monjalon@6wind.com>;
> > > > Bruce Richardson <bruce.richardson@intel.com>; Declan Doherty
> > > > <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > > > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>;  
> > Stephen  
> > > > Hemminger <stephen@networkplumber.org>
> > > > Subject: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > > > eal_parse_sysfs_valuef
> > > >
> > > > The eal_parse_sysfs_value function accepts a filename however, such  
> > interface  
> > > > introduces race-conditions to the code. Introduce the variant of this
> > > > function
> > > > that accepts an already opened file instead of a filename.
> > > >
> > > > Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> > > > ---
> > > >  lib/librte_eal/common/eal_filesystem.h |  5 +++++
> > > >  lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++-----  
> > ----  
> > > > --
> > > >  2 files changed, 30 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eal/common/eal_filesystem.h
> > > > b/lib/librte_eal/common/eal_filesystem.h
> > > > index fdb4a70..7875454 100644
> > > > --- a/lib/librte_eal/common/eal_filesystem.h
> > > > +++ b/lib/librte_eal/common/eal_filesystem.h
> > > > @@ -43,6 +43,7 @@
> > > >  /** Path of rte config file. */
> > > >  #define RUNTIME_CONFIG_FMT "%s/.%s_config"
> > > >
> > > > +#include <stdio.h>
> > > >  #include <stdint.h>
> > > >  #include <limits.h>
> > > >  #include <unistd.h>
> > > > @@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t  
> > buflen,  
> > > > const char *hugedir, int
> > > >   * Used to read information from files on /sys */
> > > >  int eal_parse_sysfs_value(const char *filename, unsigned long *val);
> > > >
> > > > +/** Function to read a single numeric value from a file on the  
> > filesystem.  
> > > > + * Used to read information from files on /sys */
> > > > +int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
> > > > +
> > > >  #endif /* EAL_FILESYSTEM_H */
> > > > diff --git a/lib/librte_eal/linuxapp/eal/eal.c
> > > > b/lib/librte_eal/linuxapp/eal/eal.c
> > > > index 4b28197..e8fce6b 100644
> > > > --- a/lib/librte_eal/linuxapp/eal/eal.c
> > > > +++ b/lib/librte_eal/linuxapp/eal/eal.c
> > > > @@ -126,13 +126,30 @@ rte_eal_get_configuration(void)
> > > >  	return &rte_config;
> > > >  }
> > > >
> > > > +int
> > > > +eal_parse_sysfs_valuef(FILE *f, unsigned long *val)  
> > 
> > Hi Shreyansh,
> >   
> > >
> > > Trivial Comment: Maybe it is just me, but this function name is too close  
> > to its caller 'eal_parse_sysfs_value'. Probably, the name of the caller can
> > be changed to 'eal_parse_sysfs' because anyways value parsing is being done
> > in this ('eal_parse_sysfs_valuef()) function now. And, of course, dropping
> > the '..f' in this name.
> > 
> > I don't like the idea of renaming the orignal function eal_parse_sysfs_value.
> > The function
> > name is not related to its actual body but to its semantics. And the
> > semantics are still
> > the same. This would introduce many other unneeded changes...
> >   
> 
> Agree. I overlooked that.
> 
> > >
> > > I almost skipped the '..f' in the name and wondered how two functions  
> > having same name exist :D
> > 
> > I agree that a better name would be nice here. This convention was based on
> > the libc naming
> > (fopen, fclose) but the "f" letter could not be at the beginning.
> > 
> > What about one of those?
> > 
> > * eal_parse_sysfs_fd_value
> > * eal_parse_sysfs_file_value  
> 
> I don't have any better idea than above.
> 
> Though, I still feel that 'eal_parse_sysfs_value -> eal_parse_sysfs_file_value' would be slightly asymmetrical - but again, this is highly subjective argument.

I don't see any asymmetry here. The functions equal, just the new one accepts a file pointer instead of a path
and we don't have function name overloading in C.

> 
> Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be...

I think, I'll go with eal_parse_sysfs_file_value for v2. Ideally, it should be
eal_parse_sysfs_path_value and eal_parse_sysfs_file_value. Thus, this looks like
a good way.

> 
> But, eal_parse_sysfs_file_value is still preferred than eal_parse_sysfs_fd_value, for me.

Agree.

> 
> > 
> > Regards
> > Jan
> > 
> > [...]  
> 
> -
> Shreyansh

Thanks for comments
Jan

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Shreyansh Jain]

Sorry, didn't notice this email earlier...
Comments inline

> -----Original Message-----
> From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> Sent: Wednesday, June 15, 2016 3:26 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>
> Cc: dev@dpdk.org; David Marchand <david.marchand@6wind.com>; Thomas Monjalon
> <thomas.monjalon@6wind.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Declan Doherty <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> Hemminger <stephen@networkplumber.org>
> Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> eal_parse_sysfs_valuef
> 
> On Tue, 14 Jun 2016 04:30:57 +0000
> Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> 
> > Hi Jan,
> >

[...]


> > > >
> > > > I almost skipped the '..f' in the name and wondered how two functions
> > > having same name exist :D
> > >
> > > I agree that a better name would be nice here. This convention was based
> on
> > > the libc naming
> > > (fopen, fclose) but the "f" letter could not be at the beginning.
> > >
> > > What about one of those?
> > >
> > > * eal_parse_sysfs_fd_value
> > > * eal_parse_sysfs_file_value
> >
> > I don't have any better idea than above.
> >
> > Though, I still feel that 'eal_parse_sysfs_value ->
> eal_parse_sysfs_file_value' would be slightly asymmetrical - but again, this
> is highly subjective argument.
> 
> I don't see any asymmetry here. The functions equal, just the new one accepts
> a file pointer instead of a path
> and we don't have function name overloading in C.

Asymmetrical because cascading function names maybe additive for easy reading/recall.

'eal_parse_sysfs_value ==> eal_parse_sysfs_value_<XX> ==> eal_parse_sysfs_value_<XX>_<YY>'

Obviously, this is not a rule - it just makes reading and recalling of cascade easier.
As for:

eal_parse_sysfs_value => eal_parse_sysfs_file_value

inserts an identifier between a name, making it (slightly) difficult to correlate.

Again, as I mentioned earlier, this is subjective argument and matter of (personal!) choice.

> 
> >
> > Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be...
> 
> I think, I'll go with eal_parse_sysfs_file_value for v2. Ideally, it should
> be
> eal_parse_sysfs_path_value and eal_parse_sysfs_file_value. Thus, this looks
> like
> a good way.
> 
> >
> > But, eal_parse_sysfs_file_value is still preferred than
> eal_parse_sysfs_fd_value, for me.
> 
> Agree.
> 
[...]

-
Shreyansh

Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Jan Viktorin]

Hello Shreyansh,

On Thu, 16 Jun 2016 11:47:29 +0000
Shreyansh Jain <shreyansh.jain@nxp.com> wrote:

> Sorry, didn't notice this email earlier...
> Comments inline
> 
> > -----Original Message-----
> > From: Jan Viktorin [mailto:viktorin@rehivetech.com]
> > Sent: Wednesday, June 15, 2016 3:26 PM
> > To: Shreyansh Jain <shreyansh.jain@nxp.com>
> > Cc: dev@dpdk.org; David Marchand <david.marchand@6wind.com>; Thomas Monjalon
> > <thomas.monjalon@6wind.com>; Bruce Richardson <bruce.richardson@intel.com>;
> > Declan Doherty <declan.doherty@intel.com>; jianbo.liu@linaro.org;
> > jerin.jacob@caviumnetworks.com; Keith Wiles <keith.wiles@intel.com>; Stephen
> > Hemminger <stephen@networkplumber.org>
> > Subject: Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function
> > eal_parse_sysfs_valuef
> > 
> > On Tue, 14 Jun 2016 04:30:57 +0000
> > Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
> >   
> > > Hi Jan,
> > >  
> 
> [...]
> 
> 
> > > > >
> > > > > I almost skipped the '..f' in the name and wondered how two functions  
> > > > having same name exist :D
> > > >
> > > > I agree that a better name would be nice here. This convention was based  
> > on  
> > > > the libc naming
> > > > (fopen, fclose) but the "f" letter could not be at the beginning.
> > > >
> > > > What about one of those?
> > > >
> > > > * eal_parse_sysfs_fd_value
> > > > * eal_parse_sysfs_file_value  
> > >
> > > I don't have any better idea than above.
> > >
> > > Though, I still feel that 'eal_parse_sysfs_value ->  
> > eal_parse_sysfs_file_value' would be slightly asymmetrical - but again, this
> > is highly subjective argument.
> > 
> > I don't see any asymmetry here. The functions equal, just the new one accepts
> > a file pointer instead of a path
> > and we don't have function name overloading in C.  
> 
> Asymmetrical because cascading function names maybe additive for easy reading/recall.
> 
> 'eal_parse_sysfs_value ==> eal_parse_sysfs_value_<XX> ==> eal_parse_sysfs_value_<XX>_<YY>'
> 
> Obviously, this is not a rule - it just makes reading and recalling of cascade easier.
> As for:
> 
> eal_parse_sysfs_value => eal_parse_sysfs_file_value
> 
> inserts an identifier between a name, making it (slightly) difficult to correlate.
> 
> Again, as I mentioned earlier, this is subjective argument and matter of (personal!) choice.
> 
> >   
> > >
> > > Or, eal_parse_sysfs_value -> eal_parse_sysfs_value_read() may be...  
> > 
> > I think, I'll go with eal_parse_sysfs_file_value for v2. Ideally, it should
> > be
> > eal_parse_sysfs_path_value and eal_parse_sysfs_file_value. Thus, this looks
> > like
> > a good way.
> >   
> > >
> > > But, eal_parse_sysfs_file_value is still preferred than  
> > eal_parse_sysfs_fd_value, for me.
> > 
> > Agree.
> >   
> [...]

I've finally returned to your idea to name it eal_parse_sysfs_value_read.

Thanks.
Jan

> 
> -
> Shreyansh



-- 
   Jan Viktorin                  E-mail: Viktorin@RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic

[dpdk-dev] [PATCH v1 00/28] Support non-PCI devices

[Jan Viktorin]

Hello,

as a part of reworking the rte_(pci_)device/driver core of DPDK, I am
developing support for devices connected anotherway then by the PCI bus. I call
those devices _SoC devices_ and they are similar to the kernel's
platform_device. The goal is to have access to the on-chip MACs integrated
into many SoCs.

This patch set depends on

* [PATCH v2 00/17] prepare for rte_device / rte_driver
  http://thread.gmane.org/gmane.comp.networking.dpdk.devel/32387

Patches have been generated and tested on top of 84c9b5a (& 32387)
	
There are some BSD stubs (but I didn't test whether this is enough to not break
the BSD builds).

I've generalized certain internal (PCI-specific) functions of EAL to reuse them
by the SoC infrastructure (patches 1-5).

Then the SoC infra is introduced in quite small steps (6-22). For some of those
steps I've got an auto test (however not included to avoid introducing
dependencies on other series - (3)). Note:

* rte_soc_device/driver has a lot of common contents with rte_pci_* ones. This
  is a subject of future changes - introduction of rte_driver/device. The
  common members of device are:
  - next,
  - intr_handle,
  - numa_node,
  - devargs,
  - kdrv.

  The some for driver:
  - next
  - name
  - drv_flags
  
  Moreover, the addr, id and driver (from device), and devinit, devuninit
  and id_tables can be generalized to some degree as well.

* Do we want a list of PCI devices/drivers, a list of SoC devices/drivers
  (separated) or integrated into a single list and check for some type member?
  When iterating over a generic rte_driver/device, it introduces a lot of bloat
  code:

    struct rte_driver *drv;
    struct rte_pci_driver *pci_drv;
    TAILQ_FOREACH(drv, drivers_list, next) {
	    if (!is_pci(drv))
		    continue;
	    pci_drv = to_pci_driver(drv);
	    ...
    }

  I didn't find a way how to wrap this into something like PCI_DRV_FOREACH(...)
  (sys/queue.h is not suitable for this).

* rte_soc_resource and rte_pci_resource can be generalized to rte_resource.
  The similar might be possible for pci_map and mapped_pci_resource.

* RTE_*_DRV_* flags should be generalized.

* rte_soc_id has problem with the compatible property that must be const
  but we need to see it as non-const (GCC doesn't like it). Thus, I've used
  a workaround (union).

* rte_soc_addr contains fdt_path string - this can be connected with the FDT
  API (1) if possible later.

* I parse devargs by testing presence of a prefix "soc:" (not tested).

Finally (23-28), I created the necessary glue code to connect with
librte_ether. You can see that this is not a problem anymore, however, it
duplicates code. So, at this stage, the generic rte_driver/device is not
necessary but would be helpful.

I've also investigated how the VFIO and UIO work. After refactoring of the VFIO
as done in (2), it is possible to add a SoC-VFIO layer. Similar for UIO. It is
possible to utilize uio_pdev_genirq or (patched) uio_dmem_genirq. The
uio_dmem_genirq is better (after few small changes) as it provides access to
the DMA coherent memory. The VFIO should be used on platforms with I/O MMU
and with hugepages. I think, those extensions are for 16.11 as it's another
quite long amount of code. However, it it is not necessary to break APIs.

I've already posted 3 related patch sets:

(1) [RFC 0/6] Flattened Device Tree access from DPDK
    http://thread.gmane.org/gmane.comp.networking.dpdk.devel/36545

(2) [PATCH 00/15] Make VFIO support independent on PCI
    http://article.gmane.org/gmane.comp.networking.dpdk.devel/38154

(3) [PATCH v1 00/10] Include resources in tests
    http://thread.gmane.org/gmane.comp.networking.dpdk.devel/38459

I do my best to leave those patch sets independent on each other but they all
should finally work together.

The end :-). The patch set is designed to be merged partially, if needed
(due to its size) and at this stage it should help to solve the rte_driver /
rte_device task.

Regards
Jan

Jan Viktorin (28):
  eal: make enum rte_kernel_driver non-PCI specific
  eal: extract function eal_parse_sysfs_valuef
  eal/linux: extract function rte_eal_unbind_kernel_driver
  eal/linux: extract function rte_eal_get_kernel_driver_by_path
  eal: remove pci_ prefix from pci_(un)map_resource
  eal/soc: introduce very essential SoC infra definitions
  eal/soc: add rte_eal_soc_register/unregister logic
  eal/soc: implement SoC device discovery
  eal: introduce --no-soc option
  eal/soc: init SoC infra from EAL
  eal/soc: implement probing of drivers
  eal/soc: extend and utilize devargs
  eal/soc: update device on probe when already exists
  eal/soc: detect assigned kernel driver
  eal/soc: map/unmap resources
  eal/soc: add intr_handle
  eal/soc: hack (const char *) compatible setting
  eal/soc: detect numa_node of the rte_soc_device
  eal/soc: add drv_flags
  eal/soc: map resources conditionally
  eal/soc: unbind kernel driver on probe
  eal/soc: detect DMA non-coherent devices
  eal: define macro container_of
  ether: utilize container_of for pci_drv
  ether: verify we copy info from a PCI device
  ether: extract function eth_dev_get_intr_handle
  ether: extract function eth_dev_get_driver_name
  ether: support SoC device/driver

 app/test/Makefile                               |   1 +
 app/test/test_soc.c                             | 199 ++++++++++
 lib/librte_eal/bsdapp/eal/Makefile              |   2 +
 lib/librte_eal/bsdapp/eal/eal.c                 |   4 +
 lib/librte_eal/bsdapp/eal/eal_pci.c             |   2 +-
 lib/librte_eal/bsdapp/eal/eal_soc.c             |  52 +++
 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  11 +
 lib/librte_eal/common/Makefile                  |   2 +-
 lib/librte_eal/common/eal_common_dev.c          |  70 +++-
 lib/librte_eal/common/eal_common_devargs.c      |   7 +
 lib/librte_eal/common/eal_common_options.c      |   5 +
 lib/librte_eal/common/eal_common_pci.c          |  39 --
 lib/librte_eal/common/eal_common_pci_uio.c      |   6 +-
 lib/librte_eal/common/eal_common_soc.c          | 373 ++++++++++++++++++
 lib/librte_eal/common/eal_filesystem.h          |   5 +
 lib/librte_eal/common/eal_internal_cfg.h        |   1 +
 lib/librte_eal/common/eal_options.h             |   2 +
 lib/librte_eal/common/eal_private.h             |  96 +++++
 lib/librte_eal/common/include/rte_common.h      |  16 +
 lib/librte_eal/common/include/rte_dev.h         |   8 +
 lib/librte_eal/common/include/rte_devargs.h     |   8 +
 lib/librte_eal/common/include/rte_pci.h         |  42 +-
 lib/librte_eal/common/include/rte_soc.h         | 296 ++++++++++++++
 lib/librte_eal/linuxapp/eal/Makefile            |   2 +
 lib/librte_eal/linuxapp/eal/eal.c               |  98 ++++-
 lib/librte_eal/linuxapp/eal/eal_pci.c           |  62 +--
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c       |   3 +-
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c      |   4 +-
 lib/librte_eal/linuxapp/eal/eal_soc.c           | 492 ++++++++++++++++++++++++
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  13 +
 lib/librte_ether/rte_ethdev.c                   | 163 +++++++-
 lib/librte_ether/rte_ethdev.h                   |  33 +-
 32 files changed, 1952 insertions(+), 165 deletions(-)
 create mode 100644 app/test/test_soc.c
 create mode 100644 lib/librte_eal/bsdapp/eal/eal_soc.c
 create mode 100644 lib/librte_eal/common/eal_common_soc.c
 create mode 100644 lib/librte_eal/common/include/rte_soc.h
 create mode 100644 lib/librte_eal/linuxapp/eal/eal_soc.c

-- 
2.8.0

[dpdk-dev] [RFC 0/7] Support non-PCI devices

[Jan Viktorin]

Hello DPDK community,

I am proposing an RFC patch series that tries to add support for non-PCI
devices into DPDK. At the moment, the only way how to support such devices is
to utilize the virtual devices. As there are SoCs with integrated Ethernet MACs
(and other devices) that can be controled by DPDK, such infrastructure can be
helpful to write PMD for them. This is mostly the case of ARM SoCs. The ARMv7
SoCs have always an integrated EMAC (if any), the ARM+FPGA provides ways how
to put a custom EMACs into the SoC, and for ARM 64 there is the X-Gene board
with integrated 10G EMACs.

The patch set contains a simple self-test showing that this approach works. It
is possible to test it on any platform as it provides a fake sysfs and device
tree hierarchy.

I am introducing the pair rte_soc_device + rte_soc_driver and the probing code
for Linux. It is based on searching the /sys/bus/platform/devices and the
device-tree (for PMDs). No testing PMD is provided, just a simple test case
in app/test/test_soc.c.

I am unsure about the naming - rte_soc_*, however, it's the best one I've
devised so far.

I am aware of several code-issues and missing comments and moreover, there is
some infrastructure copied from the PCI code base. I will fix those in future
versions of the patch set if it is approved to the upstream. I'd be happy to
get some suggestions how to refactor the common code out from the PCI infra.

I am unaware of any other attempt to extend DPDK in this way so if there is
some work similar to this, I'd like to have a look at it.

Please, consider one important thing. There is no Linux Kernel driver that is
suitable for this kind of devices. Such driver must be PCI-independent, must
(should?) support no-IOMMU access to the device, must provide access to the
device memory specified in the device-tree's reg property, must give a way how
to perform DMA transfers, should be able to deliver interrupts, etc.

See the commits for more details...

Happy New Year!

Regards
Jan Viktorin

Jan Viktorin (7):
  eal/common: define rte_soc_* related common interface
  eal: introduce --no-soc option
  eal: add common part of the SoC infra
  eal/linuxapp: support SoC infra in linuxapp
  eal: init SoC infra on rte_eal_init
  eal/soc: make SoC infra testable on any platform
  app/test: add SoC infra probe/detach test

 app/test/Makefile                           |   3 +
 app/test/soc_test_init.sh                   |  61 +++++
 app/test/test_soc.c                         | 249 +++++++++++++++++
 lib/librte_eal/common/Makefile              |   2 +-
 lib/librte_eal/common/eal_common_devargs.c  |   6 +
 lib/librte_eal/common/eal_common_options.c  |   5 +
 lib/librte_eal/common/eal_common_soc.c      | 367 +++++++++++++++++++++++++
 lib/librte_eal/common/eal_internal_cfg.h    |   1 +
 lib/librte_eal/common/eal_options.h         |   2 +
 lib/librte_eal/common/eal_private.h         |  12 +
 lib/librte_eal/common/include/rte_devargs.h |   7 +
 lib/librte_eal/common/include/rte_soc.h     | 212 ++++++++++++++
 lib/librte_eal/linuxapp/eal/Makefile        |   3 +
 lib/librte_eal/linuxapp/eal/eal.c           |   4 +
 lib/librte_eal/linuxapp/eal/eal_soc.c       | 409 ++++++++++++++++++++++++++++
 15 files changed, 1342 insertions(+), 1 deletion(-)
 create mode 100755 app/test/soc_test_init.sh
 create mode 100644 app/test/test_soc.c
 create mode 100644 lib/librte_eal/common/eal_common_soc.c
 create mode 100644 lib/librte_eal/common/include/rte_soc.h
 create mode 100644 lib/librte_eal/linuxapp/eal/eal_soc.c

-- 
2.6.3

[dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef

[Jan Viktorin]

The eal_parse_sysfs_value function accepts a filename however, such interface
introduces race-conditions to the code. Introduce the variant of this function
that accepts an already opened file instead of a filename.

Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
---
 lib/librte_eal/common/eal_filesystem.h |  5 +++++
 lib/librte_eal/linuxapp/eal/eal.c      | 36 +++++++++++++++++++++++-----------
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/lib/librte_eal/common/eal_filesystem.h b/lib/librte_eal/common/eal_filesystem.h
index fdb4a70..7875454 100644
--- a/lib/librte_eal/common/eal_filesystem.h
+++ b/lib/librte_eal/common/eal_filesystem.h
@@ -43,6 +43,7 @@
 /** Path of rte config file. */
 #define RUNTIME_CONFIG_FMT "%s/.%s_config"
 
+#include <stdio.h>
 #include <stdint.h>
 #include <limits.h>
 #include <unistd.h>
@@ -115,4 +116,8 @@ eal_get_hugefile_temp_path(char *buffer, size_t buflen, const char *hugedir, int
  * Used to read information from files on /sys */
 int eal_parse_sysfs_value(const char *filename, unsigned long *val);
 
+/** Function to read a single numeric value from a file on the filesystem.
+ * Used to read information from files on /sys */
+int eal_parse_sysfs_valuef(FILE *f, unsigned long *val);
+
 #endif /* EAL_FILESYSTEM_H */
diff --git a/lib/librte_eal/linuxapp/eal/eal.c b/lib/librte_eal/linuxapp/eal/eal.c
index 4b28197..e8fce6b 100644
--- a/lib/librte_eal/linuxapp/eal/eal.c
+++ b/lib/librte_eal/linuxapp/eal/eal.c
@@ -126,13 +126,30 @@ rte_eal_get_configuration(void)
 	return &rte_config;
 }
 
+int
+eal_parse_sysfs_valuef(FILE *f, unsigned long *val)
+{
+	char buf[BUFSIZ];
+	char *end = NULL;
+
+	RTE_VERIFY(f != NULL);
+
+	if (fgets(buf, sizeof(buf), f) == NULL)
+		return -1;
+
+	*val = strtoul(buf, &end, 0);
+	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n'))
+		return -2;
+
+	return 0;
+}
+
 /* parse a sysfs (or other) file containing one integer value */
 int
 eal_parse_sysfs_value(const char *filename, unsigned long *val)
 {
+	int ret;
 	FILE *f;
-	char buf[BUFSIZ];
-	char *end = NULL;
 
 	if ((f = fopen(filename, "r")) == NULL) {
 		RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
@@ -140,21 +157,18 @@ eal_parse_sysfs_value(const char *filename, unsigned long *val)
 		return -1;
 	}
 
-	if (fgets(buf, sizeof(buf), f) == NULL) {
+	ret = eal_parse_sysfs_valuef(f, val);
+	if (ret == -1) {
 		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
-			__func__, filename);
-		fclose(f);
-		return -1;
+				__func__, filename);
 	}
-	*val = strtoul(buf, &end, 0);
-	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
+	else if (ret < 0) {
 		RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
 				__func__, filename);
-		fclose(f);
-		return -1;
 	}
+
 	fclose(f);
-	return 0;
+	return ret;
 }
 
 
-- 
2.8.0