DPDK-dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/7] net/mlx5: prepare Tx vectorization
From: Nelio Laranjeiro @ 2016-11-24 16:03 UTC (permalink / raw)
  To: dev; +Cc: Thomas Monjalon, Adrien Mazarguil, Elad Persiko
In-Reply-To: <cover.1479995764.git.nelio.laranjeiro@6wind.com>

Prepare the code to write the Work Queue Element with vectorized
instructions.

Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Signed-off-by: Elad Persiko <eladpe@mellanox.com>
Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
 drivers/net/mlx5/mlx5_rxtx.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index ffd09ac..5dacd93 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -391,6 +391,8 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 		uint32_t length;
 		unsigned int ds = 0;
 		uintptr_t addr;
+		uint16_t pkt_inline_sz = MLX5_WQE_DWORD_SIZE;
+		uint8_t ehdr[2];
 #ifdef MLX5_PMD_SOFT_COUNTERS
 		uint32_t total_length = 0;
 #endif
@@ -416,6 +418,8 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			rte_prefetch0(*pkts);
 		addr = rte_pktmbuf_mtod(buf, uintptr_t);
 		length = DATA_LEN(buf);
+		ehdr[0] = ((uint8_t *)addr)[0];
+		ehdr[1] = ((uint8_t *)addr)[1];
 #ifdef MLX5_PMD_SOFT_COUNTERS
 		total_length = length;
 #endif
@@ -439,24 +443,20 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 		} else {
 			wqe->eseg.cs_flags = 0;
 		}
