DPDK patches and discussions
 help / color / mirror / Atom feed
* Re: [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef
@ 2016-06-13 14:18 Shreyansh Jain
  2016-06-13 14:24 ` Jan Viktorin
  0 siblings, 1 reply; 7+ messages in thread
From: Shreyansh Jain @ 2016-06-13 14:18 UTC (permalink / raw)
  To: Jan Viktorin
  Cc: dev, David Marchand, Thomas Monjalon, Bruce Richardson,
	Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH v1 00/28] Support non-PCI devices
  2016-01-01 21:05 ` [dpdk-dev] [RFC 0/7] " Jan Viktorin
@ 2016-05-06 13:47 Jan Viktorin
  2016-01-01 21:05 ` [dpdk-dev] [RFC 0/7] " Jan Viktorin
  0 siblings, 1 reply; 7+ messages in thread
From: Jan Viktorin @ 2016-05-06 13:47 UTC (permalink / raw)
  To: dev
  Cc: Jan Viktorin, David Marchand, Thomas Monjalon, Bruce Richardson,
	Declan Doherty, jianbo.liu, jerin.jacob, Keith Wiles,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-07-04 13:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13 14:18 [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef Shreyansh Jain
2016-06-13 14:24 ` Jan Viktorin
2016-06-14  4:30   ` Shreyansh Jain
2016-06-15  9:56     ` Jan Viktorin
2016-06-16 11:47       ` Shreyansh Jain
2016-07-04 13:24         ` Jan Viktorin
  -- strict thread matches above, loose matches on Subject: below --
2016-05-06 13:47 [dpdk-dev] [PATCH v1 00/28] Support non-PCI devices Jan Viktorin
2016-01-01 21:05 ` [dpdk-dev] [RFC 0/7] " Jan Viktorin
2016-05-06 13:47   ` [dpdk-dev] [PATCH v1 02/28] eal: extract function eal_parse_sysfs_valuef Jan Viktorin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).