Re: [ovs-dev] [RFC PATCH v2 1/1] netdev-dpdk: Add support for DPDK 16.07

2016-07-22 Thread Daniele Di Proietto 
[...]

> @@ -1776,7 +1764,8 @@ netdev_dpdk_get_stats(const struct netdev
> > *netdev, struct netdev_stats *stats)
> >  netdev_dpdk_get_carrier(netdev, );
> >  ovs_mutex_lock(>mutex);
> >
> > -struct rte_eth_xstats *rte_xstats;
> > +struct rte_eth_xstat *rte_xstats;
> > +struct rte_eth_xstat_name *rte_xstats_names;
> >  int rte_xstats_len, rte_xstats_ret;
> >
> >  if (rte_eth_stats_get(dev->port_id, _stats)) {
> > @@ -1785,20 +1774,51 @@ netdev_dpdk_get_stats(const struct netdev
> > *netdev, struct netdev_stats *stats)
> >  return EPROTO;
> >  }
> >
> > -rte_xstats_len = rte_eth_xstats_get(dev->port_id, NULL, 0);
> > -if (rte_xstats_len > 0) {
> > -rte_xstats = dpdk_rte_mzalloc(sizeof(*rte_xstats) *
> rte_xstats_len);
> > -memset(rte_xstats, 0xff, sizeof(*rte_xstats) * rte_xstats_len);
> > -rte_xstats_ret = rte_eth_xstats_get(dev->port_id, rte_xstats,
> > -rte_xstats_len);
> > -if (rte_xstats_ret > 0 && rte_xstats_ret <= rte_xstats_len) {
> > -netdev_dpdk_convert_xstats(stats, rte_xstats,
> rte_xstats_ret);
> > -}
> > +/* Get length of statistics */
> > +rte_xstats_len = rte_eth_xstats_get_names(dev->port_id, NULL, 0);
> > +if (rte_xstats_len < 0) {
> > +VLOG_WARN("Cannot get XSTATS values for port: %i",
> dev->port_id);
> > +goto out;
> > +}
> > +/* Reserve memory for xstats names */
> > +rte_xstats_names = dpdk_rte_mzalloc(sizeof(*rte_xstats_names)
> > +* rte_xstats_len);
> > +if (rte_xstats_names == NULL) {
> >
> > Minor: how about !rte_xstats_names?
> >
> > +VLOG_WARN("Cannot allocate memory for XSTATS names for port %i",
> > +  dev->port_id);
> > +rte_free(rte_xstats_names);
> >
> > This rte_free seems unnecessary
> >
> > +goto out;
> > +}
> > +/* Reserve memory for xstats values */
> > +rte_xstats = dpdk_rte_mzalloc(sizeof(*rte_xstats) * rte_xstats_len);
> > +if (rte_xstats == NULL) {
> >
> > Minor: how about !rte_xstats?
> >
> > +VLOG_WARN("Cannot allocate memory for XSTATS values for port
> %i",
> > +  dev->port_id);
> >  rte_free(rte_xstats);
> >
> > This rte_free seems unnecessary.
> >
> > +goto out;
> >
> > I think here we would leak rte_xstats_names.
> >
> > +}
> > +/* Retreive xstats names */
> > +if (rte_xstats_len != rte_eth_xstats_get_names(dev->port_id,
> > +   rte_xstats_names,
> rte_xstats_len)) {
> > +VLOG_WARN("Cannot get XSTATS names for port: %i.",
> dev->port_id);
> > +rte_free(rte_xstats);
> > +rte_free(rte_xstats_names);
> > +goto out;
> > +}
> > +/* Retreive xstats values */
> > +memset(rte_xstats, 0xff, sizeof(*rte_xstats) * rte_xstats_len);
> > +rte_xstats_ret = rte_eth_xstats_get(dev->port_id, rte_xstats,
> > +rte_xstats_len);
> > +if (rte_xstats_ret > 0 && rte_xstats_ret <= rte_xstats_len) {
> > +netdev_dpdk_convert_xstats(stats, rte_xstats, rte_xstats_names,
> > +   rte_xstats_len);
> >
> > Who frees rte_xstats_names and rte_xstats?
> >
> >  } else {
> > -VLOG_WARN("Can't get XSTATS counters for port: %i.",
> dev->port_id);
> > +VLOG_WARN("Cannot get XSTATS values for port: %i.",
> dev->port_id);
> > +rte_free(rte_xstats);
> > +rte_free(rte_xstats_names);
> >  }
> >
> > +out:
> >
> > Perhaps it's easier to always free rte_xstats and rte_xstats_names here.
> I've fixed this in the v3 and moved the free as suggested.
>
> > Also, is there a reason not to use xcalloc() and free(), instead?
> > This is not the fastpath, as far as I understand there's no reason for
> it to be in
> > hugepages.
> Just following what was already there. But good point - I've changed this
> to use xcalloc and free.
>
>
Ok, thanks


> >
> >  stats->rx_packets = rte_stats.ipackets;
> >  stats->tx_packets = rte_stats.opackets;
> >  stats->rx_bytes = rte_stats.ibytes;
>

[...]


> > @@ -2233,26 +2250,27 @@ netdev_dpdk_remap_txqs(struct netdev_dpdk
> > *dev)
> >   * A new virtio-net device is added to a vhost port.
> >   */
> >  static int
> > -new_device(struct virtio_net *virtio_dev)
> > +new_device(int vid)
> >  {
> >  struct netdev_dpdk *dev;
> >  bool exists = false;
> >  int newnode = 0;
> > -long err = 0;
> > +char ifname[PATH_MAX];
> > +
> > +rte_vhost_get_ifname(vid, ifname, sizeof(ifname));
> >
> >  ovs_mutex_lock(_mutex);
> >  /* Add device to the vhost port with the same name as that passed
> down.
> > */
> >  LIST_FOR_EACH(dev, list_node, _list) {
> > -if (strncmp(virtio_dev->ifname, dev->vhost_id, IF_NAME_SZ) ==
> 0) {
> > -uint32_t qp_num = virtio_dev->virt_qp_nb;
> > +if

Re: [ovs-dev] [RFC PATCH v2 1/1] netdev-dpdk: Add support for DPDK 16.07

2016-07-22 Thread Loftus, Ciara 
> 
> Hi Ciara,
> thanks for the patch.
> It mostly looks good to me, except a few comments inline
> Thanks,
> Daniele

Thanks for the review Daniele. I've pushed a new version that includes your 
suggestions. Responses inline.

> 
> 2016-07-12 2:11 GMT-07:00 Ciara Loftus :
> This commit introduces support for DPDK 16.07 and consequently breaks
> compatibility with DPDK 16.04.
> 
> DPDK 16.07 introduces some changes to various APIs. These have been
> updated in OVS, including:
> * xstats API: changes to structure of xstats
> * vhost API:  replace virtio-net references with 'vid'
> 
> Signed-off-by: Ciara Loftus 
> 
> ---
>  .travis/linux-build.sh   |   2 +-
>  INSTALL.DPDK-ADVANCED.md |   8 +-
>  INSTALL.DPDK.md          |  20 ++--
>  lib/netdev-dpdk.c        | 243 +++---
> -
>  4 files changed, 135 insertions(+), 138 deletions(-)
> 
> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> index 065de39..1b3d43d 100755
> --- a/.travis/linux-build.sh
> +++ b/.travis/linux-build.sh
> @@ -68,7 +68,7 @@ fi
> 
>  if [ "$DPDK" ]; then
>      if [ -z "$DPDK_VER" ]; then
> -        DPDK_VER="16.04"
> +        DPDK_VER="16.07"
> 
I wanted to test it on travis, but the files are not there yet :)
> 
>      fi
>      install_dpdk $DPDK_VER
>      if [ "$CC" = "clang" ]; then
> diff --git a/INSTALL.DPDK-ADVANCED.md b/INSTALL.DPDK-ADVANCED.md
> index 9ae536d..ec1de29 100644
> --- a/INSTALL.DPDK-ADVANCED.md
> +++ b/INSTALL.DPDK-ADVANCED.md
> @@ -43,7 +43,7 @@ for DPDK and OVS.
>      For IVSHMEM case, set `export DPDK_TARGET=x86_64-ivshmem-
> linuxapp-gcc`
> 
>      ```
> -    export DPDK_DIR=/usr/src/dpdk-16.04
> +    export DPDK_DIR=/usr/src/dpdk-16.07
>      export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>      make install T=$DPDK_TARGET DESTDIR=install
>      ```
> @@ -339,7 +339,7 @@ For users wanting to do packet forwarding using
> kernel stack below are the steps
>         cd /usr/src/cmdline_generator
>         wget https://raw.githubusercontent.com/netgroup-polito/un-
> orchestrator/master/orchestrator/compute_controller/plugins/kvm-
> libvirt/cmdline_generator/cmdline_generator.c
>         wget https://raw.githubusercontent.com/netgroup-polito/un-
> orchestrator/master/orchestrator/compute_controller/plugins/kvm-
> libvirt/cmdline_generator/Makefile
> -       export RTE_SDK=/usr/src/dpdk-16.04
> +       export RTE_SDK=/usr/src/dpdk-16.07
>         export RTE_TARGET=x86_64-ivshmem-linuxapp-gcc
>         make
>         ./build/cmdline_generator -m -p dpdkr0 XXX
> @@ -363,7 +363,7 @@ For users wanting to do packet forwarding using
> kernel stack below are the steps
>         mount -t hugetlbfs nodev /dev/hugepages (if not already mounted)
> 
>         # Build the DPDK ring application in the VM
> -       export RTE_SDK=/root/dpdk-16.04
> +       export RTE_SDK=/root/dpdk-16.07
>         export RTE_TARGET=x86_64-ivshmem-linuxapp-gcc
>         make
> 
> @@ -374,7 +374,7 @@ For users wanting to do packet forwarding using
> kernel stack below are the steps
> 
>  ##  6. Vhost Walkthrough
> 
> -DPDK 16.04 supports two types of vhost:
> +DPDK 16.07 supports two types of vhost:
> 
>  1. vhost-user - enabled default
> 
> diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
> index 5407794..9022ad8 100644
> --- a/INSTALL.DPDK.md
> +++ b/INSTALL.DPDK.md
> @@ -21,7 +21,7 @@ The DPDK support of Open vSwitch is considered
> 'experimental'.
> 
>  ### Prerequisites
> 
> -* Required: DPDK 16.04, libnuma
> +* Required: DPDK 16.07, libnuma
>  * Hardware: [DPDK Supported NICs] when physical ports in use
> 
>  ##  2. Building and Installation
> @@ -42,10 +42,10 @@ advanced install guide [INSTALL.DPDK-
> ADVANCED.md]
> 
>       ```
>       cd /usr/src/
> -     wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.04.zip
> -     unzip dpdk-16.04.zip
> +     wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.07.zip
> +     unzip dpdk-16.07.zip
> 
> -     export DPDK_DIR=/usr/src/dpdk-16.04
> +     export DPDK_DIR=/usr/src/dpdk-16.07
>       cd $DPDK_DIR
>       ```
> 
> @@ -329,9 +329,9 @@ can be found in [Vhost Walkthrough].
> 
>    ```
>    cd /root/dpdk/
> -  wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.04.zip
> -  unzip dpdk-16.04.zip
> -  export DPDK_DIR=/root/dpdk/dpdk-16.04
> +  wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.07.zip
> +  unzip dpdk-16.07.zip
> +  export DPDK_DIR=/root/dpdk/dpdk-16.07
>    export DPDK_TARGET=x86_64-native-linuxapp-gcc
>    export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>    cd $DPDK_DIR
> @@ -487,7 +487,7 @@ can be found in [Vhost Walkthrough].
>             
>             
>               
> -             
> +             
>               
>               
>             
> @@ -557,9 +557,9 @@ can be found in [Vhost Walkthrough].
>      DPDK. It is recommended that users update Network Interface firmware
> to
>      match what has been validated for the DPDK release.
> 
> -    For DPDK 16.04, the list of validated

Re: [ovs-dev] [RFC PATCH v2 1/1] netdev-dpdk: Add support for DPDK 16.07

2016-07-15 Thread Daniele Di Proietto 
Hi Ciara,

thanks for the patch.

It mostly looks good to me, except a few comments inline

Thanks,

Daniele

2016-07-12 2:11 GMT-07:00 Ciara Loftus :

> This commit introduces support for DPDK 16.07 and consequently breaks
> compatibility with DPDK 16.04.
>
> DPDK 16.07 introduces some changes to various APIs. These have been
> updated in OVS, including:
> * xstats API: changes to structure of xstats
> * vhost API:  replace virtio-net references with 'vid'
>
> Signed-off-by: Ciara Loftus 
>
> ---
>  .travis/linux-build.sh   |   2 +-
>  INSTALL.DPDK-ADVANCED.md |   8 +-
>  INSTALL.DPDK.md  |  20 ++--
>  lib/netdev-dpdk.c| 243
> +++
>  4 files changed, 135 insertions(+), 138 deletions(-)
>
> diff --git a/.travis/linux-build.sh b/.travis/linux-build.sh
> index 065de39..1b3d43d 100755
> --- a/.travis/linux-build.sh
> +++ b/.travis/linux-build.sh
> @@ -68,7 +68,7 @@ fi
>
>  if [ "$DPDK" ]; then
>  if [ -z "$DPDK_VER" ]; then
> -DPDK_VER="16.04"
> +DPDK_VER="16.07"
>

I wanted to test it on travis, but the files are not there yet :)


>  fi
>  install_dpdk $DPDK_VER
>  if [ "$CC" = "clang" ]; then
> diff --git a/INSTALL.DPDK-ADVANCED.md b/INSTALL.DPDK-ADVANCED.md
> index 9ae536d..ec1de29 100644
> --- a/INSTALL.DPDK-ADVANCED.md
> +++ b/INSTALL.DPDK-ADVANCED.md
> @@ -43,7 +43,7 @@ for DPDK and OVS.
>  For IVSHMEM case, set `export DPDK_TARGET=x86_64-ivshmem-linuxapp-gcc`
>
>  ```
> -export DPDK_DIR=/usr/src/dpdk-16.04
> +export DPDK_DIR=/usr/src/dpdk-16.07
>  export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>  make install T=$DPDK_TARGET DESTDIR=install
>  ```
> @@ -339,7 +339,7 @@ For users wanting to do packet forwarding using kernel
> stack below are the steps
> cd /usr/src/cmdline_generator
> wget
> https://raw.githubusercontent.com/netgroup-polito/un-orchestrator/master/orchestrator/compute_controller/plugins/kvm-libvirt/cmdline_generator/cmdline_generator.c
> wget
> https://raw.githubusercontent.com/netgroup-polito/un-orchestrator/master/orchestrator/compute_controller/plugins/kvm-libvirt/cmdline_generator/Makefile
> -   export RTE_SDK=/usr/src/dpdk-16.04
> +   export RTE_SDK=/usr/src/dpdk-16.07
> export RTE_TARGET=x86_64-ivshmem-linuxapp-gcc
> make
> ./build/cmdline_generator -m -p dpdkr0 XXX
> @@ -363,7 +363,7 @@ For users wanting to do packet forwarding using kernel
> stack below are the steps
> mount -t hugetlbfs nodev /dev/hugepages (if not already mounted)
>
> # Build the DPDK ring application in the VM
> -   export RTE_SDK=/root/dpdk-16.04
> +   export RTE_SDK=/root/dpdk-16.07
> export RTE_TARGET=x86_64-ivshmem-linuxapp-gcc
> make
>
> @@ -374,7 +374,7 @@ For users wanting to do packet forwarding using kernel
> stack below are the steps
>
>  ##  6. Vhost Walkthrough
>
> -DPDK 16.04 supports two types of vhost:
> +DPDK 16.07 supports two types of vhost:
>
>  1. vhost-user - enabled default
>
> diff --git a/INSTALL.DPDK.md b/INSTALL.DPDK.md
> index 5407794..9022ad8 100644
> --- a/INSTALL.DPDK.md
> +++ b/INSTALL.DPDK.md
> @@ -21,7 +21,7 @@ The DPDK support of Open vSwitch is considered
> 'experimental'.
>
>  ### Prerequisites
>
> -* Required: DPDK 16.04, libnuma
> +* Required: DPDK 16.07, libnuma
>  * Hardware: [DPDK Supported NICs] when physical ports in use
>
>  ##  2. Building and Installation
> @@ -42,10 +42,10 @@ advanced install guide [INSTALL.DPDK-ADVANCED.md]
>
>   ```
>   cd /usr/src/
> - wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.04.zip
> - unzip dpdk-16.04.zip
> + wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.07.zip
> + unzip dpdk-16.07.zip
>
> - export DPDK_DIR=/usr/src/dpdk-16.04
> + export DPDK_DIR=/usr/src/dpdk-16.07
>   cd $DPDK_DIR
>   ```
>
> @@ -329,9 +329,9 @@ can be found in [Vhost Walkthrough].
>
>```
>cd /root/dpdk/
> -  wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.04.zip
> -  unzip dpdk-16.04.zip
> -  export DPDK_DIR=/root/dpdk/dpdk-16.04
> +  wget http://dpdk.org/browse/dpdk/snapshot/dpdk-16.07.zip
> +  unzip dpdk-16.07.zip
> +  export DPDK_DIR=/root/dpdk/dpdk-16.07
>export DPDK_TARGET=x86_64-native-linuxapp-gcc
>export DPDK_BUILD=$DPDK_DIR/$DPDK_TARGET
>cd $DPDK_DIR
> @@ -487,7 +487,7 @@ can be found in [Vhost Walkthrough].
> 
> 
>   
> - 
> + 
>   
>   
> 
> @@ -557,9 +557,9 @@ can be found in [Vhost Walkthrough].
>  DPDK. It is recommended that users update Network Interface firmware
> to
>  match what has been validated for the DPDK release.
>
> -For DPDK 16.04, the list of validated firmware versions can be found
> at:
> +For DPDK 16.07, the list of validated firmware versions can be found
> at:
>
> -