-		raw  = (uint8_t *)(uintptr_t)&wqe->eseg.inline_hdr[0];
-		/* Start the know and common part of the WQE structure. */
-		wqe->ctrl[0] = htonl((txq->wqe_ci << 8) | MLX5_OPCODE_SEND);
-		wqe->ctrl[2] = 0;
-		wqe->ctrl[3] = 0;
-		wqe->eseg.rsvd0 = 0;
-		wqe->eseg.rsvd1 = 0;
-		wqe->eseg.mss = 0;
-		wqe->eseg.rsvd2 = 0;
-		/* Start by copying the Ethernet Header. */
-		memcpy((uint8_t *)raw, ((uint8_t *)addr), 16);
+		raw = ((uint8_t *)(uintptr_t)wqe) + 2 * MLX5_WQE_DWORD_SIZE;
+		/*
+		 * Start by copying the Ethernet header minus the first two
+		 * bytes which will be appended at the end of the Ethernet
+		 * segment.
+		 */
+		memcpy((uint8_t *)raw, ((uint8_t *)addr) + 2, 16);
 		length -= MLX5_WQE_DWORD_SIZE;
 		addr += MLX5_WQE_DWORD_SIZE;
 		/* Replace the Ethernet type by the VLAN if necessary. */
 		if (buf->ol_flags & PKT_TX_VLAN_PKT) {
 			uint32_t vlan = htonl(0x81000000 | buf->vlan_tci);
 
-			memcpy((uint8_t *)(raw + MLX5_WQE_DWORD_SIZE -
+			memcpy((uint8_t *)(raw + MLX5_WQE_DWORD_SIZE - 2 -
 					   sizeof(vlan)),
 			       &vlan, sizeof(vlan));
 			addr -= sizeof(vlan);
@@ -468,10 +468,13 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 				(uintptr_t)&(*txq->wqes)[1 << txq->wqe_n];
 			uint16_t max_inline =
 				txq->max_inline * RTE_CACHE_LINE_SIZE;
-			uint16_t pkt_inline_sz = MLX5_WQE_DWORD_SIZE;
 			uint16_t room;
 
-			raw += MLX5_WQE_DWORD_SIZE;
+			/*
+			 * raw starts two bytes before the boundary to
+			 * continue the above copy of packet data.
+			 */
+			raw += MLX5_WQE_DWORD_SIZE - 2;
 			room = end - (uintptr_t)raw;
 			if (room > max_inline) {
 				uintptr_t addr_end = (addr + max_inline) &
@@ -487,8 +490,6 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 				/* Sanity check. */
 				assert(addr <= addr_end);
 			}
-			/* Store the inlined packet size in the WQE. */
-			wqe->eseg.inline_hdr_sz = htons(pkt_inline_sz);
 			/*
 			 * 2 DWORDs consumed by the WQE header + 1 DSEG +
 			 * the size of the inline part of the packet.
@@ -570,7 +571,18 @@ mlx5_tx_burst(void *dpdk_txq, struct rte_mbuf **pkts, uint16_t pkts_n)
 			--pkts_n;
 next_pkt:
 		++i;
+		/* Initialize known and common part of the WQE structure. */
+		wqe->ctrl[0] = htonl((txq->wqe_ci << 8) | MLX5_OPCODE_SEND);
 		wqe->ctrl[1] = htonl(txq->qp_num_8s | ds);
+		wqe->ctrl[2] = 0;
+		wqe->ctrl[3] = 0;
+		wqe->eseg.rsvd0 = 0;
+		wqe->eseg.rsvd1 = 0;
+		wqe->eseg.mss = 0;
+		wqe->eseg.rsvd2 = 0;
+		wqe->eseg.inline_hdr_sz = htons(pkt_inline_sz);
+		wqe->eseg.inline_hdr[0] = ehdr[0];
+		wqe->eseg.inline_hdr[1] = ehdr[1];
 		txq->wqe_ci += (ds + 3) / 4;
 #ifdef MLX5_PMD_SOFT_COUNTERS
 		/* Increment sent bytes counter. */
-- 
2.1.4

^ permalink raw reply related

* [PATCH 0/7] net/mlx5: improve single core performance
From: Nelio Laranjeiro @ 2016-11-24 16:03 UTC (permalink / raw)
  To: dev; +Cc: Thomas Monjalon, Adrien Mazarguil

This series applies on top of
"[PATCH] eal: define generic vector types" [1][2]

Using built-in vector types forces compilers to consider SIMD instructions in
specific places in order to improve performance on both IBM POWER8 and Intel
architectures.

For example, testpmd single-thread I/O forwarding packets per second
performance is improved by 6% on Intel platforms.

 [1] http://dpdk.org/ml/archives/dev/2016-November/050261.html
 [2] http://dpdk.org/dev/patchwork/patch/17024/

Nelio Laranjeiro (7):
  net/mlx5: prepare Tx vectorization
  net/mlx5: use work queue buffer as a raw buffer
  net/mlx5: use vector types to speed up processing
  net/mlx5: fix missing inline attributes
  net/mlx5: move static prototype
  net/mlx5: optimize copy of Ethernet header
  net/mlx5: remove inefficient prefetching

 drivers/net/mlx5/mlx5_prm.h  |  20 +++-
 drivers/net/mlx5/mlx5_rxtx.c | 243 +++++++++++++++++++++++--------------------
 drivers/net/mlx5/mlx5_rxtx.h |   2 +-
 drivers/net/mlx5/mlx5_txq.c  |   8 +-
 4 files changed, 150 insertions(+), 123 deletions(-)

-- 
2.1.4

^ permalink raw reply

* Re: [PATCH 01/56] net/sfc: libefx-based PMD stub sufficient to build and init
From: Andrew Rybchenko @ 2016-11-24 15:59 UTC (permalink / raw)
  To: Ferruh Yigit, dev
In-Reply-To: <a96a2795-82d4-7c75-3917-da61afa16f8b@intel.com>

On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> The PMD is put into the sfc/efx subdirectory to have a place for
>> the second PMD and library shared by both.
>>
>> Enable the PMD by default on supported configuratons.
>>
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   MAINTAINERS                                     |   6 ++
>>   config/common_base                              |   6 ++
>>   config/defconfig_arm-armv7a-linuxapp-gcc        |   1 +
>>   config/defconfig_arm64-armv8a-linuxapp-gcc      |   1 +
>>   config/defconfig_i686-native-linuxapp-gcc       |   5 +
>>   config/defconfig_i686-native-linuxapp-icc       |   5 +
>>   config/defconfig_ppc_64-power8-linuxapp-gcc     |   1 +
>>   config/defconfig_tile-tilegx-linuxapp-gcc       |   1 +
>>   config/defconfig_x86_64-native-linuxapp-icc     |   5 +
>>   config/defconfig_x86_x32-native-linuxapp-gcc    |   5 +
>>   doc/guides/nics/features/sfc_efx.ini            |  10 ++
>>   doc/guides/nics/index.rst                       |   1 +
>>   doc/guides/nics/sfc_efx.rst                     | 109 +++++++++++++++++++++
> Can you also update release notes please, to announce new driver.

Thanks, will do in v2.

> <...>
>
>> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
>> new file mode 100644
>> index 0000000..71f07ca
>> --- /dev/null
>> +++ b/drivers/net/sfc/efx/Makefile
>> @@ -0,0 +1,81 @@
> <...>
>> +
>> +include $(RTE_SDK)/mk/rte.vars.mk
>> +
>> +#
>> +# library name
>> +#
>> +LIB = librte_pmd_sfc_efx.a
>> +
>> +CFLAGS += -O3
>> +
>> +# Enable basic warnings but disable some which are accepted
>> +CFLAGS += -Wall
> It is possible to use $(WERROR_FLAGS), which set automatically based on
> selected compiler. See mk/toolchain/* .

Thanks, will do in v2.

> And you can add extra options here, please keep in mind that there are
> three compiler supported right now: gcc, clang and icc. You may require
> to add compiler and version checks..

I've tried to disable the driver build on ICC since we've never tested it.
I've failed to find list of compiler versions which must/should be checked.
I've tested versions which come with RHEL 7.2, Debian Jessie and Sid.
(In v1 I've lost my fixes for clang which produce warnings because of
unsupported -W option)

>> +CFLAGS += -Wno-strict-aliasing
>> +
>> +# Enable extra warnings but disable some which are accepted
>> +CFLAGS += -Wextra
>> +CFLAGS += -Wno-empty-body
>> +CFLAGS += -Wno-sign-compare
>> +CFLAGS += -Wno-type-limits
>> +CFLAGS += -Wno-unused-parameter
> Is there a way to not disable these warnings but fix in the source code?
> Or move to CFLAGS_BASE_DRIVER, if the reason is the base driver?

Will do in v2.

>> +
>> +# More warnings not enabled by above aggregators
>> +CFLAGS += -Waggregate-return
>> +CFLAGS += -Wbad-function-cast
>> +CFLAGS += -Wcast-qual
>> +CFLAGS += -Wdisabled-optimization
>> +CFLAGS += -Wmissing-declarations
>> +CFLAGS += -Wmissing-prototypes
>> +CFLAGS += -Wnested-externs
>> +CFLAGS += -Wold-style-definition
>> +CFLAGS += -Wpointer-arith
>> +CFLAGS += -Wstrict-prototypes
>> +CFLAGS += -Wundef
>> +CFLAGS += -Wwrite-strings
> If you believe some can be useful for everybody, please feel free to add
> to mk/toolchain/* .

I'll definitely remove duplicates which are already included in 
$(WERROR_FLAGS).
I'd prefer to keep the rest just here for now. I think that adding it 
world-wide
requires testing on really many compiler versions etc.

>> +
>> +EXPORT_MAP := rte_pmd_sfc_efx_version.map
>> +
>> +LIBABIVER := 1
>> +
>> +#
>> +# all source are stored in SRCS-y
>> +#
>> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_ethdev.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += sfc_kvargs.c
>> +
>> +
>> +# this lib depends upon:
>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_eal
>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_kvargs
>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_ether
>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mempool
>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_SFC_EFX_PMD) += lib/librte_mbuf
>> +
>> +include $(RTE_SDK)/mk/rte.lib.mk
>> diff --git a/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
>> new file mode 100644
>> index 0000000..1901bcb
>> --- /dev/null
>> +++ b/drivers/net/sfc/efx/rte_pmd_sfc_efx_version.map
>> @@ -0,0 +1,4 @@
>> +DPDK_16.07 {
> Now this become 17.02

Thanks, will fix in v2.

>> +
>> +	local: *;
>> +};
>> diff --git a/drivers/net/sfc/efx/sfc.h b/drivers/net/sfc/efx/sfc.h
>> new file mode 100644
>> index 0000000..16fd2bb
>> --- /dev/null
>> +++ b/drivers/net/sfc/efx/sfc.h
>> @@ -0,0 +1,53 @@
> <..>
>> +
>> +#ifndef _SFC_H
>> +#define	_SFC_H
> s/^I/ /
> This also exists in other locations and files..

Will fix in v2.
I thought that DPDK prefers TAB after #define as FreeBSD does, but 
counting shows that space is really preferred.
I think that such things should be caught by checkpatch.

> <...>

^ permalink raw reply

* Re: [PATCH 30/56] net/sfc: include libefx in build
From: Andrew Rybchenko @ 2016-11-24 15:44 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: Artem Andreev
In-Reply-To: <18cbe35d-ee10-961b-f6a9-abfb0232c974@intel.com>

See one question below.

On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> From: Artem Andreev <Artem.Andreev@oktetlabs.ru>
>>
>> Implement efsys.h for the PMD.
>>
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> Signed-off-by: Artem Andreev <Artem.Andreev@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   drivers/net/sfc/efx/Makefile |  54 +++
>>   drivers/net/sfc/efx/efsys.h  | 767 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 821 insertions(+)
>>   create mode 100644 drivers/net/sfc/efx/efsys.h
>>
>> diff --git a/drivers/net/sfc/efx/Makefile b/drivers/net/sfc/efx/Makefile
>> index 71f07ca..de95ea8 100644
>> --- a/drivers/net/sfc/efx/Makefile
>> +++ b/drivers/net/sfc/efx/Makefile
>> @@ -33,6 +33,8 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>   #
>>   LIB = librte_pmd_sfc_efx.a
>>   
>> +CFLAGS += -I$(SRCDIR)/base/
>> +CFLAGS += -I$(SRCDIR)
>>   CFLAGS += -O3
>>   
>>   # Enable basic warnings but disable some which are accepted
>> @@ -60,6 +62,17 @@ CFLAGS += -Wstrict-prototypes
>>   CFLAGS += -Wundef
>>   CFLAGS += -Wwrite-strings
>>   
>> +# Extra CFLAGS for base driver files
>> +CFLAGS_BASE_DRIVER += -Wno-unused-variable
>> +CFLAGS_BASE_DRIVER += -Wno-unused-but-set-variable
> clang complain about this one:
> warning: unknown warning option '-Wno-unused-but-set-variable'; did you
> mean '-Wno-unused-const-variable'? [-Wunknown-warning-option]

Will fix in v2

>> +
>> +#
>> +# List of base driver object files for which
>> +# special CFLAGS above should be applied
>> +#
>> +BASE_DRIVER_OBJS=$(patsubst %.c,%.o,$(notdir $(wildcard $(SRCDIR)/base/*.c)))
>> +$(foreach obj, $(BASE_DRIVER_OBJS), $(eval CFLAGS+=$(CFLAGS_BASE_DRIVER)))
> This cause multiple "-Wno-unused-variable -Wno-unused-but-set-variable"
> params in final command, I guess the intention is:
>
> $(foreach obj, $(BASE_DRIVER_OBJS), $(eval
> CFLAGS_$(obj)+=$(CFLAGS_BASE_DRIVER)))
>
> Fixing this may generate a few compiler warnings.

Many thanks, will fix in v2.

> <...>
>
>> diff --git a/drivers/net/sfc/efx/efsys.h b/drivers/net/sfc/efx/efsys.h
>> new file mode 100644
>> index 0000000..2eef996
>> --- /dev/null
>> +++ b/drivers/net/sfc/efx/efsys.h
>> @@ -0,0 +1,767 @@
> <...>
>
> I guess below is hardcoded compile time configuration for libefx, do you
> think does it make sense to document this default configuration?

Yes, it is libefx configuration and more options will be enabled when 
corresponding
functionality is supported in the PMD.
I'm sorry, but I don't understand what would you like to see in the 
documentation.
Could you clarify, please?

>> +
>> +#define	EFSYS_OPT_NAMES 0
>> +
>> +#define	EFSYS_OPT_SIENA 0
>> +#define	EFSYS_OPT_HUNTINGTON 1
>> +#define	EFSYS_OPT_MEDFORD 1
>> +#ifdef RTE_LIBRTE_SFC_EFX_DEBUG
>> +#define	EFSYS_OPT_CHECK_REG 1
>> +#else
>> +#define	EFSYS_OPT_CHECK_REG 0
>> +#endif
>> +
>> +#define	EFSYS_OPT_MCDI 1
>> +#define	EFSYS_OPT_MCDI_LOGGING 0
>> +#define	EFSYS_OPT_MCDI_PROXY_AUTH 0
>> +
>> +#define	EFSYS_OPT_MAC_STATS 0
>> +
>> +#define	EFSYS_OPT_LOOPBACK 0
>> +
>> +#define	EFSYS_OPT_MON_MCDI 0
>> +#define	EFSYS_OPT_MON_STATS 0
>> +
>> +#define	EFSYS_OPT_PHY_STATS 0
>> +#define	EFSYS_OPT_BIST 0
>> +#define	EFSYS_OPT_PHY_LED_CONTROL 0
>> +#define	EFSYS_OPT_PHY_FLAGS 0
>> +
>> +#define	EFSYS_OPT_VPD 0
>> +#define	EFSYS_OPT_NVRAM 0
>> +#define	EFSYS_OPT_BOOTCFG 0
>> +
>> +#define	EFSYS_OPT_DIAG 0
>> +#define	EFSYS_OPT_RX_SCALE 0
>> +#define	EFSYS_OPT_QSTATS 0
>> +#define	EFSYS_OPT_FILTER 1
>> +#define	EFSYS_OPT_RX_SCATTER 0
>> +
>> +#define	EFSYS_OPT_EV_PREFETCH 0
>> +
>> +#define	EFSYS_OPT_DECODE_INTR_FATAL 0
>> +
>> +#define	EFSYS_OPT_LICENSING 0
>> +
>> +#define	EFSYS_OPT_ALLOW_UNCONFIGURED_NIC 0
>> +
>> +#define	EFSYS_OPT_RX_PACKED_STREAM 0
> <...>

^ permalink raw reply

* [virtio] virtio-net PMD cannot be used on Ubuntu 16.10
From: Hobywan Kenoby @ 2016-11-24 15:44 UTC (permalink / raw)
  To: dev@dpdk.org

Hello,

While I (almost) never had a problem with testpmd, Ubuntu 16.10 make it fail:

$ sudo ./testpmd -c 0x3 -n1 --no-huge -- --disable-hw-vlan --disable-rss -i --rxq=1 --txq=1 --rxd=256 --txd=256
EAL: Detected 4 lcore(s)
EAL: Probing VFIO support...
EAL: WARNING: cpu flags constant_tsc=yes nonstop_tsc=no -> using unreliable clock cycles !
EAL: PCI device 0000:00:08.0 on NUMA socket -1
EAL:   probe driver: 1af4:1000 net_virtio
EAL: Error - exiting with code: 1
  Cause: Requested device 0000:00:08.0 cannot be used


This boils down to virtio_pci.c

	if (!check_vq_phys_addr_ok(vq))
		return -1;

which fails because vq is 0x7ffff7172700 and is greater than 16TB.


is there an issue or do I configure something badly?
I used the same method to test on 14.04 15.10 and 16.04 without problems.
Huge pages are configured and are used by DPDK.

-HK

^ permalink raw reply

* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Thomas Monjalon @ 2016-11-24 15:35 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: dev, bruce.richardson, harry.van.haaren, hemant.agrawal,
	gage.eads
In-Reply-To: <20161124015912.GA13508@svelivela-lt.caveonetworks.com>

2016-11-24 07:29, Jerin Jacob:
> On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:
> > 2016-11-18 11:14, Jerin Jacob:
> > > +Eventdev API - EXPERIMENTAL
> > > +M: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > +F: lib/librte_eventdev/
> > 
> > OK to mark it experimental.
> > What is the plan to remove the experimental word?
> 
> IMO, EXPERIMENTAL status can be changed when
> - At least two event drivers available(Intel and Cavium are working on
>   SW and HW event drivers)
> - Functional test applications are fine with at least two drivers
> - Portable example application to showcase the features of the library
> - eventdev integration with another dpdk subsystem such as ethdev
> 
> Thoughts?. I am not sure the criteria used in cryptodev case.

Sounds good.
We will be more confident when drivers and tests will be implemented.

I think the roadmap for the SW driver targets the release 17.05.
Do you still plan 17.02 for this API and the Cavium driver?

> > > +#define EVENTDEV_NAME_SKELETON_PMD event_skeleton
> > > +/**< Skeleton event device PMD name */
> > 
> > I do not understand this #define.
> 
> Applications can explicitly request the a specific driver though driver
> name. This will go as argument to rte_event_dev_get_dev_id(const char *name).
> The reason for keeping this #define in rte_eventdev.h is that,
> application needs to include only rte_eventdev.h not rte_eventdev_pmd.h.

So each driver must register its name in the API?
Is it really needed?

> > > +struct rte_event_dev_config {
> > > +	uint32_t dequeue_wait_ns;
> > > +	/**< rte_event_dequeue() wait for *dequeue_wait_ns* ns on this device.
> > 
> > Please explain exactly when the wait occurs and why.
> 
> Here is the explanation from rte_event_dequeue() API definition,
> -
> @param wait
> 0 - no-wait, returns immediately if there is no event.
> >0 - wait for the event, if the device is configured with
> RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT then this function will wait until
> the event available or *wait* time.
> if the device is not configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT
> then this function will wait until the event available or *dequeue_wait_ns*
>                                                       ^^^^^^^^^^^^^^^^^^^^^^
> ns which was previously supplied to rte_event_dev_configure()
> -
> This is provides the application to have control over, how long the
> implementation should wait if event is not available.
> 
> Let me know what exact changes are required if details are not enough in
> rte_event_dequeue() API definition.

Maybe that timeout would be a better name.
It waits only if there is nothing in the queue.
It can be interesting to highlight in this comment that this parameter
makes the dequeue function a blocking call.

> > > +/** Event port configuration structure */
> > > +struct rte_event_port_conf {
> > > +	int32_t new_event_threshold;
> > > +	/**< A backpressure threshold for new event enqueues on this port.
> > > +	 * Use for *closed system* event dev where event capacity is limited,
> > > +	 * and cannot exceed the capacity of the event dev.
> > > +	 * Configuring ports with different thresholds can make higher priority
> > > +	 * traffic less likely to  be backpressured.
> > > +	 * For example, a port used to inject NIC Rx packets into the event dev
> > > +	 * can have a lower threshold so as not to overwhelm the device,
> > > +	 * while ports used for worker pools can have a higher threshold.
> > > +	 * This value cannot exceed the *nb_events_limit*
> > > +	 * which previously supplied to rte_event_dev_configure()
> > > +	 */
> > > +	uint8_t dequeue_depth;
> > > +	/**< Configure number of bulk dequeues for this event port.
> > > +	 * This value cannot exceed the *nb_event_port_dequeue_depth*
> > > +	 * which previously supplied to rte_event_dev_configure()
> > > +	 */
> > > +	uint8_t enqueue_depth;
> > > +	/**< Configure number of bulk enqueues for this event port.
> > > +	 * This value cannot exceed the *nb_event_port_enqueue_depth*
> > > +	 * which previously supplied to rte_event_dev_configure()
> > > +	 */
> > > +};
> > 
> > The depth configuration is not clear to me.
> 
> Basically the maximum number of events can be enqueued/dequeued at time
> from a given event port. depth of one == non burst mode.

OK so depth is the queue size. Please could you reword?

> > > +/* Event types to classify the event source */
> > 
> > Why this classification is needed?
> 
> This for application pipeling and the cases like, if application wants to know which
> subsystem generated the event.
> 
> example packet forwarding loop on the worker cores:
> while(1) {
> 	ev = dequeue()
> 	// event from ethdev subsystem
> 	if (ev.event_type == RTE_EVENT_TYPE_ETHDEV) {
> 		- swap the mac address
> 		- push to atomic queue for ingress flow order maintenance
> 		  by CORE
> 	/* events from core */
> 	} else if (ev.event_type == RTE_EVENT_TYPE_CORE) {
> 
> 	}
> 	enqueue(ev);
> }

I don't know why but I feel this classification is weak.
You need to track the source of the event. Does it make sense to go beyond
and identify the source device?

> > > +#define RTE_EVENT_TYPE_ETHDEV           0x0
> > > +/**< The event generated from ethdev subsystem */
> > > +#define RTE_EVENT_TYPE_CRYPTODEV        0x1
> > > +/**< The event generated from crypodev subsystem */
> > > +#define RTE_EVENT_TYPE_TIMERDEV         0x2
> > > +/**< The event generated from timerdev subsystem */
> > > +#define RTE_EVENT_TYPE_CORE             0x3
> > > +/**< The event generated from core.
> > 
> > What is core?
> 
> The event are generated by lcore for pipeling. Any suggestion for
> better name? lcore?

What about CPU or SW?

> > > +		/**< Opaque event pointer */
> > > +		struct rte_mbuf *mbuf;
> > > +		/**< mbuf pointer if dequeued event is associated with mbuf */
> > 
> > How do we know that an event is associated with mbuf?
> 
> By looking at the event source/type RTE_EVENT_TYPE_*
> 
> > Does it mean that such events are always converted into mbuf even if the
> > application does not need it?
> 
> Hardware has dependency on getting physical address of the event, so any
> struct that has "phys_addr_t buf_physaddr" works.

I do not understand.

I tought that decoding the event would be the responsibility of the app
by calling a function like
rte_eventdev_convert_to_mbuf(struct rte_event *, struct rte_mbuf *).

> > > +struct rte_eventdev_driver;
> > > +struct rte_eventdev_ops;
> > 
> > I think it is better to split API and driver interface in two files.
> > (we should do this split in ethdev)
> 
> I thought so, but then the "static inline" versions of northbound
> API(like rte_event_enqueue) will go another file(due to the fact that
> implementation need to deference "dev->data->ports[port_id]"). Do you want that way?
> I would like to keep all northbound API in rte_eventdev.h and not any of them
> in rte_eventdev_pmd.h.

My comment was confusing.
You are doing 2 files, one for API (what you call northbound I think)
and the other one for driver interface (what you call southbound I think),
it's very fine.

> > > +/**
> > > + * Enqueue the event object supplied in the *rte_event* structure on an
> > > + * event device designated by its *dev_id* through the event port specified by
> > > + * *port_id*. The event object specifies the event queue on which this
> > > + * event will be enqueued.
> > > + *
> > > + * @param dev_id
> > > + *   Event device identifier.
> > > + * @param port_id
> > > + *   The identifier of the event port.
> > > + * @param ev
> > > + *   Pointer to struct rte_event
> > > + *
> > > + * @return
> > > + *  - 0 on success
> > > + *  - <0 on failure. Failure can occur if the event port's output queue is
> > > + *     backpressured, for instance.
> > > + */
> > > +static inline int
> > > +rte_event_enqueue(uint8_t dev_id, uint8_t port_id, struct rte_event *ev)
> > 
> > Is it really needed to have non-burst variant of enqueue/dequeue?
> 
> Yes. certain HW can work only with non burst variants.

Same comment as Bruce, we must keep only the burst variant.
We cannot have different API for different HW.

> > > +/**
> > > + * Converts nanoseconds to *wait* value for rte_event_dequeue()
> > > + *
> > > + * If the device is configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT flag then
> > > + * application can use this function to convert wait value in nanoseconds to
> > > + * implementations specific wait value supplied in rte_event_dequeue()
> > 
> > Why is it implementation-specific?
> > Why this conversion is not internal in the driver?
> 
> This is for performance optimization, otherwise in drivers
> need to convert ns to ticks in "fast path"

So why not defining the unit of this timeout as CPU cycles like the ones
returned by rte_get_timer_cycles()?

^ permalink raw reply

* Re: dpdk/vpp and cross-version migration for vhost
From: Kavanagh, Mark B @ 2016-11-24 15:24 UTC (permalink / raw)
  To: Kevin Traynor, Maxime Coquelin, Yuanhan Liu, Weglicki, MichalX
  Cc: Michael S. Tsirkin, dev@dpdk.org, Stephen Hemminger,
	qemu-devel@nongnu.org, libvir-list@redhat.com,
	vpp-dev@lists.fd.io, Marc-André Lureau
In-Reply-To: <f6133f8c-860f-c3a6-0138-73411586c99d@redhat.com>

>
>On 11/24/2016 12:47 PM, Maxime Coquelin wrote:
>>
>>
>> On 11/24/2016 01:33 PM, Yuanhan Liu wrote:
>>> On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote:
>>>> > On 11/24/2016 06:31 AM, Yuanhan Liu wrote:
>>>>> > > On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote:
>>>>>>>> > >>>> You keep assuming that you have the VM started first and
>>>>>>>> > >>>> figure out things afterwards, but this does not work.
>>>>>>>> > >>>>
>>>>>>>> > >>>> Think about a cluster of machines. You want to start a VM in
>>>>>>>> > >>>> a way that will ensure compatibility with all hosts
>>>>>>>> > >>>> in a cluster.
>>>>>>> > >>>
>>>>>>> > >>> I see. I was more considering about the case when the dst
>>>>>>> > >>> host (including the qemu and dpdk combo) is given, and
>>>>>>> > >>> then determine whether it will be a successfull migration
>>>>>>> > >>> or not.
>>>>>>> > >>>
>>>>>>> > >>> And you are asking that we need to know which host could
>>>>>>> > >>> be a good candidate before starting the migration. In such
>>>>>>> > >>> case, we indeed need some inputs from both the qemu and
>>>>>>> > >>> vhost-user backend.
>>>>>>> > >>>
>>>>>>> > >>> For DPDK, I think it could be simple, just as you said, it
>>>>>>> > >>> could be either a tiny script, or even a macro defined in
>>>>>>> > >>> the source code file (we extend it every time we add a
>>>>>>> > >>> new feature) to let the libvirt to read it. Or something
>>>>>>> > >>> else.
>>>>>> > >>
>>>>>> > >> There's the issue of APIs that tweak features as Maxime
>>>>>> > >> suggested.
>>>>> > >
>>>>> > > Yes, it's a good point.
>>>>> > >
>>>>>> > >> Maybe the only thing to do is to deprecate it,
>>>>> > >
>>>>> > > Looks like so.
>>>>> > >
>>>>>> > >> but I feel some way for application to pass info into
>>>>>> > >> guest might be benefitial.
>>>>> > >
>>>>> > > The two APIs are just for tweaking feature bits DPDK supports
>>>>> before
>>>>> > > any device got connected. It's another way to disable some features
>>>>> > > (the another obvious way is to through QEMU command lines).
>>>>> > >
>>>>> > > IMO, it's bit handy only in a case like: we have bunch of VMs.
>>>>> Instead
>>>>> > > of disabling something though qemu one by one, we could disable it
>>>>> > > once in DPDK.
>>>>> > >
>>>>> > > But I doubt the useful of it. It's only used in DPDK's vhost
>>>>> example
>>>>> > > after all. Nor is it used in vhost pmd, neither is it used in OVS.
>>>> >
>>>> > rte_vhost_feature_disable() is currently used in OVS,
>>>> lib/netdev-dpdk.c
>>> Hmmm. I must have checked very old code ...
>>>> >
>>>> > netdev_dpdk_vhost_class_init(void)
>>>> > {
>>>> >     static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>>>> >
>>>> >     /* This function can be called for different classes.  The
>>>> > initialization
>>>> >      * needs to be done only once */
>>>> >     if (ovsthread_once_start(&once)) {
>>>> >         rte_vhost_driver_callback_register(&virtio_net_device_ops);
>>>> >         rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
>>>> >                                   | 1ULL << VIRTIO_NET_F_HOST_TSO6
>>>> >                                   | 1ULL << VIRTIO_NET_F_CSUM);
>>> I saw the commit introduced such change, but it tells no reason why
>>> it was added.
>>
>> I'm also interested to know the reason.
>
>I can't remember off hand, added Mark K or Michal W who should be able
>to shed some light on it.

DPDK v16.04 added support for vHost User TSO; as such, by default, TSO is advertised to guest devices as an available feature during feature negotiation with QEMU.
However, while the vHost user backend sets up the majority of the mbuf fields that are required for TSO, there is still a reliance on the associated DPDK application (i.e. in this case OvS-DPDK) to set the remaining flags and/or offsets. Since OvS-DPDK doesn't currently provide that functionality, it is necessary to explicitly disable TSO; otherwise, undefined behaviour will ensue.

>
>> In any case, I think this is something that can/should be managed by
>> the management tool, which  should disable it in cmd parameters.
>>
>> Kevin, do you agree?
>
>I think best to find out the reason first. Because if no reason to
>disable in the code, then no need to debate!
>
>>
>> Cheers,
>> Maxime

^ permalink raw reply

* Re: [PATCH 31/56] net/sfc: implement dummy callback to get device information
From: Andrew Rybchenko @ 2016-11-24 15:05 UTC (permalink / raw)
  To: Ferruh Yigit, dev
In-Reply-To: <8d244f7f-59e1-bbe8-9df4-45bff3e3e5cc@intel.com>

On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> Just a stub to be filled in when corresponding functionality is
>> implemented.
> What about merging this stub with real implementation?
> Or perhaps replace with code that adds dummy .dev_configure?

Thanks, I like the second idea. Will do in v2.

>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   drivers/net/sfc/efx/sfc_ethdev.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/sfc/efx/sfc_ethdev.c b/drivers/net/sfc/efx/sfc_ethdev.c
>> index ff20a13..0deff07 100644
>> --- a/drivers/net/sfc/efx/sfc_ethdev.c
>> +++ b/drivers/net/sfc/efx/sfc_ethdev.c
>> @@ -37,9 +37,16 @@
>>   #include "sfc_kvargs.h"
>>   
>>   
>> +static void
>> +sfc_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>> +{
>> +	struct sfc_adapter *sa = dev->data->dev_private;
>> +
>> +	sfc_log_init(sa, "entry");
>> +}
>> +
>>   static const struct eth_dev_ops sfc_eth_dev_ops = {
>> -	/* Just dummy init to avoid build-time warning */
>> -	.dev_configure			= NULL,
>> +	.dev_infos_get			= sfc_dev_infos_get,
>>   };
>>   
>>   static int
>>

^ permalink raw reply

* Re: dpdk/vpp and cross-version migration for vhost
From: Kevin Traynor @ 2016-11-24 15:01 UTC (permalink / raw)
  To: Maxime Coquelin, Yuanhan Liu, Kavanagh, Mark B, michalx.weglicki
  Cc: Michael S. Tsirkin, dev, Stephen Hemminger, qemu-devel,
	libvir-list, vpp-dev, Marc-André Lureau
In-Reply-To: <0f83baa0-206e-4af2-1a97-d59c8e0582b8@redhat.com>

On 11/24/2016 12:47 PM, Maxime Coquelin wrote:
> 
> 
> On 11/24/2016 01:33 PM, Yuanhan Liu wrote:
>> On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote:
>>> > On 11/24/2016 06:31 AM, Yuanhan Liu wrote:
>>>> > > On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote:
>>>>>>> > >>>> You keep assuming that you have the VM started first and
>>>>>>> > >>>> figure out things afterwards, but this does not work.
>>>>>>> > >>>>
>>>>>>> > >>>> Think about a cluster of machines. You want to start a VM in
>>>>>>> > >>>> a way that will ensure compatibility with all hosts
>>>>>>> > >>>> in a cluster.
>>>>>> > >>>
>>>>>> > >>> I see. I was more considering about the case when the dst
>>>>>> > >>> host (including the qemu and dpdk combo) is given, and
>>>>>> > >>> then determine whether it will be a successfull migration
>>>>>> > >>> or not.
>>>>>> > >>>
>>>>>> > >>> And you are asking that we need to know which host could
>>>>>> > >>> be a good candidate before starting the migration. In such
>>>>>> > >>> case, we indeed need some inputs from both the qemu and
>>>>>> > >>> vhost-user backend.
>>>>>> > >>>
>>>>>> > >>> For DPDK, I think it could be simple, just as you said, it
>>>>>> > >>> could be either a tiny script, or even a macro defined in
>>>>>> > >>> the source code file (we extend it every time we add a
>>>>>> > >>> new feature) to let the libvirt to read it. Or something
>>>>>> > >>> else.
>>>>> > >>
>>>>> > >> There's the issue of APIs that tweak features as Maxime
>>>>> > >> suggested.
>>>> > >
>>>> > > Yes, it's a good point.
>>>> > >
>>>>> > >> Maybe the only thing to do is to deprecate it,
>>>> > >
>>>> > > Looks like so.
>>>> > >
>>>>> > >> but I feel some way for application to pass info into
>>>>> > >> guest might be benefitial.
>>>> > >
>>>> > > The two APIs are just for tweaking feature bits DPDK supports
>>>> before
>>>> > > any device got connected. It's another way to disable some features
>>>> > > (the another obvious way is to through QEMU command lines).
>>>> > >
>>>> > > IMO, it's bit handy only in a case like: we have bunch of VMs.
>>>> Instead
>>>> > > of disabling something though qemu one by one, we could disable it
>>>> > > once in DPDK.
>>>> > >
>>>> > > But I doubt the useful of it. It's only used in DPDK's vhost
>>>> example
>>>> > > after all. Nor is it used in vhost pmd, neither is it used in OVS.
>>> >
>>> > rte_vhost_feature_disable() is currently used in OVS,
>>> lib/netdev-dpdk.c
>> Hmmm. I must have checked very old code ...
>>> >
>>> > netdev_dpdk_vhost_class_init(void)
>>> > {
>>> >     static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>>> >
>>> >     /* This function can be called for different classes.  The
>>> > initialization
>>> >      * needs to be done only once */
>>> >     if (ovsthread_once_start(&once)) {
>>> >         rte_vhost_driver_callback_register(&virtio_net_device_ops);
>>> >         rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
>>> >                                   | 1ULL << VIRTIO_NET_F_HOST_TSO6
>>> >                                   | 1ULL << VIRTIO_NET_F_CSUM);
>> I saw the commit introduced such change, but it tells no reason why
>> it was added.
> 
> I'm also interested to know the reason.

I can't remember off hand, added Mark K or Michal W who should be able
to shed some light on it.

> In any case, I think this is something that can/should be managed by
> the management tool, which  should disable it in cmd parameters.
> 
> Kevin, do you agree?

I think best to find out the reason first. Because if no reason to
disable in the code, then no need to debate!

> 
> Cheers,
> Maxime

^ permalink raw reply

* Re: [PATCH 32/56] net/sfc: implement driver operation to init device on attach
From: Andrew Rybchenko @ 2016-11-24 14:58 UTC (permalink / raw)
  To: Ferruh Yigit, dev
In-Reply-To: <76a60072-2c7b-b674-ca7f-1dc9ca66815a@intel.com>

On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> The setup and configuration of the PMD is not performance sensitive,
>> but is not thread safe either. It is possible that the multiple
>> read/writes during PMD setup and configuration could be corrupted
>> in a multi-thread environment.
> Right, this is not thread-safe, but this is valid for all PMDs, it is
> not expected to have initialization in multi-threaded environment, that
> said so, synchronization also won't hurt, as you said this is not fast
> path, just may not be necessary.

In fact further patches really need the lock and it should be introduced and
maintained from the very beginning. I'll add comments in v2 to explain it.

>> Since this is not performance
>> sensitive, the developer can choose to add their own layer to provide
>> thread-safe setup and configuration. It is expected that, in most
>> applications, the initial configuration of the network ports would be
>> done by a single thread at startup.
>>
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> <...>

Thanks,
Andrew.

^ permalink raw reply

* Re: [PATCH 33/56] net/sfc: add device configure and close stubs
From: Andrew Rybchenko @ 2016-11-24 14:46 UTC (permalink / raw)
  To: Ferruh Yigit, dev
In-Reply-To: <d802d81b-2206-1c5d-8844-9cde4f832469@intel.com>

On 11/23/2016 06:26 PM, Ferruh Yigit wrote:
> On 11/21/2016 3:00 PM, Andrew Rybchenko wrote:
>> Reviewed-by: Andy Moreton <amoreton@solarflare.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
> <...>
>
>> diff --git a/drivers/net/sfc/efx/sfc.h b/drivers/net/sfc/efx/sfc.h
>> index 01d652d..d040f98 100644
>> --- a/drivers/net/sfc/efx/sfc.h
>> +++ b/drivers/net/sfc/efx/sfc.h
> <...>
>> @@ -131,7 +147,7 @@ sfc_adapter_unlock(struct sfc_adapter *sa)
>>   }
>>   
>>   static inline void
>> -sfc_adapter_lock_destroy(struct sfc_adapter *sa)
>> +sfc_adapter_lock_fini(struct sfc_adapter *sa)
> Why not do proper naming in 32/56 at first place?

Thanks, will be fixed in v2.

> <...>

^ permalink raw reply

* Re: [RFC 2/9] ethdev: move queue id check in generic layer
From: Olivier Matz @ 2016-11-24 13:05 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: thomas.monjalon, konstantin.ananyev, wenzhuo.lu, helin.zhang
In-Reply-To: <174b8455-3806-26a1-38bd-edb756c101d0@intel.com>

Hi Ferruh,

On Thu, 2016-11-24 at 10:59 +0000, Ferruh Yigit wrote:
> On 11/24/2016 9:54 AM, Olivier Matz wrote:
> > The check of queue_id is done in all drivers implementing
> > rte_eth_rx_queue_count(). Factorize this check in the generic
> > function.
> > 
> > Note that the nfp driver was doing the check differently, which
> > could
> > induce crashes if the queue index was too big.
> > 
> > By the way, also move the is_supported test before the port valid
> > and
> > queue valid test.
> > 
> > PR=52423
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> > Acked-by: Ivan Boule <ivan.boule@6wind.com>
> > ---
> 
> <...>
> 
> > diff --git a/lib/librte_ether/rte_ethdev.h
> > b/lib/librte_ether/rte_ethdev.h
> > index c3edc23..9551cfd 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2693,7 +2693,7 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t
> > queue_id,
> >   *  The queue id on the specific port.
> >   * @return
> >   *  The number of used descriptors in the specific queue, or:
> > - *     (-EINVAL) if *port_id* is invalid
> > + *     (-EINVAL) if *port_id* or *queue_id* is invalid
> >   *     (-ENOTSUP) if the device does not support this function
> >   */
> >  static inline int
> > @@ -2701,8 +2701,10 @@ rte_eth_rx_queue_count(uint8_t port_id,
> > uint16_t queue_id)
> >  {
> >  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> >  
> > -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count,
> > -ENOTSUP);
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> 
> Doing port validity check before accessing dev->dev_ops-
> >rx_queue_count
> can be good idea.
> 
> What about validating port_id even before accessing
> rte_eth_devices[port_id]?
> 

oops right, we should not move this line, it's stupid...

Thanks for the feedback,
Olivier

^ permalink raw reply

* Re: dpdk/vpp and cross-version migration for vhost
From: Maxime Coquelin @ 2016-11-24 12:47 UTC (permalink / raw)
  To: Yuanhan Liu, Kevin Traynor
  Cc: Michael S. Tsirkin, dev, Stephen Hemminger, qemu-devel,
	libvir-list, vpp-dev, Marc-André Lureau
In-Reply-To: <20161124123304.GG5048@yliu-dev.sh.intel.com>



On 11/24/2016 01:33 PM, Yuanhan Liu wrote:
> On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote:
>> > On 11/24/2016 06:31 AM, Yuanhan Liu wrote:
>>> > > On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote:
>>>>>> > >>>> You keep assuming that you have the VM started first and
>>>>>> > >>>> figure out things afterwards, but this does not work.
>>>>>> > >>>>
>>>>>> > >>>> Think about a cluster of machines. You want to start a VM in
>>>>>> > >>>> a way that will ensure compatibility with all hosts
>>>>>> > >>>> in a cluster.
>>>>> > >>>
>>>>> > >>> I see. I was more considering about the case when the dst
>>>>> > >>> host (including the qemu and dpdk combo) is given, and
>>>>> > >>> then determine whether it will be a successfull migration
>>>>> > >>> or not.
>>>>> > >>>
>>>>> > >>> And you are asking that we need to know which host could
>>>>> > >>> be a good candidate before starting the migration. In such
>>>>> > >>> case, we indeed need some inputs from both the qemu and
>>>>> > >>> vhost-user backend.
>>>>> > >>>
>>>>> > >>> For DPDK, I think it could be simple, just as you said, it
>>>>> > >>> could be either a tiny script, or even a macro defined in
>>>>> > >>> the source code file (we extend it every time we add a
>>>>> > >>> new feature) to let the libvirt to read it. Or something
>>>>> > >>> else.
>>>> > >>
>>>> > >> There's the issue of APIs that tweak features as Maxime
>>>> > >> suggested.
>>> > >
>>> > > Yes, it's a good point.
>>> > >
>>>> > >> Maybe the only thing to do is to deprecate it,
>>> > >
>>> > > Looks like so.
>>> > >
>>>> > >> but I feel some way for application to pass info into
>>>> > >> guest might be benefitial.
>>> > >
>>> > > The two APIs are just for tweaking feature bits DPDK supports before
>>> > > any device got connected. It's another way to disable some features
>>> > > (the another obvious way is to through QEMU command lines).
>>> > >
>>> > > IMO, it's bit handy only in a case like: we have bunch of VMs. Instead
>>> > > of disabling something though qemu one by one, we could disable it
>>> > > once in DPDK.
>>> > >
>>> > > But I doubt the useful of it. It's only used in DPDK's vhost example
>>> > > after all. Nor is it used in vhost pmd, neither is it used in OVS.
>> >
>> > rte_vhost_feature_disable() is currently used in OVS, lib/netdev-dpdk.c
> Hmmm. I must have checked very old code ...
>> >
>> > netdev_dpdk_vhost_class_init(void)
>> > {
>> >     static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> >
>> >     /* This function can be called for different classes.  The
>> > initialization
>> >      * needs to be done only once */
>> >     if (ovsthread_once_start(&once)) {
>> >         rte_vhost_driver_callback_register(&virtio_net_device_ops);
>> >         rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
>> >                                   | 1ULL << VIRTIO_NET_F_HOST_TSO6
>> >                                   | 1ULL << VIRTIO_NET_F_CSUM);
> I saw the commit introduced such change, but it tells no reason why
> it was added.

I'm also interested to know the reason.
In any case, I think this is something that can/should be managed by
the management tool, which  should disable it in cmd parameters.

Kevin, do you agree?

Cheers,
Maxime

^ permalink raw reply

* Re: [PATCH] doc: introduce PVP reference benchmark
From: Maxime Coquelin @ 2016-11-24 12:39 UTC (permalink / raw)
  To: Kevin Traynor, yuanhan.liu, thomas.monjalon, john.mcnamara,
	zhiyong.yang, dev
  Cc: fbaudin
In-Reply-To: <ef9f7089-a61e-5e78-cc81-b209ae8b60f8@redhat.com>



On 11/24/2016 12:58 PM, Kevin Traynor wrote:
> On 11/23/2016 09:00 PM, 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>
>
> A short template/hint of the main things to report after running could
> be useful to help ML discussions about results e.g.
>
> Traffic Generator: IXIA
> Acceptable Loss: 100% (i.e. raw throughput test)
> DPDK version/commit: v16.11
> QEMU version/commit: v2.7.0
> Patches applied: <link to patchwork>
> CPU: E5-2680 v3, 2.8GHz
> Result: x mpps
> NIC: ixgbe 82599

Good idea, I'll add a section in the end providing this template.

>
>> ---
>>  doc/guides/howto/img/pvp_2nics.svg           | 556 +++++++++++++++++++++++++++
>>  doc/guides/howto/index.rst                   |   1 +
>>  doc/guides/howto/pvp_reference_benchmark.rst | 389 +++++++++++++++++++
>>  3 files changed, 946 insertions(+)
>>  create mode 100644 doc/guides/howto/img/pvp_2nics.svg
>>  create mode 100644 doc/guides/howto/pvp_reference_benchmark.rst
>>
>
> <snip>
>
>> +Host tuning
>> +~~~~~~~~~~~
>
> I would add turbo boost =disabled on BIOS.
>
+1, will be in next revision.

>> +
>> +#. Append these options to Kernel command line:
>> +
>> +   .. code-block:: console
>> +
>> +    intel_pstate=disable mce=ignore_ce default_hugepagesz=1G hugepagesz=1G hugepages=6 isolcpus=2-7 rcu_nocbs=2-7 nohz_full=2-7 iommu=pt intel_iommu=on
>> +
>> +#. Disable hyper-threads at runtime if necessary and BIOS not accessible:
>> +
>> +   .. code-block:: console
>> +
>> +    cat /sys/devices/system/cpu/cpu*[0-9]/topology/thread_siblings_list \
>> +        | sort | uniq \
>> +        | awk -F, '{system("echo 0 > /sys/devices/system/cpu/cpu"$2"/online")}'
>> +
>> +#. Disable NMIs:
>> +
>> +   .. code-block:: console
>> +
>> +    echo 0 > /proc/sys/kernel/nmi_watchdog
>> +
>> +#. Exclude isolated CPUs from the writeback cpumask:
>> +
>> +   .. code-block:: console
>> +
>> +    echo ffffff03 > /sys/bus/workqueue/devices/writeback/cpumask
>> +
>> +#. Isolate CPUs from IRQs:
>> +
>> +   .. code-block:: console
>> +
>> +    clear_mask=0xfc #Isolate CPU2 to CPU7 from IRQs
>> +    for i in /proc/irq/*/smp_affinity
>> +    do
>> +     echo "obase=16;$(( 0x$(cat $i) & ~$clear_mask ))" | bc > $i
>> +    done
>> +
>> +Qemu build
>> +~~~~~~~~~~
>> +
>> +   .. code-block:: console
>> +
>> +    git clone git://dpdk.org/dpdk
>> +    cd dpdk
>> +    export RTE_SDK=$PWD
>> +    make install T=x86_64-native-linuxapp-gcc DESTDIR=install
>> +
>> +DPDK build
>> +~~~~~~~~~~
>> +
>> +   .. code-block:: console
>> +
>> +    git clone git://dpdk.org/dpdk
>> +    cd dpdk
>> +    export RTE_SDK=$PWD
>> +    make install T=x86_64-native-linuxapp-gcc DESTDIR=install
>> +
>> +Testpmd launch
>> +~~~~~~~~~~~~~~
>> +
>> +#. Assign NICs to DPDK:
>> +
>> +   .. code-block:: console
>> +
>> +    modprobe vfio-pci
>> +    $RTE_SDK/install/sbin/dpdk-devbind -b vfio-pci 0000:11:00.0 0000:11:00.1
>> +
>> +*Note: Sandy Bridge family seems to have some limitations wrt its IOMMU,
>> +giving poor performance results. To achieve good performance on these machines,
>> +consider using UIO instead.*
>> +
>> +#. Launch testpmd application:
>> +
>> +   .. code-block:: console
>> +
>> +    $RTE_SDK/install/bin/testpmd -l 0,2,3,4,5 --socket-mem=1024 -n 4 \
>> +        --vdev 'net_vhost0,iface=/tmp/vhost-user1' \
>> +        --vdev 'net_vhost1,iface=/tmp/vhost-user2' -- \
>> +        --portmask=f --disable-hw-vlan -i --rxq=1 --txq=1
>> +        --nb-cores=4 --forward-mode=io
>> +
>> +#. In testpmd interactive mode, set the portlist to obtin the right chaining:
>> +
>> +   .. code-block:: console
>> +
>> +    set portlist 0,2,1,3
>> +    start
>> +
>> +VM launch
>> +~~~~~~~~~
>> +
>> +The VM may be launched ezither by calling directly QEMU, or by using libvirt.
>
> s/ezither/either
>
>> +
>> +#. Qemu way:
>> +
>> +Launch QEMU with two Virtio-net devices paired to the vhost-user sockets created by testpmd:
>> +
>> +   .. code-block:: console
>> +
>> +    <QEMU path>/bin/x86_64-softmmu/qemu-system-x86_64 \
>> +        -enable-kvm -cpu host -m 3072 -smp 3 \
>> +        -chardev socket,id=char0,path=/tmp/vhost-user1 \
>> +        -netdev type=vhost-user,id=mynet1,chardev=char0,vhostforce \
>> +        -device virtio-net-pci,netdev=mynet1,mac=52:54:00:02:d9:01,addr=0x10 \
>> +        -chardev socket,id=char1,path=/tmp/vhost-user2 \
>> +        -netdev type=vhost-user,id=mynet2,chardev=char1,vhostforce \
>> +        -device virtio-net-pci,netdev=mynet2,mac=52:54:00:02:d9:02,addr=0x11 \
>> +        -object memory-backend-file,id=mem,size=3072M,mem-path=/dev/hugepages,share=on \
>> +        -numa node,memdev=mem -mem-prealloc \
>> +        -net user,hostfwd=tcp::1002$1-:22 -net nic \
>> +        -qmp unix:/tmp/qmp.socket,server,nowait \
>> +        -monitor stdio <vm_image>.qcow2
>
> Probably mergeable rx data path =off would want to be tested also when
> evaluating any performance improvements/regressions.
Right, I'll add few lines about this.

>
>> +
>> +You can use this qmp-vcpu-pin script to pin vCPUs:
>> +
>> +   .. code-block:: python
>> +
>> +    #!/usr/bin/python
>> +    # QEMU vCPU pinning tool
>> +    #
>> +    # Copyright (C) 2016 Red Hat Inc.
>> +    #
>> +    # Authors:
>> +    #  Maxime Coquelin <maxime.coquelin@redhat.com>
>> +    #
>> +    # This work is licensed under the terms of the GNU GPL, version 2.  See
>> +    # the COPYING file in the top-level directory
>> +    import argparse
>> +    import json
>> +    import os
>> +
>> +    from subprocess import call
>> +    from qmp import QEMUMonitorProtocol
>> +
>> +    pinned = []
>> +
>> +    parser = argparse.ArgumentParser(description='Pin QEMU vCPUs to physical CPUs')
>> +    parser.add_argument('-s', '--server', type=str, required=True,
>> +                        help='QMP server path or address:port')
>> +    parser.add_argument('cpu', type=int, nargs='+',
>> +                        help='Physical CPUs IDs')
>> +    args = parser.parse_args()
>> +
>> +    devnull = open(os.devnull, 'w')
>> +
>> +    srv = QEMUMonitorProtocol(args.server)
>> +    srv.connect()
>> +
>> +    for vcpu in srv.command('query-cpus'):
>> +        vcpuid = vcpu['CPU']
>> +        tid = vcpu['thread_id']
>> +        if tid in pinned:
>> +            print 'vCPU{}\'s tid {} already pinned, skipping'.format(vcpuid, tid)
>> +            continue
>> +
>> +        cpuid = args.cpu[vcpuid % len(args.cpu)]
>> +        print 'Pin vCPU {} (tid {}) to physical CPU {}'.format(vcpuid, tid, cpuid)
>> +        try:
>> +            call(['taskset', '-pc', str(cpuid), str(tid)], stdout=devnull)
>> +            pinned.append(tid)
>> +        except OSError:
>> +            print 'Failed to pin vCPU{} to CPU{}'.format(vcpuid, cpuid)
>> +
>> +
>> +That can be used this way, for example to pin 3 vCPUs to CPUs 1, 6 and 7:
>
> I think it would be good to explicitly explain the link you've made on
> core numbers in this case between isolcpus, the vCPU pinning above and
> the core list in the testpmd cmd line later.

Yes.
So vCPU0 doesn't run testpmd lcores, so doesn't need to be pinned to an
isolated CPU. vCPU1&2 are used as lcores, so are pinned to isolated
CPUs.

I'll add this information to next version.

Thanks,
Maxime

^ permalink raw reply

* [RFC PATCH] i40e: fix setting of default MAC address
From: Igor Ryzhov @ 2016-11-24 12:34 UTC (permalink / raw)
  To: dev; +Cc: helin.zhang, jingjing.wu

While testing X710 cards in our lab I found that setting of default MAC address
doesn't work correctly for i40e driver. I compared DPDK driver implementation
with Linux driver implementation and found that a lot of code is lost in DPDK.
I tried to make DPDK implementation similar to Linux implementation and it
worked for me – now everything is working. But I'm not sure that my changes are
correct so, please, maintainers, check the patch very careful.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
---
 drivers/net/i40e/i40e_ethdev.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 67778ba..b73f9c8 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -9694,6 +9694,7 @@ static int i40e_get_eeprom(struct rte_eth_dev *dev,
 static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 				      struct ether_addr *mac_addr)
 {
+	struct i40e_vsi *vsi = I40E_DEV_PRIVATE_TO_MAIN_VSI(dev->data->dev_private);
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
 	if (!is_valid_assigned_ether_addr(mac_addr)) {
@@ -9701,8 +9702,33 @@ static void i40e_set_default_mac_addr(struct rte_eth_dev *dev,
 		return;
 	}
 
-	/* Flags: 0x3 updates port address */
-	i40e_aq_mac_address_write(hw, 0x3, mac_addr->addr_bytes, NULL);
+	i40e_aq_mac_address_write(hw, I40E_AQC_WRITE_TYPE_LAA_WOL, mac_addr->addr_bytes, NULL);
+
+	if (!memcmp(&dev->data->mac_addrs[0].addr_bytes, hw->mac.addr, ETH_ADDR_LEN)) {
+		struct i40e_aqc_remove_macvlan_element_data element;
+
+		memset(&element, 0, sizeof(element));
+		memcpy(element.mac_addr, &dev->data->mac_addrs[0].addr_bytes, ETH_ADDR_LEN);
+		element.flags = I40E_AQC_MACVLAN_DEL_PERFECT_MATCH;
+		i40e_aq_remove_macvlan(hw, vsi->seid, &element, 1, NULL);
+	} else {
+		i40e_vsi_delete_mac(vsi, &dev->data->mac_addrs[0]);
+	}
+
+	if (!memcmp(mac_addr->addr_bytes, hw->mac.addr, ETH_ADDR_LEN)) {
+		struct i40e_aqc_add_macvlan_element_data element;
+
+		memset(&element, 0, sizeof(element));
+		memcpy(element.mac_addr, hw->mac.addr, ETH_ADDR_LEN);
+		element.flags = CPU_TO_LE16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
+		i40e_aq_add_macvlan(hw, vsi->seid, &element, 1, NULL);
+	} else {
+		struct i40e_mac_filter_info filter;
+
+		memcpy(&filter.mac_addr, mac_addr, ETH_ADDR_LEN);
+		filter.filter_type = RTE_MAC_PERFECT_MATCH;
+		i40e_vsi_add_mac(vsi, &filter);
+	}
 }
 
 static int
-- 
2.6.4

^ permalink raw reply related

* Re: dpdk/vpp and cross-version migration for vhost
From: Yuanhan Liu @ 2016-11-24 12:33 UTC (permalink / raw)
  To: Kevin Traynor
  Cc: Michael S. Tsirkin, Maxime Coquelin, dev, Stephen Hemminger,
	qemu-devel, libvir-list, vpp-dev, Marc-André Lureau
In-Reply-To: <4d6e8cf0-fe19-43a9-ff73-c2a9cdeb681e@redhat.com>

On Thu, Nov 24, 2016 at 09:30:49AM +0000, Kevin Traynor wrote:
> On 11/24/2016 06:31 AM, Yuanhan Liu wrote:
> > On Tue, Nov 22, 2016 at 04:53:05PM +0200, Michael S. Tsirkin wrote:
> >>>> You keep assuming that you have the VM started first and
> >>>> figure out things afterwards, but this does not work.
> >>>>
> >>>> Think about a cluster of machines. You want to start a VM in
> >>>> a way that will ensure compatibility with all hosts
> >>>> in a cluster.
> >>>
> >>> I see. I was more considering about the case when the dst
> >>> host (including the qemu and dpdk combo) is given, and
> >>> then determine whether it will be a successfull migration
> >>> or not.
> >>>
> >>> And you are asking that we need to know which host could
> >>> be a good candidate before starting the migration. In such
> >>> case, we indeed need some inputs from both the qemu and
> >>> vhost-user backend.
> >>>
> >>> For DPDK, I think it could be simple, just as you said, it
> >>> could be either a tiny script, or even a macro defined in
> >>> the source code file (we extend it every time we add a
> >>> new feature) to let the libvirt to read it. Or something
> >>> else.
> >>
> >> There's the issue of APIs that tweak features as Maxime
> >> suggested.
> > 
> > Yes, it's a good point.
> > 
> >> Maybe the only thing to do is to deprecate it,
> > 
> > Looks like so.
> > 
> >> but I feel some way for application to pass info into
> >> guest might be benefitial.
> > 
> > The two APIs are just for tweaking feature bits DPDK supports before
> > any device got connected. It's another way to disable some features
> > (the another obvious way is to through QEMU command lines).
> > 
> > IMO, it's bit handy only in a case like: we have bunch of VMs. Instead
> > of disabling something though qemu one by one, we could disable it
> > once in DPDK.
> > 
> > But I doubt the useful of it. It's only used in DPDK's vhost example
> > after all. Nor is it used in vhost pmd, neither is it used in OVS.
> 
> rte_vhost_feature_disable() is currently used in OVS, lib/netdev-dpdk.c

Hmmm. I must have checked very old code ...
> 
> netdev_dpdk_vhost_class_init(void)
> {
>     static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> 
>     /* This function can be called for different classes.  The
> initialization
>      * needs to be done only once */
>     if (ovsthread_once_start(&once)) {
>         rte_vhost_driver_callback_register(&virtio_net_device_ops);
>         rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4
>                                   | 1ULL << VIRTIO_NET_F_HOST_TSO6
>                                   | 1ULL << VIRTIO_NET_F_CSUM);

I saw the commit introduced such change, but it tells no reason why
it was added.

commit 362ca39639ae871806be5ae97d55e1cbb14afd92
Author: mweglicx <michalx.weglicki@intel.com>
Date:   Thu Apr 14 17:40:06 2016 +0100

    Update relevant artifacts to add support for DPDK 16.04.

    Following changes are applied:
     - INSTALL.DPDK.md: CONFIG_RTE_BUILD_COMBINE_LIBS step has been
       removed because it is no longer present in DPDK configuration
       (combined library is created by default),
     - INSTALL.DPDK.md: VHost Cuse configuration is updated,
     - netdev-dpdk.c: Link speed definition is changed in DPDK and
       netdev_dpdk_get_features is updated accordingly,
     - netdev-dpdk.c: TSO and checksum offload has been disabled for
       vhostuser device.
     - .travis/linux-build.sh: DPDK version is updated and legacy
       flags have been removed in configuration.

    Signed-off-by: Michal Weglicki <michalx.weglicki@intel.com>
    Signed-off-by: Panu Matilainen <pmatilai@redhat.com>
    Acked-by: Daniele Di Proietto <diproiettod@vmware.com>

	--yliu

^ permalink raw reply

* Re: [PATCH 1/4] eventdev: introduce event driven programming model
From: Bruce Richardson @ 2016-11-24 12:26 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Thomas Monjalon, dev, harry.van.haaren, hemant.agrawal, gage.eads
In-Reply-To: <20161124015912.GA13508@svelivela-lt.caveonetworks.com>

On Thu, Nov 24, 2016 at 07:29:13AM +0530, Jerin Jacob wrote:
> On Wed, Nov 23, 2016 at 07:39:09PM +0100, Thomas Monjalon wrote:

Just some comments on mine triggered by Thomas comments?

<snip>
> > + */
> > > +static inline int
> > > +rte_event_enqueue(uint8_t dev_id, uint8_t port_id, struct rte_event *ev)
> > 
> > Is it really needed to have non-burst variant of enqueue/dequeue?
> 
> Yes. certain HW can work only with non burst variants.

In those cases is it not acceptable just to have the dequeue_burst
function return 1 all the time? It would allow apps to be more portable
between burst and non-burst varients would it not.

> > 
> > > +/**
> > > + * Converts nanoseconds to *wait* value for rte_event_dequeue()
> > > + *
> > > + * If the device is configured with RTE_EVENT_DEV_CFG_PER_DEQUEUE_WAIT flag then
> > > + * application can use this function to convert wait value in nanoseconds to
> > > + * implementations specific wait value supplied in rte_event_dequeue()
> > 
> > Why is it implementation-specific?
> > Why this conversion is not internal in the driver?
> 
> This is for performance optimization, otherwise in drivers
> need to convert ns to ticks in "fast path"
> 
> > 
Is that really likely to be a performance bottleneck. I would expect
modern cores to fly through basic arithmetic in a negligable amount of
cycles?

/Bruce

^ permalink raw reply

* Re: [PATCH] doc: introduce PVP reference benchmark
From: Kevin Traynor @ 2016-11-24 11:58 UTC (permalink / raw)
  To: Maxime Coquelin, yuanhan.liu, thomas.monjalon, john.mcnamara,
	zhiyong.yang, dev
  Cc: fbaudin
In-Reply-To: <20161123210006.7113-1-maxime.coquelin@redhat.com>

On 11/23/2016 09:00 PM, 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>

A short template/hint of the main things to report after running could
be useful to help ML discussions about results e.g.

Traffic Generator: IXIA
Acceptable Loss: 100% (i.e. raw throughput test)
DPDK version/commit: v16.11
QEMU version/commit: v2.7.0
Patches applied: <link to patchwork>
CPU: E5-2680 v3, 2.8GHz
Result: x mpps
NIC: ixgbe 82599

> ---
>  doc/guides/howto/img/pvp_2nics.svg           | 556 +++++++++++++++++++++++++++
>  doc/guides/howto/index.rst                   |   1 +
>  doc/guides/howto/pvp_reference_benchmark.rst | 389 +++++++++++++++++++
>  3 files changed, 946 insertions(+)
>  create mode 100644 doc/guides/howto/img/pvp_2nics.svg
>  create mode 100644 doc/guides/howto/pvp_reference_benchmark.rst
> 

<snip>

> +Host tuning
> +~~~~~~~~~~~

I would add turbo boost =disabled on BIOS.

> +
> +#. Append these options to Kernel command line:
> +
> +   .. code-block:: console
> +
> +    intel_pstate=disable mce=ignore_ce default_hugepagesz=1G hugepagesz=1G hugepages=6 isolcpus=2-7 rcu_nocbs=2-7 nohz_full=2-7 iommu=pt intel_iommu=on
> +
> +#. Disable hyper-threads at runtime if necessary and BIOS not accessible:
> +
> +   .. code-block:: console
> +
> +    cat /sys/devices/system/cpu/cpu*[0-9]/topology/thread_siblings_list \
> +        | sort | uniq \
> +        | awk -F, '{system("echo 0 > /sys/devices/system/cpu/cpu"$2"/online")}'
> +
> +#. Disable NMIs:
> +
> +   .. code-block:: console
> +
> +    echo 0 > /proc/sys/kernel/nmi_watchdog
> +
> +#. Exclude isolated CPUs from the writeback cpumask:
> +
> +   .. code-block:: console
> +
> +    echo ffffff03 > /sys/bus/workqueue/devices/writeback/cpumask
> +
> +#. Isolate CPUs from IRQs:
> +
> +   .. code-block:: console
> +
> +    clear_mask=0xfc #Isolate CPU2 to CPU7 from IRQs
> +    for i in /proc/irq/*/smp_affinity
> +    do
> +     echo "obase=16;$(( 0x$(cat $i) & ~$clear_mask ))" | bc > $i
> +    done
> +
> +Qemu build
> +~~~~~~~~~~
> +
> +   .. code-block:: console
> +
> +    git clone git://dpdk.org/dpdk
> +    cd dpdk
> +    export RTE_SDK=$PWD
> +    make install T=x86_64-native-linuxapp-gcc DESTDIR=install
> +
> +DPDK build
> +~~~~~~~~~~
> +
> +   .. code-block:: console
> +
> +    git clone git://dpdk.org/dpdk
> +    cd dpdk
> +    export RTE_SDK=$PWD
> +    make install T=x86_64-native-linuxapp-gcc DESTDIR=install
> +
> +Testpmd launch
> +~~~~~~~~~~~~~~
> +
> +#. Assign NICs to DPDK:
> +
> +   .. code-block:: console
> +
> +    modprobe vfio-pci
> +    $RTE_SDK/install/sbin/dpdk-devbind -b vfio-pci 0000:11:00.0 0000:11:00.1
> +
> +*Note: Sandy Bridge family seems to have some limitations wrt its IOMMU,
> +giving poor performance results. To achieve good performance on these machines,
> +consider using UIO instead.*
> +
> +#. Launch testpmd application:
> +
> +   .. code-block:: console
> +
> +    $RTE_SDK/install/bin/testpmd -l 0,2,3,4,5 --socket-mem=1024 -n 4 \
> +        --vdev 'net_vhost0,iface=/tmp/vhost-user1' \
> +        --vdev 'net_vhost1,iface=/tmp/vhost-user2' -- \
> +        --portmask=f --disable-hw-vlan -i --rxq=1 --txq=1
> +        --nb-cores=4 --forward-mode=io
> +
> +#. In testpmd interactive mode, set the portlist to obtin the right chaining:
> +
> +   .. code-block:: console
> +
> +    set portlist 0,2,1,3
> +    start
> +
> +VM launch
> +~~~~~~~~~
> +
> +The VM may be launched ezither by calling directly QEMU, or by using libvirt.

s/ezither/either

> +
> +#. Qemu way:
> +
> +Launch QEMU with two Virtio-net devices paired to the vhost-user sockets created by testpmd:
> +
> +   .. code-block:: console
> +
> +    <QEMU path>/bin/x86_64-softmmu/qemu-system-x86_64 \
> +        -enable-kvm -cpu host -m 3072 -smp 3 \
> +        -chardev socket,id=char0,path=/tmp/vhost-user1 \
> +        -netdev type=vhost-user,id=mynet1,chardev=char0,vhostforce \
> +        -device virtio-net-pci,netdev=mynet1,mac=52:54:00:02:d9:01,addr=0x10 \
> +        -chardev socket,id=char1,path=/tmp/vhost-user2 \
> +        -netdev type=vhost-user,id=mynet2,chardev=char1,vhostforce \
> +        -device virtio-net-pci,netdev=mynet2,mac=52:54:00:02:d9:02,addr=0x11 \
> +        -object memory-backend-file,id=mem,size=3072M,mem-path=/dev/hugepages,share=on \
> +        -numa node,memdev=mem -mem-prealloc \
> +        -net user,hostfwd=tcp::1002$1-:22 -net nic \
> +        -qmp unix:/tmp/qmp.socket,server,nowait \
> +        -monitor stdio <vm_image>.qcow2

Probably mergeable rx data path =off would want to be tested also when
evaluating any performance improvements/regressions.

> +
> +You can use this qmp-vcpu-pin script to pin vCPUs:
> +
> +   .. code-block:: python
> +
> +    #!/usr/bin/python
> +    # QEMU vCPU pinning tool
> +    #
> +    # Copyright (C) 2016 Red Hat Inc.
> +    #
> +    # Authors:
> +    #  Maxime Coquelin <maxime.coquelin@redhat.com>
> +    #
> +    # This work is licensed under the terms of the GNU GPL, version 2.  See
> +    # the COPYING file in the top-level directory
> +    import argparse
> +    import json
> +    import os
> +
> +    from subprocess import call
> +    from qmp import QEMUMonitorProtocol
> +
> +    pinned = []
> +
> +    parser = argparse.ArgumentParser(description='Pin QEMU vCPUs to physical CPUs')
> +    parser.add_argument('-s', '--server', type=str, required=True,
> +                        help='QMP server path or address:port')
> +    parser.add_argument('cpu', type=int, nargs='+',
> +                        help='Physical CPUs IDs')
> +    args = parser.parse_args()
> +
> +    devnull = open(os.devnull, 'w')
> +
> +    srv = QEMUMonitorProtocol(args.server)
> +    srv.connect()
> +
> +    for vcpu in srv.command('query-cpus'):
> +        vcpuid = vcpu['CPU']
> +        tid = vcpu['thread_id']
> +        if tid in pinned:
> +            print 'vCPU{}\'s tid {} already pinned, skipping'.format(vcpuid, tid)
> +            continue
> +
> +        cpuid = args.cpu[vcpuid % len(args.cpu)]
> +        print 'Pin vCPU {} (tid {}) to physical CPU {}'.format(vcpuid, tid, cpuid)
> +        try:
> +            call(['taskset', '-pc', str(cpuid), str(tid)], stdout=devnull)
> +            pinned.append(tid)
> +        except OSError:
> +            print 'Failed to pin vCPU{} to CPU{}'.format(vcpuid, cpuid)
> +
> +
> +That can be used this way, for example to pin 3 vCPUs to CPUs 1, 6 and 7:

I think it would be good to explicitly explain the link you've made on
core numbers in this case between isolcpus, the vCPU pinning above and
the core list in the testpmd cmd line later.

> +
> +   .. code-block:: console
> +
> +    export PYTHONPATH=$PYTHONPATH:<QEMU path>/scripts/qmp
> +    ./qmp-vcpu-pin -s /tmp/qmp.socket 1 6 7
> +
> +#. Libvirt way:
> +
> +Some initial steps are required for libvirt to be able to connect to testpmd's
> +sockets.
> +
> +First, SELinux policy needs to be set to permissiven, as testpmd is run as root
> +(reboot required):
> +
> +   .. code-block:: console
> +
> +    cat /etc/selinux/config
> +
> +    # This file controls the state of SELinux on the system.
> +    # SELINUX= can take one of these three values:
> +    #     enforcing - SELinux security policy is enforced.
> +    #     permissive - SELinux prints warnings instead of enforcing.
> +    #     disabled - No SELinux policy is loaded.
> +    SELINUX=permissive
> +    # SELINUXTYPE= can take one of three two values:
> +    #     targeted - Targeted processes are protected,
> +    #     minimum - Modification of targeted policy. Only selected processes are protected. 
> +    #     mls - Multi Level Security protection.
> +    SELINUXTYPE=targeted
> +
> +
> +Also, Qemu needs to be run as root, which has to be specified in /etc/libvirt/qemu.conf:
> +
> +   .. code-block:: console
> +
> +    user = "root"
> +
> +Once the domain created, following snippset is an extract of most important
> +information (hugepages, vCPU pinning, Virtio PCI devices):
> +
> +   .. code-block:: xml
> +
> +    <domain type='kvm'>
> +      <memory unit='KiB'>3145728</memory>
> +      <currentMemory unit='KiB'>3145728</currentMemory>
> +      <memoryBacking>
> +        <hugepages>
> +          <page size='1048576' unit='KiB' nodeset='0'/>
> +        </hugepages>
> +        <locked/>
> +      </memoryBacking>
> +      <vcpu placement='static'>3</vcpu>
> +      <cputune>
> +        <vcpupin vcpu='0' cpuset='1'/>
> +        <vcpupin vcpu='1' cpuset='6'/>
> +        <vcpupin vcpu='2' cpuset='7'/>
> +        <emulatorpin cpuset='0'/>
> +      </cputune>
> +      <numatune>
> +        <memory mode='strict' nodeset='0'/>
> +      </numatune>
> +      <os>
> +        <type arch='x86_64' machine='pc-i440fx-rhel7.0.0'>hvm</type>
> +        <boot dev='hd'/>
> +      </os>
> +      <cpu mode='host-passthrough'>
> +        <topology sockets='1' cores='3' threads='1'/>
> +        <numa>
> +          <cell id='0' cpus='0-2' memory='3145728' unit='KiB' memAccess='shared'/>
> +        </numa>
> +      </cpu>
> +      <devices>
> +        <interface type='vhostuser'>
> +          <mac address='56:48:4f:53:54:01'/>
> +          <source type='unix' path='/tmp/vhost-user1' mode='client'/>
> +          <model type='virtio'/>
> +          <driver name='vhost' rx_queue_size='256' />
> +          <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
> +        </interface>
> +        <interface type='vhostuser'>
> +          <mac address='56:48:4f:53:54:02'/>
> +          <source type='unix' path='/tmp/vhost-user2' mode='client'/>
> +          <model type='virtio'/>
> +          <driver name='vhost' rx_queue_size='256' />
> +          <address type='pci' domain='0x0000' bus='0x00' slot='0x11' function='0x0'/>
> +        </interface>
> +      </devices>
> +    </domain>
> +
> +Guest setup
> +...........
> +
> +Guest tuning
> +~~~~~~~~~~~~
> +
> +#. Append these options to Kernel command line:
> +
> +   .. code-block:: console
> +
> +    default_hugepagesz=1G hugepagesz=1G hugepages=1 intel_iommu=on iommu=pt isolcpus=1,2 rcu_nocbs=1,2 nohz_full=1,2
> +
> +#. Disable NMIs:
> +
> +   .. code-block:: console
> +
> +    echo 0 > /proc/sys/kernel/nmi_watchdog
> +
> +#. Exclude isolated CPU1 and CPU2 from the writeback wq cpumask:
> +
> +   .. code-block:: console
> +
> +    echo 1 > /sys/bus/workqueue/devices/writeback/cpumask
> +
> +#. Isolate CPUs from IRQs:
> +
> +   .. code-block:: console
> +
> +    clear_mask=0x6 #Isolate CPU1 and CPU2 from IRQs
> +    for i in /proc/irq/*/smp_affinity
> +    do
> +      echo "obase=16;$(( 0x$(cat $i) & ~$clear_mask ))" | bc > $i
> +    done
> +
> +DPDK build
> +~~~~~~~~~~
> +
> +   .. code-block:: console
> +
> +    git clone git://dpdk.org/dpdk
> +    cd dpdk
> +    export RTE_SDK=$PWD
> +    make install T=x86_64-native-linuxapp-gcc DESTDIR=install
> +
> +Testpmd launch
> +~~~~~~~~~~~~~~
> +
> +Probe vfio module without iommu:
> +
> +   .. code-block:: console
> +
> +    modprobe -r vfio_iommu_type1
> +    modprobe -r vfio
> +    modprobe  vfio enable_unsafe_noiommu_mode=1
> +    cat /sys/module/vfio/parameters/enable_unsafe_noiommu_mode
> +    modprobe vfio-pci
> +
> +Bind virtio-net devices to DPDK:
> +
> +   .. code-block:: console
> +
> +    $RTE_SDK/tools/dpdk-devbind.py -b vfio-pci 0000:00:10.0 0000:00:11.0
> +
> +Start testpmd:
> +
> +   .. code-block:: console
> +
> +    $RTE_SDK/install/bin/testpmd -l 0,1,2 --socket-mem 1024 -n 4 \
> +        --proc-type auto --file-prefix pg -- \
> +        --portmask=3 --forward-mode=macswap --port-topology=chained \
> +        --disable-hw-vlan --disable-rss -i --rxq=1 --txq=1 \
> +        --rxd=256 --txd=256 --nb-cores=2 --auto-start
> 

^ permalink raw reply

* Re: [PATCH v2 1/2] net: remove dead driver names
From: Ferruh Yigit @ 2016-11-24 11:42 UTC (permalink / raw)
  To: David Marchand, thomas.monjalon
  Cc: dev, jblunck, linville, declan.doherty, zlu, lsun,
	alejandro.lucero, mtetsuyah, nicolas.pernas.maradei, harish.patil,
	rasesh.mody, sony.chacko, bruce.richardson, huawei.xie,
	yuanhan.liu, jianfeng.tan
In-Reply-To: <1479751574-5704-1-git-send-email-david.marchand@6wind.com>

On 11/21/2016 6:06 PM, David Marchand wrote:
> Since commit b1fb53a39d88 ("ethdev: remove some PCI specific handling"),
> rte_eth_dev_info_get() relies on dev->data->drv_name to report the driver
> name to caller.
> 
> Having the pmds set driver_info->driver_name in the pmds is useless,
> since ethdev overwrites it right after.
> The only thing the pmd must do is:
> - for pci drivers, call rte_eth_copy_pci_info() which then sets
>   data->drv_name
> - for vdev drivers, manually set data->drv_name
> 
> At this stage, virtio-user does not properly report a driver name (fixed in
> next commit).
> 
> Signed-off-by: David Marchand <david.marchand@6wind.com>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Reviewed-by: Jan Blunck <jblunck@infradead.org>

Series applied to dpdk-next-net/master, thanks.

^ permalink raw reply

* [PATCH v2 5/5] Revert "bonding: use existing enslaved device queues"
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable
In-Reply-To: <1479986809-14934-1-git-send-email-jblunck@infradead.org>

From: Ilya Maximets <i.maximets@samsung.com>

This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.

It is necessary to reconfigure all queues every time because configuration
can be changed.

For example, if we're reconfiguring bonding device with new memory pool,
already configured queues will still use the old one. And if the old
mempool be freed, application likely will panic in attempt to use
freed mempool.

This happens when we use the bonding device with OVS 2.6 while MTU
reconfiguration:

PANIC in rte_mempool_get_ops():
assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed

Cc: <stable@dpdk.org>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
Acked-by: Declan Doherty <declan.doherty@intel.com>
Acked-by: Declan Doherty <declan.doherty@intel.com>
Acked-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index e61afc9..b604642 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1317,8 +1317,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	struct bond_rx_queue *bd_rx_q;
 	struct bond_tx_queue *bd_tx_q;
 
-	uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
-	uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
 	int errval;
 	uint16_t q_id;
 
@@ -1362,9 +1360,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Rx Queues */
-	/* Use existing queues, if any */
-	for (q_id = old_nb_rx_queues;
-	     q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
+	for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
 		bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
 
 		errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
@@ -1380,9 +1376,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	}
 
 	/* Setup Tx Queues */
-	/* Use existing queues, if any */
-	for (q_id = old_nb_tx_queues;
-	     q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
+	for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
 		bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
 
 		errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 4/5] net/bonding: Force reconfiguration of removed slave interfaces
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable
In-Reply-To: <1479986809-14934-1-git-send-email-jblunck@infradead.org>

After a slave interface is removed from a bond group it still has the
configuration of the bond interface. Lets enforce that the slave interface
is reconfigured after removal by resetting it.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index a80b6fa..e61afc9 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1454,6 +1454,9 @@ slave_remove(struct bond_dev_private *internals,
 				(internals->slave_count - i - 1));
 
 	internals->slave_count--;
+
+	/* force reconfiguration of slave interfaces */
+	_rte_eth_dev_reset(slave_eth_dev);
 }
 
 static void
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 3/5] ethdev: Add DPDK internal _rte_eth_dev_reset()
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable
In-Reply-To: <1479986809-14934-1-git-send-email-jblunck@infradead.org>

This is a helper for DPDK internal users to force a reconfiguration of a
device.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c          | 16 ++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++++
 lib/librte_ether/rte_ether_version.map |  6 ++++++
 3 files changed, 35 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a3986ad..9e69ee5 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -858,6 +858,22 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	return 0;
 }
 
+void
+_rte_eth_dev_reset(struct rte_eth_dev *dev)
+{
+	if (dev->data->dev_started) {
+		RTE_PMD_DEBUG_TRACE(
+			"port %d must be stopped to allow reset\n",
+			dev->data->port_id);
+		return;
+	}
+
+	rte_eth_dev_rx_queue_config(dev, 0);
+	rte_eth_dev_tx_queue_config(dev, 0);
+
+	memset(&dev->data->dev_conf, 0, sizeof(dev->data->dev_conf));
+}
+
 static void
 rte_eth_dev_config_restore(uint8_t port_id)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..e0740db 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1914,6 +1914,19 @@ int rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_queue,
 		uint16_t nb_tx_queue, const struct rte_eth_conf *eth_conf);
 
 /**
+ * @internal Release a devices rx/tx queues and clear its configuration to
+ * force the user application to reconfigure it. It is for DPDK internal user
+ * only.
+ *
+ * @param dev
+ *  Pointer to struct rte_eth_dev.
+ *
+ * @return
+ *  void
+ */
+void _rte_eth_dev_reset(struct rte_eth_dev *dev);
+
+/**
  * Allocate and set up a receive queue for an Ethernet device.
  *
  * The function allocates a contiguous block of memory for *nb_rx_desc*
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 72be66d..0c31c5d 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -147,3 +147,9 @@ DPDK_16.11 {
 	rte_eth_dev_pci_remove;
 
 } DPDK_16.07;
+
+DPDK_17.02 {
+	global:
+
+	_rte_eth_dev_reset;
+} DPDK_16.11;
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 2/5] ethdev: Free rx/tx_queues after releasing all queues
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable
In-Reply-To: <1479986809-14934-1-git-send-email-jblunck@infradead.org>

If all queues are released lets also free up the dev->data->rx/tx_queues
to be able to properly reinitialize.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 8c4b6cd..a3986ad 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -531,6 +531,9 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 
 		for (i = nb_queues; i < old_nb_queues; i++)
 			(*dev->dev_ops->rx_queue_release)(rxq[i]);
+
+		rte_free(dev->data->rx_queues);
+		dev->data->rx_queues = NULL;
 	}
 	dev->data->nb_rx_queues = nb_queues;
 	return 0;
@@ -682,6 +685,9 @@ rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
 
 		for (i = nb_queues; i < old_nb_queues; i++)
 			(*dev->dev_ops->tx_queue_release)(txq[i]);
+
+		rte_free(dev->data->tx_queues);
+		dev->data->tx_queues = NULL;
 	}
 	dev->data->nb_tx_queues = nb_queues;
 	return 0;
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 1/5] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable
In-Reply-To: <1479986809-14934-1-git-send-email-jblunck@infradead.org>

If a queue has been setup before lets release it before we setup.
Otherwise we might leak resources.

Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
 lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..8c4b6cd 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1010,6 +1010,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 	uint32_t mbp_buf_size;
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	void **rxq;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1068,6 +1069,14 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	rxq = dev->data->rx_queues;
+	if (rxq[rx_queue_id]) {
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
+					-ENOTSUP);
+		(*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
+		rxq[rx_queue_id] = NULL;
+	}
+
 	if (rx_conf == NULL)
 		rx_conf = &dev_info.default_rxconf;
 
@@ -1089,6 +1098,7 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	void **txq;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1121,6 +1131,14 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
 		return -EINVAL;
 	}
 
+	txq = dev->data->tx_queues;
+	if (txq[tx_queue_id]) {
+		RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
+					-ENOTSUP);
+		(*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
+		txq[tx_queue_id] = NULL;
+	}
+
 	if (tx_conf == NULL)
 		tx_conf = &dev_info.default_txconf;
 
-- 
2.7.4

^ permalink raw reply related

* [PATCH v2 0/5] bonding: setup all queues of slave devices
From: Jan Blunck @ 2016-11-24 11:26 UTC (permalink / raw)
  To: dev
  Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty,
	ehkinzie, bernard.iremonger, stable

Prior to 16.11 some drivers (e.g. virtio) still had problems if their
queues where setup repeatedly. The bonding driver was working around the
problem by reusing already setup queues. This series of patches changes the
way how queue setup is done to give control to the driver to properly release
already initialized queues before they are setup again. Therefore the driver
call sequence is as if the number of queues is temporarily reduced before the
queues are setup again.

Ilya Maximets (1):
  Revert "bonding: use existing enslaved device queues"

Jan Blunck (4):
  ethdev: Call rx/tx_queue_release before rx/tx_queue_setup
  ethdev: Free rx/tx_queues after releasing all queues
  ethdev: Add DPDK internal _rte_eth_dev_reset()
  net/bonding: Force reconfiguration of removed slave interfaces

 drivers/net/bonding/rte_eth_bond_pmd.c | 13 +++++------
 lib/librte_ether/rte_ethdev.c          | 40 ++++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 13 +++++++++++
 lib/librte_ether/rte_ether_version.map |  6 +++++
 4 files changed, 64 insertions(+), 8 deletions(-)

-- 
2.7.4

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox