* Re: [PATCH v12 0/6] add Tx preparation
From: Yuanhan Liu @ 2016-12-07 10:18 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Yigit, Ferruh, Olivier Matz, Thomas Monjalon, dev@dpdk.org,
Jan Medala, Jakub Palider, Netanel Belgazal, Evgeny Schemeilin,
Alejandro Lucero, Yong Wang, Andrew Rybchenko, Hemant Agrawal,
Kulasek, TomaszX
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E4E4B@irsmsx105.ger.corp.intel.com>
On Wed, Dec 07, 2016 at 10:13:14AM +0000, Ananyev, Konstantin wrote:
>
> Hi Yliu,
>
> >
> > On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > > > Please, we need a comment for each driver saying
> > > > "it is OK, we do not need any checksum preparation for TSO"
> > > > or
> > > > "yes we have to implement tx_prepare or TSO will not work in this mode"
> > > >
> >
> > Sorry for late. For virtio, I think it's not a must. The checksum stuff
> > has been handled inside the Tx function. However, we may could move it
> > to tx_prepare, which would actually recover the performance lost
> > introduced while enabling TSO for the non-TSO case.
> >
>
> So would you like to provide a patch for it,
> Or would you like to keep tx_prepare() for virtio as NOP for now?
Hi Konstantin,
I'd keep it as it is for now. It should be a trivial patch after all, that
I could provide it when everything are settled down.
--yliu
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-12-07 10:13 UTC (permalink / raw)
To: Yuanhan Liu, Yigit, Ferruh, Olivier Matz
Cc: Thomas Monjalon, dev@dpdk.org, Jan Medala, Jakub Palider,
Netanel Belgazal, Evgeny Schemeilin, Alejandro Lucero, Yong Wang,
Andrew Rybchenko, Hemant Agrawal, Kulasek, TomaszX
In-Reply-To: <20161207081144.GC31182@yliu-dev.sh.intel.com>
Hi Yliu,
>
> On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > > Please, we need a comment for each driver saying
> > > "it is OK, we do not need any checksum preparation for TSO"
> > > or
> > > "yes we have to implement tx_prepare or TSO will not work in this mode"
> > >
>
> Sorry for late. For virtio, I think it's not a must. The checksum stuff
> has been handled inside the Tx function. However, we may could move it
> to tx_prepare, which would actually recover the performance lost
> introduced while enabling TSO for the non-TSO case.
>
So would you like to provide a patch for it,
Or would you like to keep tx_prepare() for virtio as NOP for now?
Thanks
Konstantin
^ permalink raw reply
* Re: [PATCH] vhost: allow for many vhost user ports
From: Yuanhan Liu @ 2016-12-07 10:12 UTC (permalink / raw)
To: Jan Wickbom; +Cc: dev, patrik.r.andersson
In-Reply-To: <1480606010-6132-1-git-send-email-jan.wickbom@ericsson.com>
On Thu, Dec 01, 2016 at 04:26:50PM +0100, Jan Wickbom wrote:
> static int
> -fdset_fill(fd_set *rfset, fd_set *wfset, struct fdset *pfdset)
> +fdset_fill(struct pollfd *rwfds, struct fdset *pfdset)
> {
> struct fdentry *pfdentry;
> - int i, maxfds = -1;
> - int num = MAX_FDS;
> -
> - if (pfdset == NULL)
> - return -1;
> + int i;
> + int num;
>
> - for (i = 0; i < num; i++) {
> + for (i = 0, num = pfdset->num; i < num; i++) {
> pfdentry = &pfdset->fd[i];
> - if (pfdentry->fd != -1) {
> - int added = 0;
> - if (pfdentry->rcb && rfset) {
> - FD_SET(pfdentry->fd, rfset);
> - added = 1;
> - }
> - if (pfdentry->wcb && wfset) {
> - FD_SET(pfdentry->fd, wfset);
> - added = 1;
> - }
> - if (added)
> - maxfds = pfdentry->fd < maxfds ?
> - maxfds : pfdentry->fd;
> +
> + if (pfdentry->fd < 0) {
> + /* Hole in the list. Move the last one here */
> +
> + *pfdentry = pfdset->fd[num - 1];
> + pfdset->fd[num - 1].fd = -1;
> + num = fdset_adjust_num(pfdset);
> }
> + rwfds[i].fd = pfdentry->fd;
> + rwfds[i].events = pfdentry->rcb ? POLLIN : 0;
> + rwfds[i].events |= pfdentry->wcb ? POLLOUT : 0;
Another thing is we don't have to re-init this rwfds array again
and again. Instead, we could
- set it up correctly when fdset_add is invoked: set the fd and
events.
- reset revents when it's been handled at fdset_event_dispatch().
- swap with the last one and shrink the array on fd delete
Could you make a follow up patch for that?
Thanks.
--yliu
^ permalink raw reply
* Re: [PATCH 10/32] net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
From: Shreyansh Jain @ 2016-12-07 10:13 UTC (permalink / raw)
To: Hemant Agrawal, dev
Cc: thomas.monjalon, bruce.richardson, david.marchand@6wind.com
In-Reply-To: <1480875447-23680-11-git-send-email-hemant.agrawal@nxp.com>
+ CC: David Marchand
On Sunday 04 December 2016 11:47 PM, Hemant Agrawal wrote:
> The DPAA2 bus driver is a rte_bus driver which scans the fsl-mc bus.
>
> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> ---
> drivers/net/Makefile | 2 +-
> drivers/net/dpaa2/Makefile | 60 ++++++++++++++
> drivers/net/dpaa2/dpaa2_bus.c | 99 +++++++++++++++++++++++
> drivers/net/dpaa2/rte_dpaa2.h | 121 ++++++++++++++++++++++++++++
> drivers/net/dpaa2/rte_pmd_dpaa2_version.map | 4 +
> mk/rte.app.mk | 1 +
> 6 files changed, 286 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/dpaa2/Makefile
> create mode 100644 drivers/net/dpaa2/dpaa2_bus.c
> create mode 100644 drivers/net/dpaa2/rte_dpaa2.h
> create mode 100644 drivers/net/dpaa2/rte_pmd_dpaa2_version.map
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index bc93230..2bcf67b 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -55,7 +55,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
> DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
> DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
> -
> +DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2
> ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
> DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
> endif # $(CONFIG_RTE_LIBRTE_VHOST)
> diff --git a/drivers/net/dpaa2/Makefile b/drivers/net/dpaa2/Makefile
> new file mode 100644
> index 0000000..a99ce22
> --- /dev/null
> +++ b/drivers/net/dpaa2/Makefile
> @@ -0,0 +1,60 @@
> +# BSD LICENSE
> +#
> +# Copyright (c) 2016 NXP. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in
> +# the documentation and/or other materials provided with the
> +# distribution.
> +# * Neither the name of NXP nor the names of its
> +# contributors may be used to endorse or promote products derived
> +# from this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name
> +#
> +LIB = librte_pmd_dpaa2.a
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +
> +CFLAGS += -I$(RTE_SDK)/drivers/net/dpaa2
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/common/
> +CFLAGS += -I$(RTE_SDK)/lib/librte_eal/linuxapp/eal
> +
> +# versioning export map
> +EXPORT_MAP := rte_pmd_dpaa2_version.map
> +
> +# library version
> +LIBABIVER := 1
> +
> +
> +# Interfaces with DPDK
> +SRCS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2_bus.c
> +
> +# library dependencies
> +DEPDIRS-y += lib/librte_eal
> +DEPDIRS-y += drivers/common/dpaa/mc
> +DEPDIRS-y += drivers/common/dpaa/qbman
> +
> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/net/dpaa2/dpaa2_bus.c b/drivers/net/dpaa2/dpaa2_bus.c
> new file mode 100644
> index 0000000..571066c
> --- /dev/null
> +++ b/drivers/net/dpaa2/dpaa2_bus.c
> @@ -0,0 +1,99 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright (c) 2016 NXP. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of NXP nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <string.h>
> +#include <dirent.h>
> +
> +#include <rte_log.h>
> +#include <rte_bus.h>
> +#include <rte_dpaa2.h>
> +#include <rte_eal_memconfig.h>
> +#include <rte_malloc.h>
> +#include <rte_devargs.h>
> +#include <rte_memcpy.h>
> +#include <rte_ethdev.h>
> +
> +#include "eal_filesystem.h"
> +#include "eal_private.h"
> +
> +void
> +rte_dpaa2_register(struct rte_dpaa2_driver *driver)
> +{
> + struct rte_bus *bus;
> +
> + bus = rte_eal_get_bus("dpaa2");
> + if (!bus) {
> + RTE_LOG(ERR, EAL, "DPAA2 bus not registered\n");
> + return;
> + }
> +
> + rte_eal_bus_add_driver(bus, &driver->driver);
> +}
> +
> +void
> +rte_dpaa2_unregister(struct rte_dpaa2_driver *driver)
> +{
> + struct rte_bus *bus;
> +
> + bus = driver->driver.bus;
> + if (!bus) {
> + RTE_LOG(ERR, EAL, "Unable to find bus for device\n");
> + return;
> + }
> +
> + rte_eal_bus_remove_driver(&driver->driver);
> +}
> +
> +int rte_dpaa2_probe(struct rte_driver *driver __rte_unused,
> + struct rte_device *device __rte_unused)
> +{
> + return 0;
> +}
> +
> +int rte_dpaa2_scan(struct rte_bus *bus_d __rte_unused)
> +{
> + return 0;
> +}
> +
> +int rte_dpaa2_match(struct rte_driver *driver __rte_unused,
> + struct rte_device *device __rte_unused)
> +{
> + return 0;
> +}
> +
> +struct rte_bus dpaa2_bus = {
> + .scan = rte_dpaa2_scan,
> + .match = rte_dpaa2_match,
> + .probe = rte_dpaa2_probe,
> +};
> +
> +RTE_REGISTER_BUS(dpaa2, dpaa2_bus);
> diff --git a/drivers/net/dpaa2/rte_dpaa2.h b/drivers/net/dpaa2/rte_dpaa2.h
> new file mode 100644
> index 0000000..b36eed8
> --- /dev/null
> +++ b/drivers/net/dpaa2/rte_dpaa2.h
> @@ -0,0 +1,121 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright (c) 2016 NXP. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of NXP nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef _RTE_DPAA2_H_
> +#define _RTE_DPAA2_H_
> +
> +/**
> + * @file
> + *
> + * RTE DPAA2 Interface
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/queue.h>
> +#include <stdint.h>
> +#include <inttypes.h>
> +
> +#include <rte_debug.h>
> +#include <rte_interrupts.h>
> +#include <rte_dev.h>
> +
> +
> +struct rte_dpaa2_driver;
> +/**
> + * A structure describing a DPAA2 device.
> + */
> +struct rte_dpaa2_device {
> + TAILQ_ENTRY(rte_dpaa2_device) next; /**< Next probed DPAA2 device. */
> + struct rte_device device; /**< Inherit core device */
> + uint16_t dev_type; /**< Device Type */
> + uint16_t object_id; /**< DPAA2 Object ID */
> + struct rte_intr_handle intr_handle; /**< Interrupt handle */
> + struct rte_dpaa2_driver *driver; /**< Associated driver */
> +};
> +
> +/**
> + * A structure describing a DPAA2 driver.
> + */
> +struct rte_dpaa2_driver {
> + TAILQ_ENTRY(rte_dpaa2_driver) next; /**< Next in list. */
> + struct rte_driver driver; /**< Inherit core driver. */
> + uint32_t drv_flags; /**< Flags contolling handling of device. */
> +};
> +
> +/**
> + * Register a DPAA2 driver.
> + *
> + * @param driver
> + * A pointer to a rte_dpaa2_driver structure describing the driver
> + * to be registered.
> + */
> +void rte_dpaa2_register(struct rte_dpaa2_driver *driver);
> +
> +/**
> + * Unregister a DPAA2 driver.
> + *
> + * @param driver
> + * A pointer to a rte_dpaa2_driver structure describing the driver
> + * to be unregistered.
> + */
> +void rte_dpaa2_unregister(struct rte_dpaa2_driver *driver);
> +
> +/**
> + *
> + */
> +int rte_dpaa2_probe(struct rte_driver *driver, struct rte_device *device);
> +int rte_dpaa2_match(struct rte_driver *driver, struct rte_device *device);
> +int rte_dpaa2_scan(struct rte_bus *bus);
> +
> +/** Helper for DPAA2 device registration from driver (eth, crypto) instance */
> +#define RTE_PMD_REGISTER_DPAA2(nm, dpaa2_drv) \
> +RTE_INIT(dpaa2initfn_ ##nm); \
> +static void dpaa2initfn_ ##nm(void) \
> +{\
> + (dpaa2_drv).driver.name = RTE_STR(nm);\
> + rte_dpaa2_register(&dpaa2_drv); \
> +} \
> +RTE_PMD_EXPORT_NAME(nm, __COUNTER__)
> +
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_DPAA2_H_ */
> diff --git a/drivers/net/dpaa2/rte_pmd_dpaa2_version.map b/drivers/net/dpaa2/rte_pmd_dpaa2_version.map
> new file mode 100644
> index 0000000..31eca32
> --- /dev/null
> +++ b/drivers/net/dpaa2/rte_pmd_dpaa2_version.map
> @@ -0,0 +1,4 @@
> +DPDK_17.02 {
> +
> + local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index f75f0e2..9e1c17c 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -101,6 +101,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE) += -lrte_cfgfile
>
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BOND) += -lrte_pmd_bond
> _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += -lrte_pmd_xenvirt -lxenstore
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += -lrte_pmd_dpaa2 -ldpaa2_mc -ldpaa2_qbman
>
> ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),n)
> # plugins (link only if static libraries)
>
IMO, the way Bus is kept is debatable.
- should it be in EAL (lib/librte_eal/linuxapp/eal_pci.c like Bus
patches) [1]?
- Should it a 'handler/driver' parallel to device drivers?
I personally prefer a clean layer for buses with:
- RTE_SDK/drivers/net/dpaa2/
- RTE_SDK/drivers/bus
- RTE_SDK/drivers/bus/dpaa2/
- RTE_SDK/drivers/bus/dpaa2/dpaa2_bus.c etc.
For PCI, which is generic (or for other similar generic buses, like
platform), we can keep the implementation within lib/librte_eal/linuxapp/*.
What do other think about this?
[1] http://dpdk.org/ml/archives/dev/2016-December/051359.html
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Adrien Mazarguil @ 2016-12-07 10:08 UTC (permalink / raw)
To: Ananyev, Konstantin
Cc: Thomas Monjalon, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Alejandro Lucero,
Harish Patil, Rasesh Mody, Jerin Jacob, Yuanhan Liu, Yong Wang,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <2601191342CEEE43887BDE71AB9772583F0E4C13@irsmsx105.ger.corp.intel.com>
On Tue, Dec 06, 2016 at 08:31:35PM +0000, Ananyev, Konstantin wrote:
> > Hi Konstantin,
> >
> > On Tue, Dec 06, 2016 at 10:56:26AM +0000, Ananyev, Konstantin wrote:
> > >
> > > Hi Adrien,
> > >
> > > >
> > > > On Mon, Dec 05, 2016 at 04:43:52PM +0000, Ananyev, Konstantin wrote:
> > > > [...]
> > > > > > On Fri, Dec 02, 2016 at 01:00:55AM +0000, Ananyev, Konstantin wrote:
> > > > > > [...]
> > > > > > > > On Wed, Nov 30, 2016 at 10:54:50AM +0000, Ananyev, Konstantin wrote:
> > > > > > > > [...]
> > > > > > > > > Do you have anything particular in mind here?
> > > > > > > >
> > > > > > > > Nothing in particular, so for the sake of the argument, let's suppose that I
> > > > > > > > would like to add a field to expose some limitation that only applies to my
> > > > > > > > PMD during TX but looks generic enough to make sense, e.g. maximum packet
> > > > > > > > size when VLAN tagging is requested.
> > > > > > >
> > > > > > > Hmm, I didn't hear about such limitations so far, but if it is real case -
> > > > > > > sure, feel free to submit the patch.
> > > > > >
> > > > > > I won't, that was hypothetical.
> > > > >
> > > > > Then why we discussing it? :)
> > > >
> > > > Just to make a point, which is that new limitations may appear anytime and
> > > > tx_prepare() can now be used to check for them. First patch of the series
> > > > does it:
> > > >
> > > > + uint16_t nb_seg_max; /**< Max number of segments per whole packet. */
> > > > + uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
> > > >
> > > > And states that:
> > > >
> > > > + * For each packet to send, the rte_eth_tx_prepare() function performs
> > > > + * the following operations:
> > > > + *
> > > > + * - Check if packet meets devices requirements for tx offloads.
> > > > + *
> > > > + * - Check limitations about number of segments.
> > > > + *
> > > > + * - Check additional requirements when debug is enabled.
> > > > + *
> > > > + * - Update and/or reset required checksums when tx offload is set for packet.
> > >
> > > I think I already explained in my previous email why I think that
> > > nb_seg_max and nb_mtu_seg_max are not redundant because of tx_prepare().
> > > From my point they are complement to tx_prepare():
> > > Even if people do use tx_prepare() they still should take this information into account.
> > > As an example ixgbe can't TX packets with then 40 segments.
> > > tx_prepare() for ixgbe will flag that issue, but it can't make a decision on user behalf
> > > what to do in that case: drop the packet, try to coalesce it into the packet with less
> > > number of segments, split the packet into several smaller, etc.
> > > That's up to user to make such decision, and to make it, user might need this information.
> >
> > Yet tx_prepare() has already the ability to update mbuf contents, issue is
> > what will this function do in the future, where will it stop? It is defined
> > in a way that each PMD does what it wants to make mbufs edible for
> > tx_burst(), because of this applications will just always call it to be on
> > the safe side.
> >
> > > > It's like making this function mandatory IMO.
> > >
> > > That's probably where confusion starts: I don't think that
> > > tx_prepare() should be mandatory for the user to call.
> > > Yes, it should be a recommended way.
> > > But the user still should have the ability to by-pass it,
> > > if he believes there is no need for it, or he prefers to implement
> > > the same functionality on his own.
> > > As an example, if the user knows that he is going to send a group
> > > of one-segment packets that don't require any tx offloads, he can safely skip
> > > tx_prepare() for them.
> >
> > I understand your point, and agree with the example you provide. Many
> > applications do not know what's inside mbufs though, except perhaps that
> > they contain TCP and may want to perform TSO because of that. Those will
> > have to call tx_prepare() to be future-proof.
> >
> > > > > > > > PMDs are free to set that field to some
> > > > > > > > special value (say, 0) if they do not care.
> > > > > > > >
> > > > > > > > Since that field exists however, conscious applications should check its
> > > > > > > > value for each packet that needs to be transmitted. This extra code causes a
> > > > > > > > slowdown just by sitting in the data path. Since it is not the only field in
> > > > > > > > that structure, the performance impact can be significant.
> > >
> > > Conscious user will probably use this information at the stage of packet formation.
> > > He probably has to do this sort of things for large packets anyway:
> > > Check what is the underlying mtu, to decide does he need to split the packet,
> > > or enable tso for it, etc.
> >
> > There are already too many things to check,
>
> There always been, that patch exposes them, before that upper layer probably
> had to use some hard-coded defines.
>
> > applications probably won't mind
> > a little help from PMDs. If we keep adding fields to this structure, we'll
> > have to provide some sort of PMD-specific function that checks what is
> > relevant.
>
> Why PMD specific?
> These fields are generic enough and could be used by upper layer to consult
> when packet is formed.
I've used the wrong term here, I meant a generic function provided by PMDs,
just like tx_prepare(). Some sort of generic tx_check(), whose functionality
is currently partially covered by tx_prepare() if I'm not mistaken.
> > Furthermore, assuming most packets are fine and do not require any extra
> > processing, what is rejected by tx_burst() could enter some unlikely() path
> > that attempts to rectify and re-send them. That would at least optimize the
> > common scenario.
>
> It is up to the upper layer to decide what to do with ill-formed packets:
> drop/log/try to cure/etc.
> Obviously different applications would have different logic and make different decisions here.
> If you'd like to introduce a new function (in rte_net or whatever) that would be smart and
> generic enough to cure ill-formed packets - you are more than welcome to try.
> Though discussion of such fallback function is far of scope of that patch, I believe.
I agree it's up to the upper layer, so just to clarify, I meant the
tx_burst() function could fail some check and not transmit the remaining
buffers, leaving the application to do whatever with them in some unlikely()
path:
// tx_prepare() has never been called
n = tx_burst(pkts, num);
if (unlikely(n < num)) {
n += recover_here(pkts + n, num - n);
}
> > > > > > > > Even though this code is inside applications, it remains unfair to PMDs for
> > > > > > > > which these tests are irrelevant. This problem is identified and addressed
> > > > > > > > by tx_prepare().
> > > > > > >
> > > > > > > I suppose the question is why do we need:
> > > > > > > uint16_t nb_seg_max;
> > > > > > > uint16_t nb_mtu_seg_max;
> > > > > > > as we now have tx_prepare(), right?
> > > > > > >
> > > > > > > For two reasons:
> > > > > > > 1. Some people might feel that tx_prepare() is not good (smart/fast) enough
> > > > > > > for them and would prefer to do necessary preparations for TX offloads themselves.
> > > > > > >
> > > > > > > 2. Even if people do use tx_prepare() they still should take this information into accout.
> > > > > > > As an example ixgbe can't TX packets with then 40 segments.
> > > > > > > Obviously ixbge_tx_prep() performs that check and returns an error.
> > > > > >
> > > > > > Problem is that tx_prepare() also provides safeties which are not part of
> > > > > > tx_burst(), such as not going over nb_mtu_seg_max. Because of this and the
> > > > > > fact struct rte_eth_desc_lim can grow new fields anytime, application
> > > > > > developers will be tempted to just call tx_prepare() and focus on more
> > > > > > useful things.
> > > > >
> > > > > NP with that, that was an intention beyond introducing it.
> > > > >
> > > > > > Put another way, from a user's point of view, tx_prepare() is an opaque
> > > > > > function that greatly increases tx_burst()'s ability to send mbufs as
> > > > > > requested, with extra error checking on top; applications not written to run
> > > > > > on a specific PMD/device (all of them ideally) will thus call tx_prepare()
> > > > > > at some point.
> > > > > >
> > > > > > > But it wouldn't try to merge/reallocate mbufs for you.
> > > > > > > User still has to do it himself, or just prevent creating such long chains somehow.
> > > > > >
> > > > > > Yes, that's another debate. PMDs could still implement a software fallback
> > > > > > for unlikely slow events like these. The number of PMDs is not going to
> > > > > > decrease, each device having its own set of weird limitations in specific
> > > > > > cases, PMDs should do their best to process mbufs even if that means slowly
> > > > > > due to the lack of preparation.
> > > > > >
> > > > > > tx_prepare() has its uses but should really be optional, in the sense that
> > > > > > if that function is not called, tx_burst() should deal with it somehow.
> > > > >
> > > > > As I said before, I don't think it is a good idea to put everything in tx_burst().
> > > > > If PMD driver prefer things that way, yes tx_burst() can deal with each and
> > > > > possible offload requirement itself, but it shouldn't be mandatory.
> > > >
> > > > In effect, having to call tx_prepare() otherwise makes this step mandatory
> > > > anyway. Looks like we are not going to agree here.
> > > >
> > > > > > > > Thanks to tx_prepare(), these checks are moved back into PMDs where they
> > > > > > > > belong. PMDs that do not need them do not have to provide support for
> > > > > > > > tx_prepare() and do not suffer any performance impact as result;
> > > > > > > > applications only have to make sure tx_prepare() is always called at some
> > > > > > > > point before tx_burst().
> > > > > > > >
> > > > > > > > Once you reach this stage, you've effectively made tx_prepare() mandatory
> > > > > > > > before tx_burst(). If some bug occurs, then perhaps you forgot to call
> > > > > > > > tx_prepare(), you just need to add it. The total cost for doing TX is
> > > > > > > > therefore tx_prepare() + tx_burst().
> > > > > > > >
> > > > > > > > I'm perhaps a bit pessimistic mind you, but I do not think tx_prepare() will
> > > > > > > > remain optional for long. Sure, PMDs that do not implement it do not care,
> > > > > > > > I'm focusing on applications, for which the performance impact of calling
> > > > > > > > tx_prepare() followed by tx_burst() is higher than a single tx_burst()
> > > > > > > > performing all the necessary preparation at once.
> > > > > > > >
> > > > > > > > [...]
> > > > > > > > > > Following the same logic, why can't such a thing be made part of the TX
> > > > > > > > > > burst function as well (through a direct call to rte_phdr_cksum_fix()
> > > > > > > > > > whenever necessary). From an application standpoint, what are the advantages
> > > > > > > > > > of having to:
> > > > > > > > > >
> > > > > > > > > > if (tx_prep()) // iterate and update mbufs as needed
> > > > > > > > > > tx_burst(); // iterate and send
> > > > > > > > > >
> > > > > > > > > > Compared to:
> > > > > > > > > >
> > > > > > > > > > tx_burst(); // iterate, update as needed and send
> > > > > > > > >
> > > > > > > > > I think that was discussed extensively quite a lot previously here:
> > > > > > > > > As Thomas already replied - main motivation is to allow user
> > > > > > > > > to execute them on different stages of packet TX pipeline,
> > > > > > > > > and probably on different cores.
> > > > > > > > > I think that provides better flexibility to the user to when/where
> > > > > > > > > do these preparations and hopefully would lead to better performance.
> > > > > > > >
> > > > > > > > And I agree, I think this use case is valid but does not warrant such a high
> > > > > > > > penalty when your application does not need that much flexibility. Simple
> > > > > > > > (yet conscious) applications need the highest performance. Complex ones as
> > > > > > > > you described already suffer quite a bit from IPCs and won't mind a couple
> > > > > > > > of extra CPU cycles right?
> > > > > > >
> > > > > > > It would mean an extra cache-miss for every packet, so I think performance hit
> > > > > > > would be quite significant.
> > > > > >
> > > > > > A performance hit has to occur somewhere regardless, because something has
> > > > > > to be done in order to send packets that need it. Whether this cost is in
> > > > > > application code or in a PMD function, it remains part of TX.
> > > > >
> > > > > Depending on the place the final cost would differ quite a lot.
> > > > > If you call tx_prepare() somewhere close to the place where you fill the packet header
> > > > > contents, then most likely the data that tx_prepare() has to access will be already in the cache.
> > > > > So the performance penalty will be minimal.
> > > > > If you'll try to access the same data later (at tx_burst), then the possibility that it would still
> > > > > be in cache is much less.
> > > > > If you calling tx_burst() from other core then data would for sure be out of cache,
> > > > > and even worse can still be in another core cache.
> > > >
> > > > Well sure, that's why I also think tx_prepare() has its uses, only that
> > > > since tx_prepare() is optional, tx_burst() should provide the same
> > > > functionality when tx_prepare() is not called.
> > >
> > > As I understand, to implement what you are proposing (TX_PREPARED mbuf->ol_flag)
> > > it will be required:
> > >
> > > a) Modify all existing applications that do similar to tx_prepare() stuff on their own,
> > > otherwise they'll would hit performance penalty.
> > > b) Modify at least all Intel PMDs and might be some others too (vmxnet3?).
> > >
> > > Step b) probably wouldn't cause any significant performance impact straightway,
> > > but it's for sure wouldn't make things faster, and would increase tx_burst() code
> > > complexity quite a lot.
> > > From other side, I can't see any real benefit that we will have in return.
> > > So I still opposed to that idea.
> >
> > Applications gain the ability to perform tx_burst() with offloads without
> > having to prepare anything.
>
> What means 'without preparing anything'?
> This is just not possible I think.
> One way or another application would have to to decide
> what exactly it likes to TX and what HW offloads it likes to use for it.
> So at least, it still needs to fill relevant mbuf fields:
> pkt_len, data_len, nb_segs, ol_flags, tx_offload, etc.
>
> >Currently these applications either cannot use
> > offloads at all or need to perform PMD-specific voodoo first.
>
> That's why tx_prepare() is introduced.
Yes, and the fact it appeared made me wonder why tx_burst() could not
implement its functionality as well. Why can't tx_burst() call
rte_net_intel_cksum_prepare() directly? I mean, unless offloads are
requested, this additional code should not impact performance.
> > The generic
> > alternative to this scenario being tx_prepare(), PMDs have to make this step
> > as cheap as possible.
> >
> > Yes that would slow down existing applications, people may find it
> > acceptable since we're modifying the TX API here.
> >
> > > > > > > About the 'simple' case when tx_prep() and tx_burst() are called on the same core,
> > > > > > > Why do you believe that:
> > > > > > > tx_prep(); tx_burst(); would be much slower than tx_burst() {tx_prep(), ...}?
> > > > > >
> > > > > > I mean instead of two function calls with their own loops:
> > > > > >
> > > > > > tx_prepare() { foreach (pkt) { check(); extra_check(); ... } }
> > > > > >
> > > > > > tx_burst() { foreach (pkt) { check(); stuff(); ... } }
> > > > > >
> > > > > > You end up with one:
> > > > > >
> > > > > > tx_burst() { foreach (pkt) { check(); extra_check(); stuff(); ... } }
> > > > > >
> > > > > > Which usually is more efficient.
> > > > >
> > > > > I really doubt that.
> > > > > If it would be that, what is the point to process packet in bulks?
> > > > > Usually dividing processing into different stages and at each stage processing
> > > > > multiple packet at once helps to improve performance.
> > > > > At least for IA.
> > > > > Look for example how we had to change l3fwd to improve its performance.
> > > >
> > > > Depends quite a bit on usage pattern. It is less efficient for applications
> > > > that do not modify mbuf contents because of the additional function call and
> > > > inner loop.
> > >
> > > If the application doesn't modify mbuf contents that it can simply skip calling tx_prepare().
> >
> > What if that same application wants to enable some offload as well?
>
> Hmm, wasn't that your use-case when no offloads (modifications) are required just 3 lines above?
Not exactly, this is a case where an application does not touch mbuf
contents yet wants to perform some HW offload on them. I'll concede this
scenario is likely rare enough for existing offloads.
What is likely not, though, is when an application discovers it can request
a particular offload right before calling tx_burst().
> > > > Note that I'm only pushing for the ability to conveniently address both
> > > > cases with maximum performance.
> > > >
> > > > > > > tx_prep() itself is quite expensive, let say for Intel HW it includes:
> > > > > > > - read mbuf fileds (2 cache-lines),
> > > > > > > - read packet header (1/2 cache-lines)
> > > > > > > - calculate pseudo-header csum
> > > > > > > - update packet header
> > > > > > > Comparing to that price of extra function call seems neglectable
> > > > > > > (if we TX packets in bursts of course).
> > > > > >
> > > > > > We agree its performance is a critical issue then, sharing half the read
> > > > > > steps with tx_burst() would make sense to me.
> > > > >
> > > > > I didn't understand that sentence.
> > > >
> > > > I meant this step can be shared (in addition to loop etc):
> > > >
> > > > - read mbuf fileds (2 cache-lines),
> > >
> > > Ah ok, you still believe that mixing tx_burst and tx_prepare code together
> > > would give us noticeable performance benefit.
> > > As I said above, I don't think it would, but you are welcome to try and
> > > prove me wrong.
> >
> > Depends on what you call noticeable. I guess we can at least agree having
> > two separate functions and loops cause more instructions to be generated and
> > executed.
> >
> > Now for the number of spent CPU cycles, depends of course whether mbufs are
> > still hot into the cache or not, and as I told you in my opinion we'll
> > usually see applications calling tx_prepare() just before tx_burst() to
> > benefit from offloads.
>
> Honestly, Adrien we are going in cycles here.
> Just to be clear:
> Current patch introduces tx_prepare() without affecting in any way:
> 1) existing applications
> 2) exiting PMD code (tx_burst)
> Meanwhile I still believe it is useful and provide a big step forward in terms
> of generalizing usage of HW TX offloads.
Yes and agreed. I do not oppose this new API on the basis that it's better
than what was available before.
> What you propose requires modifications for both existing applications and existing PMD code
> (full-featured tx_burst() for at least all Intel PMDs and vmxnet3 has to be significantly modified).
> You believe that with these modifications that new tx_burst() implementation
> would be noticeably faster than just current: tx_prepare(); tx_burst();
> I personally doubt that it really would (at least on modern IA).
> But as I said, you are more than welcome to prove me wrong here.
> Let say, provide a patch for ixgbe (or i40e) full-featured tx_burst() implementation,
> so that it would combine both tx_prepare() and tx_burst() functionalities into one function.
> Then we can run some performance tests with current and yours patches and compare results.
> Without that, I don't see any point to discuss your proposition any further.
> I just won't agree for such big change in existing PMDs without some solid justification beyond it.
I worry that proving you wrong would make you even stronger :), you raise
good points and I'm also a bit tired of this discussion. Without performance
numbers, my concerns are baseless.
Since non-Intel/vmxnet3 PMDs do not need tx_prepare() (yet), standardizing
on the case where it might be needed instead of assuming tx_burst() can do
all that work remains not better from a usability standpoint. It is done
because moving that stuff inside tx_burst() would be expensive even when
offloads are not requested, right?
> > > > > > > > Yes they will, therefore we need a method that satisfies both cases.
> > > > > > > >
> > > > > > > > As a possible solution, a special mbuf flag could be added to each mbuf
> > > > > > > > having gone through tx_prepare(). That way, tx_burst() could skip some
> > > > > > > > checks and things it would otherwise have done.
> > > > > > >
> > > > > > > That's an interesting idea, but it has one drawback:
> > > > > > > As I understand, it means that from now on if user doing preparations on his own,
> > > > > > > he had to setup this flag, otherwise tx_burst() would do extra unnecessary work.
> > > > > > > So any existing applications that using TX offloads and do preparation by themselves
> > > > > > > would have to be modified to avoid performance loss.
> > > > > >
> > > > > > In my opinion, users should not do preparation on their own.
> > > > >
> > > > > People already do it now.
> > > >
> > > > But we do not want them to anymore thanks to this new API, for reasons
> > > > described in the motivation section of the cover letter, right?
> > >
> > > We probably wouldn't recommend that, but if people would like to use their own stuff,
> > > or shortcuts - I don't want to stop them here.
> > >
> > > >
> > > > > > If we provide a
> > > > > > generic method, it has to be fast enough to replace theirs. Perhaps not as
> > > > > > fast since it would work with all PMDs (usual trade-off), but acceptably so.
> > > > > >
> > > > > > > > Another possibility, telling the PMD first that you always intend to use
> > > > > > > > tx_prepare() and getting a simpler/faster tx_burst() callback as a result.
> > > > > > >
> > > > > > > That what we have right now (at least for Intel HW):
> > > > > > > it is a user responsibility to do the necessary preparations/checks before calling tx_burst().
> > > > > > > With tx_prepare() we just remove from user the headache to implement tx_prepare() on his own.
> > > > > > > Now he can use a 'proper' PMD provided function.
> > > > > > >
> > > > > > > My vote still would be for that model.
> > > > > >
> > > > > > OK, then in a nutshell:
> > > > > >
> > > > > > 1. Users are not expected to perform preparation/checks themselves anymore,
> > > > > > if they do, it's their problem.
> > > > >
> > > > > I think we need to be backward compatible here.
> > > > > If the existing app doing what tx_prepare() supposed to do, it should keep working.
> > > >
> > > > It should work, only if they keep doing it as well as call tx_burst()
> > > > directly, they will likely get lower performance.
> > > >
> > > > > > 2. If configured through an API to be defined, tx_burst() can be split in
> > > > > > two and applications must call tx_prepare() at some point before
> > > > > > tx_burst().
> > > > > >
> > > > > > 3. Otherwise tx_burst() should perform the necessary preparation and checks
> > > > > > on its own by default (when tx_prepare() is not expected).
> > > > >
> > > > > As I said before, I don't think it should be mandatory for tx_burst() to do what tx_prepare() does.
> > > > > If some particular implementation of tx_burst() prefers to do things that way - that's fine.
> > > > > But it shouldn't be required to.
> > > >
> > > > You're right, however applications might find it convenient. I think most
> > > > will end up with something like the following:
> > > >
> > > > if (tx_prepare(pkts))
> > > > tx_burst(pkts));
> > >
> > > Looking at existing DPDK apps - most of them do use some sort of TX bufferization.
> > > So, even in a simplistic app it would probably be:
> > >
> > > tx_prepare(pkts);
> > > tx_buffer(pkts);
> >
> > We're down to my word against yours here I guess, to leave the choice to
> > application developers, we'd need to provide tx_prepare() and a simpler
> > tx_burst() as well as the ability to call tx_burst() directly and still get
> > offloads.
>
> From what I've seen, most DPDK libs/apps do buffer data packets for TX in one or another way:
> mtcp, warp17, seastar.
I was arguing most applications wouldn't call tx_prepare() far from
tx_burst() and that merging them would therefore make sense, which seems to
be the case in these examples. Obviously they're doing bufferization.
- warp17 would call tx_prepare() inside pkt_flush_tx_q() right before
tx_burst(), it wouldn't make sense elsewhere.
- mtcp has an interesting loop on tx_burst(), however again, tx_prepare()
would be performed inside dpdk_send_pkts(), not where mbufs are
populated.
- seastar seems to expose the ability to send bursts as well,
dpdk_qp::_send() would also call tx_prepare() before tx_burst().
> Not to mention sample apps.
> But ok, as said above, if you can prove that tx_burst() you are proposing is really much faster then
> tx_prepare(); tx_burst();
> I'll be happy to reconsider.
Nowhere I wrote "much" faster, I was just pointing out it would be more
efficient if the work was done in a single function, at least for the use
cases found in several examples you've provided.
Now if you tell me it cannot be significant, fine, but still.
> > > > > > 4. We probably still need some mbuf flag to mark mbufs that cannot be
> > > > > > modified, the refcount could also serve as a hint.
> > > > >
> > > > > If mbuf can't be modified, you probably just wouldn't call the function that supposed to do that,
> > > > > tx_prepare() in that case.
> > > >
> > > > I think it would be easier to document what offload flags may cause the
> > > > tx_burst() function to modify mbuf contents, so applications have the
> > > > ability to set or strip these flags on a mbuf basis. That way there is no
> > > > need to call tx_prepare() without knowing exactly what it's going to do.
> > >
> > > Not sure I understand what exactly do you propose in the last paragraph?
> >
> > That for each TX offload flag, we document whether preparation might cause a
> > mbuf to be written to during the tx_prepare()/tx_burst() phase. One of the
> > reasons for tx_prepare() being:
> >
> > 4) Fields in packet may require different initialization (like e.g. will
> > require pseudo-header checksum precalculation, sometimes in a
> > different way depending on packet type, and so on). Now application
> > needs to care about it.
> >
> > If we determine what offloads may cause mbuf contents to change (all of them
> > perhaps?), then applications can easily strip those flags from outgoing
> > "const" mbufs. Then it becomes acceptable for tx_burst() to modify mbuf
> > contents as per user request, which removes one reason to rely on
> > tx_prepare() for these.
>
> Hmm, I didn't get you here.
> If let say I don't need TX TCP cksum offload, why would I set this flag inside mbuf at first place?
> If I do expect PMD to do TX TCP cksum offload for me, then I have to set that flag,
> otherwise how PMD would know that I did require that offload?
Let me take an example, if you assume a cloned mbuf must be sent, pointed
data is not owned by the TX function, and software preparation for TX
offloads might cause side effects elsewhere, I'm not talking about the
changes performed by HW to the outgoing frame here.
One reason for tx_prepare() according to 4) is to make this clear; by
calling tx_prepare(), applications fully acknowledge and take responsibility
for possible side-effects.
Hence my suggestion, we could make this acceptable for tx_burst() by
documenting that requesting offloads may cause it to modify mbuf contents as
well.
Before you reply, note that I've neither acked nor nacked that series, and
intend to leave this to other people. So far it's received several acks
already so I guess it's probably ready for inclusion.
I'm not comfortable with it as an application developer because of the extra
API call that I think could be included in tx_burst(), but as a PMD
maintainer I do not mind. Might even prove useful someday if some
corner-case offload too expensive to fully handle in tx_burst() had to be
supported. An optional offload if you will.
--
Adrien Mazarguil
6WIND
^ permalink raw reply
* Re: [PATCH 11/32] net/dpaa2: add dpaa2 vfio support
From: Hemant Agrawal @ 2016-12-07 10:04 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, bruce.richardson, shreyansh.jain
In-Reply-To: <2366721.OpDVYN8WCp@xps13>
On 12/7/2016 2:08 PM, Thomas Monjalon wrote:
> 2016-12-07 12:30, Hemant Agrawal:
>> On 12/7/2016 2:34 AM, Thomas Monjalon wrote:
>>> 2016-12-04 23:47, Hemant Agrawal:
>>>> Add support for using VFIO for dpaa2 based fsl-mc bus.
>>>
>>> Why do we need so much special code for interfacing VFIO on fsl-mc?
>>> Can you reuse some code from EAL VFIO?
>>>
>>
>> fsl-mc VFIO scans the objects. So, it is slightly different.
>>
>> Even though I have tried to minimize changes and re-use the code from
>> EAL VFIO. It is still refactoring work, which we will take up little later.
>
> Do you mean you could re-use some EAL code but do not want to do it now?
> It sounds like something heard too many times earlier (from others).
> The experience tells we must not wait to do things right.
>
I meant that I was able to use some of the EAL VFIO functions. There are
some changes which I am planning to propose in eal vfio and in our driver.
I will do it in subsequent patchsets of current series shortly.
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ananyev, Konstantin @ 2016-12-07 10:03 UTC (permalink / raw)
To: Yigit, Ferruh, Yong Wang, Thomas Monjalon
Cc: Harish Patil, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Adrien Mazarguil,
Alejandro Lucero, Rasesh Mody, Jacob, Jerin, Yuanhan Liu,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <8b0acf96-cba9-6c91-92c4-93674052e995@intel.com>
Hi Ferruh,
>
> On 12/6/2016 6:25 PM, Yong Wang wrote:
> >> -----Original Message-----
> >> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
> >> Sent: Sunday, December 4, 2016 4:11 AM
> >> To: Yong Wang <yongwang@vmware.com>; Thomas Monjalon
> >> <thomas.monjalon@6wind.com>
> >> Cc: Harish Patil <harish.patil@qlogic.com>; dev@dpdk.org; Rahul Lakkireddy
> >> <rahul.lakkireddy@chelsio.com>; Stephen Hurd
> >> <stephen.hurd@broadcom.com>; Jan Medala <jan@semihalf.com>; Jakub
> >> Palider <jpa@semihalf.com>; John Daley <johndale@cisco.com>; Adrien
> >> Mazarguil <adrien.mazarguil@6wind.com>; Alejandro Lucero
> >> <alejandro.lucero@netronome.com>; Rasesh Mody
> >> <rasesh.mody@qlogic.com>; Jacob, Jerin <Jerin.Jacob@cavium.com>;
> >> Yuanhan Liu <yuanhan.liu@linux.intel.com>; Kulasek, TomaszX
> >> <tomaszx.kulasek@intel.com>; olivier.matz@6wind.com
> >> Subject: RE: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
> >>
> >> Hi
> >>
> >>
> >>
> >>>>
> >>
> >>>> 2016-11-30 17:42, Ananyev, Konstantin:
> >>
> >>>>>>> Please, we need a comment for each driver saying
> >>
> >>>>>>> "it is OK, we do not need any checksum preparation for TSO"
> >>
> >>>>>>> or
> >>
> >>>>>>> "yes we have to implement tx_prepare or TSO will not work in this
> >>
> >>>> mode"
> >>
> >>>>>>>
> >>
> >>>>>>
> >>
> >>>>>> qede PMD doesn’t currently support TSO yet, it only supports Tx
> >>
> >>>> TCP/UDP/IP
> >>
> >>>>>> csum offloads.
> >>
> >>>>>> So Tx preparation isn’t applicable. So as of now -
> >>
> >>>>>> "it is OK, we do not need any checksum preparation for TSO"
> >>
> >>>>>
> >>
> >>>>> Thanks for the answer.
> >>
> >>>>> Though please note that it not only for TSO.
> >>
> >>>>
> >>
> >>>> Oh yes, sorry, my wording was incorrect.
> >>
> >>>> We need to know if any checksum preparation is needed prior
> >>
> >>>> offloading its final computation to the hardware or driver.
> >>
> >>>> So the question applies to TSO and simple checksum offload.
> >>
> >>>>
> >>
> >>>> We are still waiting answers for
> >>
> >>>> bnxt, cxgbe, ena, nfp, thunderx, virtio and vmxnet3.
> >>
> >>>
> >>
> >>> The case for a virtual device is a little bit more complicated as packets
> >> offloaded from a virtual device can eventually be delivered to
> >>
> >>> another virtual NIC or different physical NICs that have different offload
> >> requirements. In ESX, the hypervisor will enforce that the packets
> >>
> >>> offloaded will be something that the hardware expects. The contract for
> >> vmxnet3 is that the guest needs to fill in pseudo header checksum
> >>
> >>> for both l4 checksum only and TSO + l4 checksum offload cases.
> >>
> >>
> >>
> >> Ok, so at first glance that looks to me very similar to Intel HW requirements.
> >>
> >> Could you confirm would rte_net_intel_cksum_prepare()
> >>
> >> also work for vmxnet3 or some extra modifications are required?
> >>
> >> You can look at it here: https://urldefense.proofpoint.com/v2/url?u=http-
> >> 3A__dpdk.org_dev_patchwork_patch_17184_&d=DgIGaQ&c=uilaK90D4TOV
> >> oH58JNXRgQ&r=v4BBYIqiDq552fkYnKKFBFyqvMXOR3UXSdFO2plFD1s&m=NS
> >> 4zOl2je_tyGhnOJMSnu37HmJxOZf-1KLYcVsu8iYY&s=dL-NOC-
> >> 18HclXUURQzuyW5Udw4NY13pKMndYvfgCfbA&e= .
> >>
> >> Note that for Intel HW the rules for pseudo-header csum calculation
> >>
> >> differ for TSO and non-TSO case.
> >>
> >> For TSO length inside pseudo-header are set to 0, while for non-tso case
> >>
> >> It should be set to L3 payload length.
> >>
> >> Is it the same for vmxnet3 or no?
> >>
> >> Thanks
> >>
> >> Konstantin
> >>
> >
> > Yes and this is the same for vmxnet3.
> >
>
> This means vmxnet3 PMD also should be updated, right?
Yes, that's right.
>Should that update
> be part of tx_prep patchset? Or separate patch?
Another question I suppose is who will do the actual patch for vmxnet3.
Yong, are you ok to do the patch for vmxnet3, or prefer us to do that?
Please note, that in both cases will need your help in testing/reviewing it.
Konstantin
>
> >>>
> >>
> >>>>> This is for any TX offload for which the upper layer SW would have
> >>
> >>>>> to modify the contents of the packet.
> >>
> >>>>> Though as I can see for qede neither PKT_TX_IP_CKSUM or
> >>
> >>>> PKT_TX_TCP_CKSUM
> >>
> >>>>> exhibits any extra requirements for the user.
> >>
> >>>>> Is that correct?
> >>
> >>
> >
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Ferruh Yigit @ 2016-12-07 9:57 UTC (permalink / raw)
To: Yong Wang, Ananyev, Konstantin, Thomas Monjalon
Cc: Harish Patil, dev@dpdk.org, Rahul Lakkireddy, Stephen Hurd,
Jan Medala, Jakub Palider, John Daley, Adrien Mazarguil,
Alejandro Lucero, Rasesh Mody, Jacob, Jerin, Yuanhan Liu,
Kulasek, TomaszX, olivier.matz@6wind.com
In-Reply-To: <BY2PR05MB2359E2D5220CACCC5A377D8DAF820@BY2PR05MB2359.namprd05.prod.outlook.com>
On 12/6/2016 6:25 PM, Yong Wang wrote:
>> -----Original Message-----
>> From: Ananyev, Konstantin [mailto:konstantin.ananyev@intel.com]
>> Sent: Sunday, December 4, 2016 4:11 AM
>> To: Yong Wang <yongwang@vmware.com>; Thomas Monjalon
>> <thomas.monjalon@6wind.com>
>> Cc: Harish Patil <harish.patil@qlogic.com>; dev@dpdk.org; Rahul Lakkireddy
>> <rahul.lakkireddy@chelsio.com>; Stephen Hurd
>> <stephen.hurd@broadcom.com>; Jan Medala <jan@semihalf.com>; Jakub
>> Palider <jpa@semihalf.com>; John Daley <johndale@cisco.com>; Adrien
>> Mazarguil <adrien.mazarguil@6wind.com>; Alejandro Lucero
>> <alejandro.lucero@netronome.com>; Rasesh Mody
>> <rasesh.mody@qlogic.com>; Jacob, Jerin <Jerin.Jacob@cavium.com>;
>> Yuanhan Liu <yuanhan.liu@linux.intel.com>; Kulasek, TomaszX
>> <tomaszx.kulasek@intel.com>; olivier.matz@6wind.com
>> Subject: RE: [dpdk-dev] [PATCH v12 0/6] add Tx preparation
>>
>> Hi
>>
>>
>>
>>>>
>>
>>>> 2016-11-30 17:42, Ananyev, Konstantin:
>>
>>>>>>> Please, we need a comment for each driver saying
>>
>>>>>>> "it is OK, we do not need any checksum preparation for TSO"
>>
>>>>>>> or
>>
>>>>>>> "yes we have to implement tx_prepare or TSO will not work in this
>>
>>>> mode"
>>
>>>>>>>
>>
>>>>>>
>>
>>>>>> qede PMD doesn’t currently support TSO yet, it only supports Tx
>>
>>>> TCP/UDP/IP
>>
>>>>>> csum offloads.
>>
>>>>>> So Tx preparation isn’t applicable. So as of now -
>>
>>>>>> "it is OK, we do not need any checksum preparation for TSO"
>>
>>>>>
>>
>>>>> Thanks for the answer.
>>
>>>>> Though please note that it not only for TSO.
>>
>>>>
>>
>>>> Oh yes, sorry, my wording was incorrect.
>>
>>>> We need to know if any checksum preparation is needed prior
>>
>>>> offloading its final computation to the hardware or driver.
>>
>>>> So the question applies to TSO and simple checksum offload.
>>
>>>>
>>
>>>> We are still waiting answers for
>>
>>>> bnxt, cxgbe, ena, nfp, thunderx, virtio and vmxnet3.
>>
>>>
>>
>>> The case for a virtual device is a little bit more complicated as packets
>> offloaded from a virtual device can eventually be delivered to
>>
>>> another virtual NIC or different physical NICs that have different offload
>> requirements. In ESX, the hypervisor will enforce that the packets
>>
>>> offloaded will be something that the hardware expects. The contract for
>> vmxnet3 is that the guest needs to fill in pseudo header checksum
>>
>>> for both l4 checksum only and TSO + l4 checksum offload cases.
>>
>>
>>
>> Ok, so at first glance that looks to me very similar to Intel HW requirements.
>>
>> Could you confirm would rte_net_intel_cksum_prepare()
>>
>> also work for vmxnet3 or some extra modifications are required?
>>
>> You can look at it here: https://urldefense.proofpoint.com/v2/url?u=http-
>> 3A__dpdk.org_dev_patchwork_patch_17184_&d=DgIGaQ&c=uilaK90D4TOV
>> oH58JNXRgQ&r=v4BBYIqiDq552fkYnKKFBFyqvMXOR3UXSdFO2plFD1s&m=NS
>> 4zOl2je_tyGhnOJMSnu37HmJxOZf-1KLYcVsu8iYY&s=dL-NOC-
>> 18HclXUURQzuyW5Udw4NY13pKMndYvfgCfbA&e= .
>>
>> Note that for Intel HW the rules for pseudo-header csum calculation
>>
>> differ for TSO and non-TSO case.
>>
>> For TSO length inside pseudo-header are set to 0, while for non-tso case
>>
>> It should be set to L3 payload length.
>>
>> Is it the same for vmxnet3 or no?
>>
>> Thanks
>>
>> Konstantin
>>
>
> Yes and this is the same for vmxnet3.
>
This means vmxnet3 PMD also should be updated, right? Should that update
be part of tx_prep patchset? Or separate patch?
>>>
>>
>>>>> This is for any TX offload for which the upper layer SW would have
>>
>>>>> to modify the contents of the packet.
>>
>>>>> Though as I can see for qede neither PKT_TX_IP_CKSUM or
>>
>>>> PKT_TX_TCP_CKSUM
>>
>>>>> exhibits any extra requirements for the user.
>>
>>>>> Is that correct?
>>
>>
>
^ permalink raw reply
* Re: [PATCH 18/32] net/dpaa2: introducing dpaa2 pmd driver
From: Hemant Agrawal @ 2016-12-07 9:55 UTC (permalink / raw)
To: Thomas Monjalon, Ferruh Yigit; +Cc: dev, bruce.richardson, shreyansh.jain
In-Reply-To: <2956951.cEfl6HOu1U@xps13>
On 12/7/2016 2:38 AM, Thomas Monjalon wrote:
> 2016-12-06 19:49, Ferruh Yigit:
>> On 12/4/2016 6:17 PM, Hemant Agrawal wrote:
>>> + if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>>> + eth_dev->data->dev_private = rte_zmalloc(
>>> + "ethdev private structure",
>>> + eth_drv->dev_private_size,
>>> + RTE_CACHE_LINE_SIZE);
>>> + if (eth_dev->data->dev_private == NULL)
>>> + rte_panic("Cannot allocate memzone for private port"
>>> + " data\n");
>>
>> Should this error kill all app, or return an error for this PMD that is
>> probed.
>
> It cannot be a question :)
> rte_panic() inside libs or drivers is forbidden (and existing ones must
> be removed).
>
I will change it to return an error.
^ permalink raw reply
* Re: [PATCH 00/32] NXP DPAA2 PMD
From: Hemant Agrawal @ 2016-12-07 9:53 UTC (permalink / raw)
To: Ferruh Yigit, dev; +Cc: thomas.monjalon, bruce.richardson, shreyansh.jain
In-Reply-To: <32d28626-8f6e-a12e-7e98-433e81355c18@intel.com>
On 12/7/2016 1:18 AM, Ferruh Yigit wrote:
> On 12/4/2016 6:16 PM, Hemant Agrawal wrote:
>> The patch series adds NXP’s QorIQ-Layerscape DPAA2 Architecture based
>> network SoC PMD. This version of the driver supports NXP LS208xA,
>> LS204xA and LS108x families Network SoCs.
>>
>> DPAA2, or Data Path Acceleration Architecture, is a hardware architecture
>> designed for high-speed network packet processing. It uses a bus name
>> ‘fsl-mc’, part of Linux Kernel Staging tree [2], for resource management.
>>
>> A brief description of architecture is given below; detailed description
>> is part of the documentation in the patches itself.
>>
>> DPAA2 contains hardware component called the Management Complex (or MC).
>> It manages the DPAA2 hardware resources. The MC provides an object-based
>> abstraction for software drivers to use the DPAA2 hardware.
>>
>> Some of the key objects are:
>> - DPNI, which refers to the network interface object.
>> - DPBP, which refers to HW based memory pool object
>> - DPIO, refers to processing context for accessing QBMAN
>>
>> Besides the MC, DPAA2 also includes a Hardware based Queue and Buffer Manager
>> called QBMAN. Prime responsibility of QBMAN is to allow lockless access to
>> software/user-space to the queues and buffers implemented in the hardware.
>>
>> The patch series could be logically structured into following sub-areas:
>> 1. (Patch 0001) DPAA2 Architecture overview document
>> 2. (Patches 0002-0007) Common dpaa2 hw accelerator drivers for MC and QBMAN.
>> 3. (Patch 0008) Enabling crc in armv8 core machine type
>> 4. (Patch 0009) Adding rte_device in rte_eth_dev
>> 5. (Patches 0010-0013) introduce DPAA2 bus and VFIO routines
>> 6. (Patches 0014-0017) dpio and dpbp object drivers
>> 7. (Patches 0018-0027) Support for DPAA2 Ethernet Device (ethdev)
>> 8. (Patches 0028-0032) Additional functionality in DPAA2 ethdev.
>>
>> The following design decisions are made during development:
>>
>> 1. DPAA2 implements a new bus called "dpaa2" and some common accelerator drivers.
>> These drivers will be shared with dpaa2 based crypto drivers.
>> - For this, patch series from Shreyansh [1] has been used for creating a
>> bus handler.
>> - For the purpose of this bus, rte_dpaa2_device/rte_dpaa2_driver might
>> also be required but they are not part of the first patch series.
>> Currently, rte_device/driver are being directly used as a PoC.
>>
>> 2. DPAA2 implements the HW mempool offload with DPBP object.
>> - The new pool is being configured using compile time option and pool name
>> as "dpaa2".
>>
>> 3. It maintains per lcore DPIO objects and affine the DPIO instance to the
>> processing threads accessing the QBMAN HW.
>>
>> Prerequisites:
>> - For running the PMD, NXP's SoC (board) and SDK (software/BSP) is required.
>> Information about obtaining relevant software is available in the docs
>> as part of the patch.
>> - At present the series has limited support for Ethernet functions. But,
>> more functionality would be made available in a phased manner.
>> - This PMD has been validated over the Bus Model [1] and SoC Patchset [3]
>
> Just to clarify this patchset depends other patchset, although mentioned
> above, it is good to have links for dependent patches:
>
> Dependencies:
>
> - [4]
>
> [4] http://dpdk.org/dev/patchwork/patch/17620/
Thanks for pointing. I missed it.
>
>>
>>
>> [1] http://dpdk.org/ml/archives/dev/2016-December/051349.html
>> [2] https://www.kernel.org/doc/readme/drivers-staging-fsl-mc-README.txt
>> [3] http://dpdk.org/ml/archives/dev/2016-October/048949.html
>>
>>
>> Hemant Agrawal (32):
>> doc: add dpaa2 nic details
>> drivers/common: introducing dpaa2 mc driver
>> drivers/common/dpaa2: add mc dpni object support
>> drivers/common/dpaa2: add mc dpio object support
>> drivers/common/dpaa2: add mc dpbp object support
>> drivers/common/dpaa2: add mc dpseci object support
>> drivers/common/dpaa2: adding qbman driver
>> mk/dpaa2: add the crc support to the machine type
>> lib/ether: add rte_device in rte_eth_dev
>> net/dpaa2: introducing dpaa2 bus driver for fsl-mc bus
>> net/dpaa2: add dpaa2 vfio support
>> net/dpaa2: vfio scan for net and sec device
>> net/dpaa2: add debug log macros
>> net/dpaa2: dpio object driver
>> net/dpaa2: dpio routine to affine to crypto threads
>> net/dpaa2: dpio add support to check SOC type
>> net/dpaa2: dpbp based mempool hw offload driver
>> net/dpaa2: introducing dpaa2 pmd driver
>> net/dpaa2: adding eth ops to dpaa2
>> net/dpaa2: add queue configuration support
>> net/dpaa2: add rss flow distribution
>> net/dpaa2: configure mac address at init
>> net/dpaa2: attach the buffer pool to dpni
>> net/dpaa2: add support for l3 and l4 checksum offload
>> net/dpaa2: add support for promiscuous mode
>> net/dpaa2: add mtu config support
>> net/dpaa2: add packet rx and tx support
>> net/dpaa2: add support for physical address usages
>> net/dpaa2: rx packet parsing and packet type support
>> net/dpaa2: frame queue based dq storage alloc
>> net/dpaa2: add support for non hw buffer pool packet transmit
>> net/dpaa2: enable stashing for LS2088A devices
>>
>> config/defconfig_arm64-dpaa2-linuxapp-gcc | 15 +-
>> doc/guides/nics/dpaa2.rst | 537 +++++++
>> doc/guides/nics/features/dpaa2.ini | 16 +
>> doc/guides/nics/index.rst | 1 +
>> drivers/Makefile | 1 +
>> drivers/common/Makefile | 36 +
>> drivers/common/dpaa2/Makefile | 37 +
>> drivers/common/dpaa2/mc/Makefile | 57 +
>> drivers/common/dpaa2/mc/dpaa2_mc_version.map | 4 +
>> drivers/common/dpaa2/mc/dpbp.c | 230 +++
>> drivers/common/dpaa2/mc/dpio.c | 272 ++++
>> drivers/common/dpaa2/mc/dpni.c | 667 +++++++++
>> drivers/common/dpaa2/mc/dpseci.c | 527 +++++++
>> drivers/common/dpaa2/mc/fsl_dpbp.h | 220 +++
>> drivers/common/dpaa2/mc/fsl_dpbp_cmd.h | 76 +
>> drivers/common/dpaa2/mc/fsl_dpio.h | 275 ++++
>> drivers/common/dpaa2/mc/fsl_dpio_cmd.h | 114 ++
>> drivers/common/dpaa2/mc/fsl_dpkg.h | 177 +++
>> drivers/common/dpaa2/mc/fsl_dpni.h | 1076 ++++++++++++++
>> drivers/common/dpaa2/mc/fsl_dpni_cmd.h | 301 ++++
>> drivers/common/dpaa2/mc/fsl_dpseci.h | 661 +++++++++
>> drivers/common/dpaa2/mc/fsl_dpseci_cmd.h | 248 ++++
>> drivers/common/dpaa2/mc/fsl_mc_cmd.h | 231 +++
>> drivers/common/dpaa2/mc/fsl_mc_sys.h | 98 ++
>> drivers/common/dpaa2/mc/fsl_net.h | 480 +++++++
>> drivers/common/dpaa2/mc/mc_sys.c | 126 ++
>> drivers/common/dpaa2/qbman/Makefile | 55 +
>> drivers/common/dpaa2/qbman/dpaa2_qbman_version.map | 4 +
>> drivers/common/dpaa2/qbman/include/compat.h | 550 ++++++++
>> .../common/dpaa2/qbman/include/fsl_qbman_base.h | 157 +++
>> .../common/dpaa2/qbman/include/fsl_qbman_portal.h | 1089 +++++++++++++++
>> drivers/common/dpaa2/qbman/qbman_portal.c | 1476 ++++++++++++++++++++
>> drivers/common/dpaa2/qbman/qbman_portal.h | 269 ++++
>> drivers/common/dpaa2/qbman/qbman_private.h | 164 +++
>> drivers/common/dpaa2/qbman/qbman_sys.h | 375 +++++
>> drivers/common/dpaa2/qbman/qbman_sys_decl.h | 69 +
>> drivers/net/Makefile | 2 +-
>> drivers/net/dpaa2/Makefile | 73 +
>> drivers/net/dpaa2/base/dpaa2_hw_dpbp.c | 367 +++++
>> drivers/net/dpaa2/base/dpaa2_hw_dpbp.h | 101 ++
>> drivers/net/dpaa2/base/dpaa2_hw_dpio.c | 513 +++++++
>> drivers/net/dpaa2/base/dpaa2_hw_dpio.h | 76 +
>> drivers/net/dpaa2/base/dpaa2_hw_dpni.c | 343 +++++
>> drivers/net/dpaa2/base/dpaa2_hw_dpni.h | 86 ++
>> drivers/net/dpaa2/base/dpaa2_hw_dpni_annot.h | 256 ++++
>> drivers/net/dpaa2/base/dpaa2_hw_pvt.h | 235 ++++
>> drivers/net/dpaa2/dpaa2_bus.c | 170 +++
>> drivers/net/dpaa2/dpaa2_ethdev.c | 723 ++++++++++
>> drivers/net/dpaa2/dpaa2_ethdev.h | 41 +
>> drivers/net/dpaa2/dpaa2_logs.h | 77 +
>> drivers/net/dpaa2/dpaa2_rxtx.c | 419 ++++++
>> drivers/net/dpaa2/dpaa2_vfio.c | 561 ++++++++
>> drivers/net/dpaa2/dpaa2_vfio.h | 74 +
>> drivers/net/dpaa2/rte_dpaa2.h | 121 ++
>> drivers/net/dpaa2/rte_pmd_dpaa2_version.map | 4 +
>> lib/librte_ether/rte_ethdev.h | 1 +
>> mk/machine/dpaa2/rte.vars.mk | 5 +-
>> mk/rte.app.mk | 1 +
>> 58 files changed, 14936 insertions(+), 4 deletions(-)
>> create mode 100644 doc/guides/nics/dpaa2.rst
>> create mode 100644 doc/guides/nics/features/dpaa2.ini
>> create mode 100644 drivers/common/Makefile
>> create mode 100644 drivers/common/dpaa2/Makefile
>> create mode 100644 drivers/common/dpaa2/mc/Makefile
>> create mode 100644 drivers/common/dpaa2/mc/dpaa2_mc_version.map
>> create mode 100644 drivers/common/dpaa2/mc/dpbp.c
>> create mode 100644 drivers/common/dpaa2/mc/dpio.c
>> create mode 100644 drivers/common/dpaa2/mc/dpni.c
>> create mode 100644 drivers/common/dpaa2/mc/dpseci.c
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpbp.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpbp_cmd.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpio.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpio_cmd.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpkg.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpni.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpni_cmd.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpseci.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_dpseci_cmd.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_mc_cmd.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_mc_sys.h
>> create mode 100644 drivers/common/dpaa2/mc/fsl_net.h
>> create mode 100644 drivers/common/dpaa2/mc/mc_sys.c
>> create mode 100644 drivers/common/dpaa2/qbman/Makefile
>> create mode 100644 drivers/common/dpaa2/qbman/dpaa2_qbman_version.map
>> create mode 100644 drivers/common/dpaa2/qbman/include/compat.h
>> create mode 100644 drivers/common/dpaa2/qbman/include/fsl_qbman_base.h
>> create mode 100644 drivers/common/dpaa2/qbman/include/fsl_qbman_portal.h
>> create mode 100644 drivers/common/dpaa2/qbman/qbman_portal.c
>> create mode 100644 drivers/common/dpaa2/qbman/qbman_portal.h
>> create mode 100644 drivers/common/dpaa2/qbman/qbman_private.h
>> create mode 100644 drivers/common/dpaa2/qbman/qbman_sys.h
>> create mode 100644 drivers/common/dpaa2/qbman/qbman_sys_decl.h
>> create mode 100644 drivers/net/dpaa2/Makefile
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpbp.c
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpbp.h
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpio.c
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpio.h
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpni.c
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpni.h
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_dpni_annot.h
>> create mode 100644 drivers/net/dpaa2/base/dpaa2_hw_pvt.h
>> create mode 100644 drivers/net/dpaa2/dpaa2_bus.c
>> create mode 100644 drivers/net/dpaa2/dpaa2_ethdev.c
>> create mode 100644 drivers/net/dpaa2/dpaa2_ethdev.h
>> create mode 100644 drivers/net/dpaa2/dpaa2_logs.h
>> create mode 100644 drivers/net/dpaa2/dpaa2_rxtx.c
>> create mode 100644 drivers/net/dpaa2/dpaa2_vfio.c
>> create mode 100644 drivers/net/dpaa2/dpaa2_vfio.h
>> create mode 100644 drivers/net/dpaa2/rte_dpaa2.h
>> create mode 100644 drivers/net/dpaa2/rte_pmd_dpaa2_version.map
>>
>
>
^ permalink raw reply
* Re: [PATCH 00/13] Introducing EAL Bus-Device-Driver Model
From: Shreyansh Jain @ 2016-12-07 9:55 UTC (permalink / raw)
To: David Marchand; +Cc: dev@dpdk.org, Thomas Monjalon
In-Reply-To: <CALwxeUtqnNpm=KuR3Qtb2PNUghAc=e9TKQqm4Uh8pvgZ4EmriA@mail.gmail.com>
Hello David,
On Wednesday 07 December 2016 02:22 AM, David Marchand wrote:
> "Big patchset and a lot of things to look at.
>
> Here is a first look at it.
Thanks for comments.
>
> On Sun, Dec 4, 2016 at 11:11 AM, Shreyansh Jain <shreyansh.jain@nxp.com> wrote:
>> In continuation to the RFC posted on 17/Nov [9],
>> A series of patches is being posted which attempts to create:
>> 1. A basic bus model
>> `- define rte_bus and associated methods/helpers
>> `- test infrastructure to test the Bus infra
>> 2. Changes in EAL to support PCI as a bus
>> `- a "pci" bus is registered
>> `- existing scan/match/probe are modified to allow for bus integration
>> `- PCI Device and Driver list, which were global entities, have been
>> moved to rte_bus->[device/driver]_list
>>
>> I have sanity tested this patch over a XeonD X552 available with me, as
>> well as part of PoC for verifying NXP's DPAA2 PMD (being pushed out in a
>> separate series). Exhaustive testing is still pending.
>
> I saw some checkpatch issues for patch 1 (I would ignore this one) and
> 4 (please check).
Yes, for Patch 1 I too ignored the warnings.
For Patch 4, there are 3 warnings, of which first 2 were reported by
checkpatch tool before I sent out. Surprisingly, I didn't see the third
one when I ran the tool in my environment.
I will fix them (those which I can) in v2.
>
>
>> :: Brief about Patch Layout ::
>>
>> 0001: Container_of patch from [3]
>
>> 0002~0003: Introducing the basic Bus model and associated test case
>> 0005: Support insertion of device rather than addition to tail
>
> Patch 2 and 5 could be squashed.
I deliberately kept them separate. I intent to extend the Patch 5 for
hotplugging. But, if I don't end up adding support for that in this
series, I will merge these two.
> The constructor priority stuff seems unneeded as long as we use
> explicit reference to a global (or local, did not check) bus symbol
> rather than a runtime lookup.
I didn't understand your point here.
IMO, constructor priority (or some other way to handle this) is
important. I faced this issue while verifying it at my end when the
drivers were getting registered before the bus.
Can you elaborate more on '..use explicit reference to a global...'?
>
>
>> 0004: Add scan and match callbacks for the Bus and updated test case
>
> Why do you push back the bus object in the 'scan' method ?
> This method is bus specific which means that the code "knows" the
> object registered with the callback.
This 'knows' is the grey area for me.
The bus (for example, PCI) after scanning needs to call
rte_eal_bus_add_device() to link the device in bus's device_list.
Two options:
1. Have a global reference to "pci" bus (rte_bus) somewhere in eal_pci.c
2. Call rte_eal_get_bus() every time someone needs the reference.
3. C++ style, 'this->'.
I have taken the 3rd path. It simplifies my code to not assume a handle
as well as not allow for reference fetch calls every now and then.
As a disadvantage: it means passing this as argument - and some cases
maintaining it as __rte_unused.
Taking (1) or (2) is not advantageous than this approach.
> Is is that you want to have a single scan method used by multiple buses ?
Yes, but only as a use case. For example, platform devices are of
various types - what if we have a south-bound bus over a platform bus.
In which case, a hierarchical bus layout is possible.
But, this is far-fetched idea for now.
>
>
>> 0006: Integrate bus scan/match with EAL, without any effective driver
>
> Hard to find a right balance in patch splittng, but patch 4 and 6 are
> linked, I would squash them into one.
Yes, it is hard and sometimes there is simply no strong rationale for
splitting or merging. This is one of those cases.
My idea was that one patch _only_ introduces Bus services (structures,
functions etc) and another should enable the calls to it from EAL.
In that sense, I still think 4 and 6 should remain separate, may be
consecutive, though.
>
>
>> 0007: rte_pci_driver->probe replaced with rte_driver->probe
>
> This patch is too big, please separate in two patches: eal changes
> then ethdev/driver changes.
I don't think that can be done. One change is incomplete without the other.
Changes to all files are only for rte_pci_driver->probe to
rte_driver->probe movement. EAL changes is to allow
rte_eth_dev_pci_probe function after such a change as rte_driver->probe
has different arguments as compared to rte_pci_driver->probe. The
patches won't compile if I split.
Am I missing something?
> I almost missed that mlx4 has been broken : you moved the drv_flags
> from the mlx4 pci driver to rte_driver.
Oh! This is indeed my mistake. I will fix this right away.
>
> Why do you push back the driver object in the 'probe' method ? (idem
> rte_bus->scan).
I am assuming you are referring to rte_driver->probe().
This is being done so that implementations (specific to drivers on a
particular bus) can start extracting the rte_xxx_driver, if need be.
For example, for e1000/em_ethdev.c, rte_driver->probe() have been set to
rte_eth_dev_pci_probe() which requires rte_pci_driver to work with. In
absence of the rte_driver object, this function cannot call
rte_pci_driver->probe (for example) for driver specific operations.
>
>
>> 0008: Integrate probe of drivers with EAL
>
> This patch does nothing about "remove" while its title talks about it.
Yes, that is a mistake on my part. I will fix this.
>
> + What about hotplug code ? I suppose this is for later.
I am still working on it. Maybe, with the current set, if there is some
agreement over the overall changes, I would be more confident on this
next level of changes.
>
>
>> 0009: Split the existing PCI probe into match and probe
>
> You don't need to expose rte_eal_pci_match_default() (and I am not
> sure what the "default" means here).
> This method should be internal to the pci bus object ?
1) Yes, rte_eal_pci_match_default isn't really a 'default' I will remove
that.
2) Yes, there is no need to expose it.
One trivial reason I took this liberty was so that I can put my
implementation for PCI bus in ${RTE_SDK}/drivers/bus/pci/* rather than
current ${RTE_SDK}/lib/librte_eal/linuxapp/eal_pci.c
And similarly if there are other PCI-like implementations which would
like to have their own bus rather than using PCI bus.
But, I can remove this (from Map as well)
>
>
>> 0010: Make PCI probe/match work on rte_driver/device rather than
>> rte_pci_device/rte_pci_driver
>> 0011: Patch from Ben [8], part of series [2]
>
>
>> 0012~0013: Enable Scan/Match/probe on Bus from EAL and remove unused
>> functions and lists
>
> Same thing as earlier in the series, you don't need to check for the bus object.
> The pci bus code can't be called if the bus was not registered and
> consequently, you are sure that the pci bus object is the pci_bus
> symbol.
Hm. Ok. I will remove this.
I was doing this for two reasons 1) somehow I found drivers getting
registered *before* bus and for debugging that. and 2) it made me avoid
segfaults because of my wrong code.
Anyways, it was only a debugging level change.
>
>
>
> Regards,
>
Thanks for your review. I still look forward to more discussion on
overall approach and if there are holes which I can't see (for example,
hotplugging).
Regards,
Shreyansh
^ permalink raw reply
* Re: [PATCH 0/4] eal/common: introduce rte_memset and related test
From: Yuanhan Liu @ 2016-12-07 9:48 UTC (permalink / raw)
To: Yang, Zhiyong
Cc: Maxime Coquelin, dev@dpdk.org, Richardson, Bruce,
Ananyev, Konstantin, Pierre Pfister (ppfister)
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB4CE07@BGSMSX101.gar.corp.intel.com>
On Wed, Dec 07, 2016 at 09:43:06AM +0000, Yang, Zhiyong wrote:
> > On Wed, Dec 07, 2016 at 09:28:17AM +0000, Yang, Zhiyong wrote:
> > > > >> Wouldn't be worth to also use rte_memset in Virtio PMD (not
> > > > >> compiled/tested)? :
> > > > >>
> > > > >
> > > > > I think rte_memset maybe can bring some benefit here, but , I'm
> > > > > not clear how to enter the branch and test it. :)
> > > >
> > > > Indeed, you will need Pierre's patch:
> > > > [dpdk-dev] [PATCH] virtio: tx with can_push when VERSION_1 is set
> >
> > I will apply it shortly.
> >
> > > > Thanks,
> > > > Maxime
> > > > >
> > > Thank you Maxime.
> > > I can see a little, but not obviously performance improvement here.
> >
> > Are you you have run into that code piece? FYI, you have to enable virtio 1.0
> > explicitly, which is disabled by deafault.
>
> Yes. I use the patch from Pierre and set offload = 0 ;
I meant virtio 1.0. Have you added following options for the QEMU virtio-net
device?
disable-modern=false
--yliu
^ permalink raw reply
* Re: [PATCH 0/4] eal/common: introduce rte_memset and related test
From: Yang, Zhiyong @ 2016-12-07 9:43 UTC (permalink / raw)
To: Yuanhan Liu
Cc: Maxime Coquelin, dev@dpdk.org, Richardson, Bruce,
Ananyev, Konstantin, Pierre Pfister (ppfister)
In-Reply-To: <20161207093742.GF31182@yliu-dev.sh.intel.com>
Hi, yuanhan:
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Wednesday, December 7, 2016 5:38 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
> Richardson, Bruce <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Pierre Pfister (ppfister)
> <ppfister@cisco.com>
> Subject: Re: [dpdk-dev] [PATCH 0/4] eal/common: introduce rte_memset
> and related test
>
> On Wed, Dec 07, 2016 at 09:28:17AM +0000, Yang, Zhiyong wrote:
> > > >> Wouldn't be worth to also use rte_memset in Virtio PMD (not
> > > >> compiled/tested)? :
> > > >>
> > > >
> > > > I think rte_memset maybe can bring some benefit here, but , I'm
> > > > not clear how to enter the branch and test it. :)
> > >
> > > Indeed, you will need Pierre's patch:
> > > [dpdk-dev] [PATCH] virtio: tx with can_push when VERSION_1 is set
>
> I will apply it shortly.
>
> > > Thanks,
> > > Maxime
> > > >
> > Thank you Maxime.
> > I can see a little, but not obviously performance improvement here.
>
> Are you you have run into that code piece? FYI, you have to enable virtio 1.0
> explicitly, which is disabled by deafault.
Yes. I use the patch from Pierre and set offload = 0 ;
Thanks
Zhiyong
>
> > You know, memset(hdr, 0, head_size); only consumes fewer cycles for
> virtio pmd.
> > head_size only 10 or 12 bytes.
> > I optimize rte_memset perf further for N=8~15 bytes.
> > The main purpose of Introducing rte_memset is that we can use it to
> > avoid perf drop issue instead of glibc memset on some platform, I think.
>
> For this case (as well as the 4th patch), it's more about making sure
> rte_memset is inlined.
>
> --yliu
^ permalink raw reply
* Re: [PATCH v2] virtio: tx with can_push when VERSION_1 is set
From: Yuanhan Liu @ 2016-12-07 9:42 UTC (permalink / raw)
To: Pierre Pfister (ppfister); +Cc: dev@dpdk.org, Maxime Coquelin
In-Reply-To: <C1042323-AA70-46AF-B4D2-24A470ED8DB9@cisco.com>
On Wed, Nov 30, 2016 at 09:18:42AM +0000, Pierre Pfister (ppfister) wrote:
> Current virtio driver advertises VERSION_1 support,
> but does not handle device's VERSION_1 support when
> sending packets (it looks for ANY_LAYOUT feature,
> which is absent).
>
> This patch enables 'can_push' in tx path when VERSION_1
> is advertised by the device.
>
> This significantly improves small packets forwarding rate
> towards devices advertising VERSION_1 feature.
>
> Signed-off-by: Pierre Pfister <ppfister@cisco.com>
Applied to dpdk-next-virtio.
Thanks.
--yliu
^ permalink raw reply
* Re: [PATCH v2] doc: introduce PVP reference benchmark
From: Yuanhan Liu @ 2016-12-07 9:39 UTC (permalink / raw)
To: Maxime Coquelin
Cc: thomas.monjalon, john.mcnamara, zhiyong.yang, ktraynor, dev
In-Reply-To: <20161206122440.12039-1-maxime.coquelin@redhat.com>
On Tue, Dec 06, 2016 at 01:24:40PM +0100, Maxime Coquelin wrote:
> Having reference benchmarks is important in order to obtain
> reproducible performance figures.
>
> This patch describes required steps to configure a PVP setup
> using testpmd in both host and guest.
>
> Not relying on external vSwitch ease integration in a CI loop by
> not being impacted by DPDK API changes.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Thanks for the doc!
--yliu
^ permalink raw reply
* Re: [PATCH 0/4] eal/common: introduce rte_memset and related test
From: Yuanhan Liu @ 2016-12-07 9:37 UTC (permalink / raw)
To: Yang, Zhiyong
Cc: Maxime Coquelin, dev@dpdk.org, Richardson, Bruce,
Ananyev, Konstantin, Pierre Pfister (ppfister)
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB4CDD0@BGSMSX101.gar.corp.intel.com>
On Wed, Dec 07, 2016 at 09:28:17AM +0000, Yang, Zhiyong wrote:
> > >> Wouldn't be worth to also use rte_memset in Virtio PMD (not
> > >> compiled/tested)? :
> > >>
> > >
> > > I think rte_memset maybe can bring some benefit here, but , I'm not
> > > clear how to enter the branch and test it. :)
> >
> > Indeed, you will need Pierre's patch:
> > [dpdk-dev] [PATCH] virtio: tx with can_push when VERSION_1 is set
I will apply it shortly.
> > Thanks,
> > Maxime
> > >
> Thank you Maxime.
> I can see a little, but not obviously performance improvement here.
Are you you have run into that code piece? FYI, you have to enable
virtio 1.0 explicitly, which is disabled by deafault.
> You know, memset(hdr, 0, head_size); only consumes fewer cycles for virtio pmd.
> head_size only 10 or 12 bytes.
> I optimize rte_memset perf further for N=8~15 bytes.
> The main purpose of Introducing rte_memset is that we can use it
> to avoid perf drop issue instead of glibc memset on some platform, I think.
For this case (as well as the 4th patch), it's more about making sure
rte_memset is inlined.
--yliu
^ permalink raw reply
* Re: [PATCH 0/4] eal/common: introduce rte_memset and related test
From: Yang, Zhiyong @ 2016-12-07 9:28 UTC (permalink / raw)
To: Maxime Coquelin, dev@dpdk.org
Cc: yuanhan.liu@linux.intel.com, Richardson, Bruce,
Ananyev, Konstantin, Pierre Pfister (ppfister)
In-Reply-To: <e354fc0a-14b5-0f59-d95b-7b8fd6f47b7a@redhat.com>
Hi, Maxime:
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Tuesday, December 6, 2016 4:30 PM
> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
> Cc: yuanhan.liu@linux.intel.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Pierre Pfister (ppfister)
> <ppfister@cisco.com>
> Subject: Re: [dpdk-dev] [PATCH 0/4] eal/common: introduce rte_memset
> and related test
>
>
>
> On 12/06/2016 07:33 AM, Yang, Zhiyong wrote:
> > Hi， Maxime:
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >> Sent: Friday, December 2, 2016 6:01 PM
> >> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org
> >> Cc: yuanhan.liu@linux.intel.com; Richardson, Bruce
> >> <bruce.richardson@intel.com>; Ananyev, Konstantin
> >> <konstantin.ananyev@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH 0/4] eal/common: introduce rte_memset
> >> and related test
> >>
> >> Hi Zhiyong,
> >>
> >> On 12/05/2016 09:26 AM, Zhiyong Yang wrote:
> >>> DPDK code has met performance drop badly in some case when calling
> >>> glibc function memset. Reference to discussions about memset in
> >>> http://dpdk.org/ml/archives/dev/2016-October/048628.html
> >>> It is necessary to introduce more high efficient function to fix it.
> >>> One important thing about rte_memset is that we can get clear
> >>> control on what instruction flow is used.
> >>>
> >>> This patchset introduces rte_memset to bring more high efficient
> >>> implementation, and will bring obvious perf improvement, especially
> >>> for small N bytes in the most application scenarios.
> >>>
> >>> Patch 1 implements rte_memset in the file rte_memset.h on IA
> >>> platform The file supports three types of instruction sets including
> >>> sse & avx (128bits), avx2(256bits) and avx512(512bits). rte_memset
> >>> makes use of vectorization and inline function to improve the perf
> >>> on IA. In addition, cache line and memory alignment are fully taken
> >>> into
> >> consideration.
> >>>
> >>> Patch 2 implements functional autotest to validates the function
> >>> whether to work in a right way.
> >>>
> >>> Patch 3 implements performance autotest separately in cache and
> memory.
> >>>
> >>> Patch 4 Using rte_memset instead of copy_virtio_net_hdr can bring
> >>> 3%~4% performance improvements on IA platform from virtio/vhost
> >>> non-mergeable loopback testing.
> >>>
> >>> Zhiyong Yang (4):
> >>> eal/common: introduce rte_memset on IA platform
> >>> app/test: add functional autotest for rte_memset
> >>> app/test: add performance autotest for rte_memset
> >>> lib/librte_vhost: improve vhost perf using rte_memset
> >>>
> >>> app/test/Makefile | 3 +
> >>> app/test/test_memset.c | 158 +++++++++
> >>> app/test/test_memset_perf.c | 348
> +++++++++++++++++++
> >>> doc/guides/rel_notes/release_17_02.rst | 11 +
> >>> .../common/include/arch/x86/rte_memset.h | 376
> >> +++++++++++++++++++++
> >>> lib/librte_eal/common/include/generic/rte_memset.h | 51 +++
> >>> lib/librte_vhost/virtio_net.c | 18 +-
> >>> 7 files changed, 958 insertions(+), 7 deletions(-) create mode
> >>> 100644 app/test/test_memset.c create mode 100644
> >>> app/test/test_memset_perf.c create mode 100644
> >>> lib/librte_eal/common/include/arch/x86/rte_memset.h
> >>> create mode 100644
> >> lib/librte_eal/common/include/generic/rte_memset.h
> >>>
> >>
> >> Thanks for the series, idea looks good to me.
> >>
> >> Wouldn't be worth to also use rte_memset in Virtio PMD (not
> >> compiled/tested)? :
> >>
> >
> > I think rte_memset maybe can bring some benefit here, but , I'm not
> > clear how to enter the branch and test it. :)
>
> Indeed, you will need Pierre's patch:
> [dpdk-dev] [PATCH] virtio: tx with can_push when VERSION_1 is set
>
> Thanks,
> Maxime
> >
Thank you Maxime.
I can see a little, but not obviously performance improvement here.
You know, memset(hdr, 0, head_size); only consumes fewer cycles for virtio pmd.
head_size only 10 or 12 bytes.
I optimize rte_memset perf further for N=8~15 bytes.
The main purpose of Introducing rte_memset is that we can use it
to avoid perf drop issue instead of glibc memset on some platform, I think.
> >
> >> diff --git a/drivers/net/virtio/virtio_rxtx.c
> >> b/drivers/net/virtio/virtio_rxtx.c
> >> index 22d97a4..a5f70c4 100644
> >> --- a/drivers/net/virtio/virtio_rxtx.c
> >> +++ b/drivers/net/virtio/virtio_rxtx.c
> >> @@ -287,7 +287,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq,
> >> struct rte_mbuf *cookie,
> >> rte_pktmbuf_prepend(cookie, head_size);
> >> /* if offload disabled, it is not zeroed below, do it now */
> >> if (offload == 0)
> >> - memset(hdr, 0, head_size);
> >> + rte_memset(hdr, 0, head_size);
> >> } else if (use_indirect) {
> >> /* setup tx ring slot to point to indirect
> >> * descriptor list stored in reserved region.
> >>
> >> Cheers,
> >> Maxime
^ permalink raw reply
* [PATCH v2] eal: optimize aligned rte_memcpy
From: Zhihong Wang @ 2016-12-07 1:31 UTC (permalink / raw)
To: dev; +Cc: yuanhan.liu, thomas.monjalon, lei.a.yao, Zhihong Wang
In-Reply-To: <1480641582-56186-1-git-send-email-zhihong.wang@intel.com>
This patch optimizes rte_memcpy for well aligned cases, where both
dst and src addr are aligned to maximum MOV width. It introduces a
dedicated function called rte_memcpy_aligned to handle the aligned
cases with simplified instruction stream. The existing rte_memcpy
is renamed as rte_memcpy_generic. The selection between them 2 is
done at the entry of rte_memcpy.
The existing rte_memcpy is for generic cases, it handles unaligned
copies and make store aligned, it even makes load aligned for micro
architectures like Ivy Bridge. However alignment handling comes at
a price: It adds extra load/store instructions, which can cause
complications sometime.
DPDK Vhost memcpy with Mergeable Rx Buffer feature as an example:
The copy is aligned, and remote, and there is header write along
which is also remote. In this case the memcpy instruction stream
should be simplified, to reduce extra load/store, therefore reduce
the probability of load/store buffer full caused pipeline stall, to
let the actual memcpy instructions be issued and let H/W prefetcher
goes to work as early as possible.
This patch is tested on Ivy Bridge, Haswell and Skylake, it provides
up to 20% gain for Virtio Vhost PVP traffic, with packet size ranging
from 64 to 1500 bytes.
The test can also be conducted without NIC, by setting loopback
traffic between Virtio and Vhost. For example, modify the macro
TXONLY_DEF_PACKET_LEN to the requested packet size in testpmd.h,
rebuild and start testpmd in both host and guest, then "start" on
one side and "start tx_first 32" on the other.
Signed-off-by: Zhihong Wang <zhihong.wang@intel.com>
---
.../common/include/arch/x86/rte_memcpy.h | 81 +++++++++++++++++++++-
1 file changed, 78 insertions(+), 3 deletions(-)
diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
index b3bfc23..b9785e8 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
@@ -69,6 +69,8 @@ rte_memcpy(void *dst, const void *src, size_t n) __attribute__((always_inline));
#ifdef RTE_MACHINE_CPUFLAG_AVX512F
+#define ALIGNMENT_MASK 0x3F
+
/**
* AVX512 implementation below
*/
@@ -189,7 +191,7 @@ rte_mov512blocks(uint8_t *dst, const uint8_t *src, size_t n)
}
static inline void *
-rte_memcpy(void *dst, const void *src, size_t n)
+rte_memcpy_generic(void *dst, const void *src, size_t n)
{
uintptr_t dstu = (uintptr_t)dst;
uintptr_t srcu = (uintptr_t)src;
@@ -308,6 +310,8 @@ COPY_BLOCK_128_BACK63:
#elif defined RTE_MACHINE_CPUFLAG_AVX2
+#define ALIGNMENT_MASK 0x1F
+
/**
* AVX2 implementation below
*/
@@ -387,7 +391,7 @@ rte_mov128blocks(uint8_t *dst, const uint8_t *src, size_t n)
}
static inline void *
-rte_memcpy(void *dst, const void *src, size_t n)
+rte_memcpy_generic(void *dst, const void *src, size_t n)
{
uintptr_t dstu = (uintptr_t)dst;
uintptr_t srcu = (uintptr_t)src;
@@ -499,6 +503,8 @@ COPY_BLOCK_128_BACK31:
#else /* RTE_MACHINE_CPUFLAG */
+#define ALIGNMENT_MASK 0x0F
+
/**
* SSE & AVX implementation below
*/
@@ -677,7 +683,7 @@ __extension__ ({ \
})
static inline void *
-rte_memcpy(void *dst, const void *src, size_t n)
+rte_memcpy_generic(void *dst, const void *src, size_t n)
{
__m128i xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7, xmm8;
uintptr_t dstu = (uintptr_t)dst;
@@ -821,6 +827,75 @@ COPY_BLOCK_64_BACK15:
#endif /* RTE_MACHINE_CPUFLAG */
+static inline void *
+rte_memcpy_aligned(void *dst, const void *src, size_t n)
+{
+ void *ret = dst;
+
+ /* Copy size <= 16 bytes */
+ if (n < 16) {
+ if (n & 0x01) {
+ *(uint8_t *)dst = *(const uint8_t *)src;
+ src = (const uint8_t *)src + 1;
+ dst = (uint8_t *)dst + 1;
+ }
+ if (n & 0x02) {
+ *(uint16_t *)dst = *(const uint16_t *)src;
+ src = (const uint16_t *)src + 1;
+ dst = (uint16_t *)dst + 1;
+ }
+ if (n & 0x04) {
+ *(uint32_t *)dst = *(const uint32_t *)src;
+ src = (const uint32_t *)src + 1;
+ dst = (uint32_t *)dst + 1;
+ }
+ if (n & 0x08)
+ *(uint64_t *)dst = *(const uint64_t *)src;
+
+ return ret;
+ }
+
+ /* Copy 16 <= size <= 32 bytes */
+ if (n <= 32) {
+ rte_mov16((uint8_t *)dst, (const uint8_t *)src);
+ rte_mov16((uint8_t *)dst - 16 + n,
+ (const uint8_t *)src - 16 + n);
+
+ return ret;
+ }
+
+ /* Copy 32 < size <= 64 bytes */
+ if (n <= 64) {
+ rte_mov32((uint8_t *)dst, (const uint8_t *)src);
+ rte_mov32((uint8_t *)dst - 32 + n,
+ (const uint8_t *)src - 32 + n);
+
+ return ret;
+ }
+
+ /* Copy 64 bytes blocks */
+ for (; n >= 64; n -= 64) {
+ rte_mov64((uint8_t *)dst, (const uint8_t *)src);
+ dst = (uint8_t *)dst + 64;
+ src = (const uint8_t *)src + 64;
+ }
+
+ /* Copy whatever left */
+ rte_mov64((uint8_t *)dst - 64 + n,
+ (const uint8_t *)src - 64 + n);
+
+ return ret;
+}
+
+static inline void *
+rte_memcpy(void *dst, const void *src, size_t n)
+{
+ if (!(((uintptr_t)dst | (uintptr_t)src) & ALIGNMENT_MASK))
+ return rte_memcpy_aligned(dst, src, n);
+ else
+ return rte_memcpy_generic(dst, src, n);
+}
+
#ifdef __cplusplus
}
#endif
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 11/32] net/dpaa2: add dpaa2 vfio support
From: Thomas Monjalon @ 2016-12-07 8:38 UTC (permalink / raw)
To: Hemant Agrawal; +Cc: dev, bruce.richardson, shreyansh.jain
In-Reply-To: <b0db98e6-793a-b2d5-02bb-3d0a9fc5f30e@nxp.com>
2016-12-07 12:30, Hemant Agrawal:
> On 12/7/2016 2:34 AM, Thomas Monjalon wrote:
> > 2016-12-04 23:47, Hemant Agrawal:
> >> Add support for using VFIO for dpaa2 based fsl-mc bus.
> >
> > Why do we need so much special code for interfacing VFIO on fsl-mc?
> > Can you reuse some code from EAL VFIO?
> >
>
> fsl-mc VFIO scans the objects. So, it is slightly different.
>
> Even though I have tried to minimize changes and re-use the code from
> EAL VFIO. It is still refactoring work, which we will take up little later.
Do you mean you could re-use some EAL code but do not want to do it now?
It sounds like something heard too many times earlier (from others).
The experience tells we must not wait to do things right.
^ permalink raw reply
* Re: [PATCH 1/8] drivers/common/dpaa2: Run time assembler for Descriptor formation
From: Thomas Monjalon @ 2016-12-07 8:33 UTC (permalink / raw)
To: Akhil Goyal
Cc: dev, declan.doherty, pablo.de.lara.guarch, Hemant Agrawal,
Horia Geantă
In-Reply-To: <DB5PR0401MB17681276E03FC9DD87C16FB9E6850@DB5PR0401MB1768.eurprd04.prod.outlook.com>
2016-12-07 06:24, Akhil Goyal:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> 2016-12-05 18:25, Akhil Goyal:
> > FLib is a library which helps in making the descriptors which is
> > understood by NXP's SEC hardware.
> > This patch provides header files for command words which can be used
> > for descritptor formation.
>
> It seems this code is old. Does it exist as a standalone library somewhere?
> Where was it hosted before duplicating it in DPDK?
>
> Why do you want to have a common directory drivers/common/dpaa2/flib instead of a sub-directory in the crypto driver?
>
> [Akhil] This is not really a library. This is a set of header files which is required for compilation. We have 2 other cypto drivers (for different platforms viz: Non-DPAA and DPAA1_QORIQ) which uses the same flib. So we put it in common directory. We plan to send patches for other drivers in the upcoming releases.
Please Akhil, could you answer to the three questions?
^ permalink raw reply
* Does DPDK 1.7.1 support jumboframes for VMXNET3
From: Kanika Singhal @ 2016-12-07 8:18 UTC (permalink / raw)
To: dev
Hi,
I am a new to DPDK.
My product is using DPDK 1.6 and it does not support jumbo frames for
VMXNET3.
I have to upgrade to 1.7.1, but i am not able to find info on whether the
jumbo frame support is being added for VMXNET3 or not.
I can see some jumbo frame related patches being posted for vmxnet3
Do I need to apply the patches in 1.7.1 to make jumbo work/Is there any
DPDK version which supports jumbo?
Thanks,
Kanika
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Yuanhan Liu @ 2016-12-07 8:11 UTC (permalink / raw)
To: Ferruh Yigit, Olivier Matz
Cc: Thomas Monjalon, dev, Jan Medala, Jakub Palider, Netanel Belgazal,
Evgeny Schemeilin, Alejandro Lucero, Yong Wang, Andrew Rybchenko,
Hemant Agrawal, Tomasz Kulasek, konstantin.ananyev
In-Reply-To: <2dfc10d2-bb19-1385-aa01-97e44a3db139@intel.com>
On Tue, Dec 06, 2016 at 03:53:42PM +0000, Ferruh Yigit wrote:
> > Please, we need a comment for each driver saying
> > "it is OK, we do not need any checksum preparation for TSO"
> > or
> > "yes we have to implement tx_prepare or TSO will not work in this mode"
> >
Sorry for late. For virtio, I think it's not a must. The checksum stuff
has been handled inside the Tx function. However, we may could move it
to tx_prepare, which would actually recover the performance lost
introduced while enabling TSO for the non-TSO case.
--yliu
^ permalink raw reply
* Re: [PATCH v12 0/6] add Tx preparation
From: Andrew Rybchenko @ 2016-12-07 7:55 UTC (permalink / raw)
To: Ferruh Yigit, Thomas Monjalon, dev, Jan Medala, Jakub Palider,
Netanel Belgazal, Evgeny Schemeilin, Alejandro Lucero,
Yuanhan Liu, Yong Wang, Hemant Agrawal
Cc: Tomasz Kulasek, konstantin.ananyev
In-Reply-To: <2dfc10d2-bb19-1385-aa01-97e44a3db139@intel.com>
On 12/06/2016 06:53 PM, Ferruh Yigit wrote:
> On 11/28/2016 11:03 AM, Thomas Monjalon wrote:
>> We need attention of every PMD developers on this thread.
>>
>> Reminder of what Konstantin suggested:
>> "
>> - if the PMD supports TX offloads AND
>> - if to be able use any of these offloads the upper layer SW would have to:
>> * modify the contents of the packet OR
>> * obey HW specific restrictions
>> then it is a PMD developer responsibility to provide tx_prep() that would implement
>> expected modifications of the packet contents and restriction checks.
>> Otherwise, tx_prep() implementation is not required and can be safely set to NULL.
>> "
>>
>> I copy/paste also my previous conclusion:
>>
>> Before txprep, there is only one API: the application must prepare the
>> packets checksum itself (get_psd_sum in testpmd).
>> With txprep, the application have 2 choices: keep doing the job itself
>> or call txprep which calls a PMD-specific function.
>> The question is: does non-Intel drivers need a checksum preparation for TSO?
>> Will it behave well if txprep does nothing in these drivers?
>>
>> When looking at the code, most of drivers handle the TSO flags.
>> But it is hard to know whether they rely on the pseudo checksum or not.
>>
>> git grep -l 'PKT_TX_UDP_CKSUM\|PKT_TX_TCP_CKSUM\|PKT_TX_TCP_SEG' drivers/net/
>>
>> drivers/net/bnxt/bnxt_txr.c
>> drivers/net/cxgbe/sge.c
>> drivers/net/e1000/em_rxtx.c
>> drivers/net/e1000/igb_rxtx.c
>> drivers/net/ena/ena_ethdev.c
>> drivers/net/enic/enic_rxtx.c
>> drivers/net/fm10k/fm10k_rxtx.c
>> drivers/net/i40e/i40e_rxtx.c
>> drivers/net/ixgbe/ixgbe_rxtx.c
>> drivers/net/mlx4/mlx4.c
>> drivers/net/mlx5/mlx5_rxtx.c
>> drivers/net/nfp/nfp_net.c
>> drivers/net/qede/qede_rxtx.c
>> drivers/net/thunderx/nicvf_rxtx.c
>> drivers/net/virtio/virtio_rxtx.c
>> drivers/net/vmxnet3/vmxnet3_rxtx.c
>>
>> Please, we need a comment for each driver saying
>> "it is OK, we do not need any checksum preparation for TSO"
>> or
>> "yes we have to implement tx_prepare or TSO will not work in this mode"
>>
> Still waiting response from PMDs:
> - ena
> - nfp
> - virtio
>
> Waiting clarification for preparation requirements:
> - vmxnet3
>
> Also including new PMDs to the thread:
> - sfc
The patch which adds TSO support is
http://dpdk.org/dev/patchwork/patch/17417/
We use l2/l3/l4 header length. We do NOT use prepared pseudo header
checksum and
HW does NOT need it.
Andrew.
> - dpaa2
^ permalink raw reply
* Re: [PATCH] vhost: allow for many vhost user ports
From: Yuanhan Liu @ 2016-12-07 7:43 UTC (permalink / raw)
To: Jan Wickbom; +Cc: huawei.xie@intel.com, dev@dpdk.org, Patrik Andersson R
In-Reply-To: <AM4PR0701MB214617221B10D1390754E5F194820@AM4PR0701MB2146.eurprd07.prod.outlook.com>
On Tue, Dec 06, 2016 at 11:42:36AM +0000, Jan Wickbom wrote:
> > > @@ -189,7 +206,7 @@
> > > pfdset->fd[i].fd = -1;
> > > pfdset->fd[i].rcb = pfdset-
> > >fd[i].wcb = NULL;
> > > pfdset->fd[i].dat = NULL;
> > > - pfdset->num--;
> > > + (void) fdset_adjust_num(pfdset);
> >
> > Unncessary cast.
>
> I'd like to keep the cast. The function returns int and it's nice to show we deliberately
> don't care.
I know your point, but it's not needed.
> No strong opinion though, but we should do the same everywhere. Please
> see below.
>
> >
> > > i = -1;
> > > }
> > > pthread_mutex_unlock(&pfdset->fd_mutex);
> > > @@ -211,12 +228,12 @@
> > >
> > > pfdset->fd[index].fd = -1;
> > > pfdset->fd[index].rcb = pfdset->fd[index].wcb = NULL;
> > > - pfdset->fd[index].dat = NULL;
> > > - pfdset->num--;
> > > + (void) fdset_adjust_num(pfdset);
>
> If we remove the cast above, we should remove this one as well.
Yes.
--yliu
^ permalink raw reply
* Re: [PATCH 11/32] net/dpaa2: add dpaa2 vfio support
From: Hemant Agrawal @ 2016-12-07 7:00 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, bruce.richardson, shreyansh.jain
In-Reply-To: <4516168.6vIN5TbTdY@xps13>
On 12/7/2016 2:34 AM, Thomas Monjalon wrote:
> 2016-12-04 23:47, Hemant Agrawal:
>> Add support for using VFIO for dpaa2 based fsl-mc bus.
>
> Why do we need so much special code for interfacing VFIO on fsl-mc?
> Can you reuse some code from EAL VFIO?
>
fsl-mc VFIO scans the objects. So, it is slightly different.
Even though I have tried to minimize changes and re-use the code from
EAL VFIO. It is still refactoring work, which we will take up little later.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox