Re: [dpdk-dev] [PATCH v10 07/23] event/dlb: add flexible interface

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:27 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 07/23] event/dlb: add flexible interface
> 
> This commit introduces the flexible interface. This
> interface allows the core code to operate in PF mode (direct
> hardware access) or bifurcated mode (hardware configured via
> kernel driver). This driver currently only supports PF modei,
> but bifurcated mode will be added in a future patch-set.
> Note that the flexible interface is not used for data path
> operations, and thus there are no performance concerns
> related to the use of function pointers.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 1/1] eventdev: increase MAX QUEUES PER DEV to 255

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Monday, October 26, 2020 11:01 AM
> To: Richardson, Bruce 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 1/1] eventdev: increase MAX QUEUES PER DEV to 255
> 
> DLB supports a total of 256 queues, 128 load balanced queues
> and 128 directed queues.
> 
> Signed-off-by: Timothy McDaniel 

Perhaps you can increase the queues-related fields to uint16_t in the future
to allow for the full 256, but in the meantime:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 01/23] event/dlb: add documentation and meson infrastructure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:27 PM
> To: Thomas Monjalon ; Richardson, Bruce
> ; Ray Kinsella ; Neil Horman
> 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v10 01/23] event/dlb: add documentation and meson
> infrastructure
> 
> Note that config/rte_config.h contains several configuration
> switches, providing for fine control of the PMD's
> runtime behaviour.
> 
> The meson infrastructure is expanded as additional files are
> added to this patchset.
> 
> Adds announcement of availabililty of the new driver
> for Intel Dynamic Load Balancer 1.0 hardware.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 08/23] event/dlb: add probe-time hardware init

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:27 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 08/23] event/dlb: add probe-time hardware init
> 
> This commit adds probe-time low level hardware
> initialization.  It also adds probe-time init for both
> primary and secondary DPDK processes.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 18/23] event/dlb: add dequeue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:28 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 18/23] event/dlb: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...
> 
> DLB does not currently support interrupts, but instead uses
> umonitor/umwait if supported by the processor. This allows
> the software to monitor and wait on writes to a cache-line.
> 
> DLB supports normal and sparse cq mode. In normal mode the
> hardware will pack 4 QEs into each cache line. In sparse cq
> mode, the hardware will only populate one QE per cache line.
> Software must be aware of the cq mode, and take the appropriate
> actions, based on the mode.
> 
> Signed-off-by: Timothy McDaniel 

I believe I added my tag to the v5 of this patch, but for good measure:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v6 06/23] event/dlb2: add eventdev probe

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:29 PM
> To: Burakov, Anatoly 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v6 06/23] event/dlb2: add eventdev probe
> 
> Add the eventdev portion of probe, and parse command line
> options, but do not initialize hardware.
> 
> Changes since v2 patch-set probe:
> 
> Primary and secondary probe-time init has been removed, and
> will be introduced in subsequent patches contained in
> the v3 patch-set.
> 
> Hardware init has been moved to a subsequent patch in order to
> minimize the patch size. The files dlb2/pf/dlb2_pf.c and
> dlb2/pf/dlb_main.c include stubs of hardware init functions,
> and these will be replaced with the real versions when the
> hardware layer is committed.
> 
> Initialization of the flexible interface layer has been moved to
> a subsequent patch in order to minimize patch size.
> 
> Signed-off-by: Timothy McDaniel 

If there's a next version, please put the 'Changes since' info below the '---'
line, so it's not included in the commit message. (Otherwise this can get fixed
during merge.)

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 22/23] event/dlb: add queue and port release

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:28 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 22/23] event/dlb: add queue and port release
> 
> These entry points are NO-OPS. DLB does not support
> reconfiguring individual queues or ports. The entire device
> must be reconfigured.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v6 08/23] event/dlb2: add probe-time hardware init

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:29 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v6 08/23] event/dlb2: add probe-time hardware init
> 
> This commit adds probe-time low level hardware
> initialization.  It also adds probe-time init for both
> primary and secondary DPDK processes.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 06/23] event/dlb: add eventdev probe

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:27 PM
> To: Burakov, Anatoly 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 06/23] event/dlb: add eventdev probe
> 
> Add the eventdev portion of probe, and parse command line
> options, but do not initialize hardware.
> 
> Changes since v5 patch-set probe:
> 
> Primary and secondary probe-time init has been removed, and
> will be introduced in subsequent patches contained in
> this patch-set.
> 
> Hardware init has been moved to a subsequent patch in order to
> minimize the patch size.
> 
> Initialization of the flexible interface layer has been moved to
> a subsequent patch in order to minimize patch size.
> 
> Signed-off-by: Timothy McDaniel 
> ---

Same as with the 2.0 patchset: if there's a next version, please put the
'Changes since' info below the '---' line, so it's not included in the commit
message. (Otherwise this can get fixed during merge.)

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v6 07/23] event/dlb2: add flexible interface

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:29 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v6 07/23] event/dlb2: add flexible interface
> 
> This commit introduces the flexible interface. This
> interface allows the core code to operate in PF mode (direct
> hardware access) or bifurcated mode (hardware configured via
> kernel driver). This driver currently only supports PF mode
> but bifurcated mode will be added in a future DPDK patch-set.
> Note that the flexible interface is not used for data path
> operations, and thus there are no performance concerns
> related to the use of function pointers.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v10 13/23] event/dlb: add port setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:27 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v10 13/23] event/dlb: add port setup
> 
> Configure the load balanded (ldb) or directed (dir) port.
> The consumer queue (CQ) and producer port (PP) are also
> set up here.
> 
> Signed-off-by: Timothy McDaniel 

Here also, typo in the commit message ("balanded" -> "balanced"), but that
can be fixed in the merge.

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v6 13/23] event/dlb2: add port setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, October 30, 2020 1:29 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com; tho...@monjalon.net
> Subject: [PATCH v6 13/23] event/dlb2: add port setup
> 
> Configure the load balanded (ldb) or directed (dir) port.
> The consumer queue (CQ) and producer port (PP) are also
> set up here.
> 
> Signed-off-by: Timothy McDaniel 

Typo in the commit message ("balanded" -> "balanced"), but that can
be fixed in the merge.

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH] maintainers: resign from stack library

[Eads, Gage]

(Off-list) I discussed filling the spot with Steven, Dharmik, and Honnappa. If 
I may
speak on their behalf -- Steven suggested Dharmik, who helped with Steven's 
stack
patches and knows the library well, and has shown his lock-free algorithm 
expertise
in other parts of DPDK. I'd support Dharmik to take on the role.

Thanks,
Gage

> -Original Message-
> From: Eads, Gage 
> Sent: Thursday, October 29, 2020 9:52 AM
> To: dev@dpdk.org
> Cc: olivier.m...@6wind.com; tho...@monjalon.net; Eads, Gage
> ; dharmik.thak...@arm.com; steven.lar...@arm.com;
> honnappa.nagaraha...@arm.com
> Subject: [PATCH] maintainers: resign from stack library
> 
> I'm moving on to a new position in November and won't be able to continue
> as a stack library maintainer.
> 
> Thanks to fellow maintainer Olivier, and the rest of the DPDK community,
> for the support over the past few years.
> 
> Signed-off-by: Gage Eads 
> ---
>  MAINTAINERS | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5b390d1d8..1265df1ec 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -362,7 +362,6 @@ F: app/test/test_ring*
>  F: app/test/test_func_reentrancy.c
> 
>  Stack
> -M: Gage Eads 
>  M: Olivier Matz 
>  F: lib/librte_stack/
>  F: drivers/mempool/stack/
> --
> 2.13.6

Re: [dpdk-dev] [PATCH v5 22/22] doc: Add new DLB eventdev driver to relnotes

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> To: Thomas Monjalon ; Mcnamara, John
> ; Kovacevic, Marko 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 22/22] doc: Add new DLB eventdev driver to relnotes
> 
> Added announcement of availabililty for the new driver
> for Intel Dynamic Load Balancer 1.0 hardware.
> 
> Signed-off-by: Timothy McDaniel 

Jerin's comments on the corresponding dlb2 patch apply here as well:
http://mails.dpdk.org/archives/dev/2020-October/187543.html

Re: [dpdk-dev] [PATCH v5 11/22] event/dlb: add port setup

[Eads, Gage]

[...]

> +static int
> +dlb_pf_ldb_port_create(struct dlb_hw_dev *handle,
> +struct dlb_create_ldb_port_args *cfg,
> +enum dlb_cq_poll_modes poll_mode)
> +{
> + struct dlb_dev *dlb_dev = (struct dlb_dev *)handle->pf_dev;
> + struct dlb_cmd_response response = {0};
> + int ret;
> + uint8_t *port_base;
> + const struct rte_memzone *mz;
> + int alloc_sz, qe_sz, cq_alloc_depth;
> + rte_iova_t pp_dma_base;
> + rte_iova_t pc_dma_base;
> + rte_iova_t cq_dma_base;
> + int is_dir = false;
> +
> + DLB_INFO(dev->dlb_device, "Entering %s()\n", __func__);
> +
> + if (poll_mode == DLB_CQ_POLL_MODE_STD)
> + qe_sz = sizeof(struct dlb_dequeue_qe);
> + else
> + qe_sz = RTE_CACHE_LINE_SIZE;
> +
> + /* The hardware always uses a CQ depth of at least
> +  * DLB_MIN_HARDWARE_CQ_DEPTH, even though from the user
> +  * perspective we support a depth as low as 1 for LDB ports.
> +  */
> + cq_alloc_depth = RTE_MAX(cfg->cq_depth,
> DLB_MIN_HARDWARE_CQ_DEPTH);
> +
> + /* Calculate the port memory required, including two cache lines for
> +  * credit pop counts. Round up to the nearest cache line.
> +  */
> + alloc_sz = 2 * RTE_CACHE_LINE_SIZE + cq_alloc_depth * qe_sz;
> + alloc_sz = RTE_CACHE_LINE_ROUNDUP(alloc_sz);
> +
> + port_base = dlb_alloc_coherent_aligned(&mz, &pc_dma_base,
> +alloc_sz, PAGE_SIZE);
> + if (port_base == NULL)
> + return -ENOMEM;
> +
> + /* Lock the page in memory */
> + ret = rte_mem_lock_page(port_base);
> + if (ret < 0) {
> + DLB_LOG_ERR("dlb pf pmd could not lock page for device i/o\n");
> + goto create_port_err;
> + }
> +
> + memset(port_base, 0, alloc_sz);
> + cq_dma_base = (uintptr_t)(pc_dma_base + (2 *
> RTE_CACHE_LINE_SIZE));
> +
> + ret = dlb_hw_create_ldb_port(&dlb_dev->hw,
> +  handle->domain_id,
> +  cfg,
> +  pc_dma_base,
> +  cq_dma_base,
> +  &response);
> + if (ret)
> + goto create_port_err;
> +
> + pp_dma_base = (uintptr_t)dlb_dev->hw.func_kva + PP_BASE(is_dir);
> + dlb_port[response.id][DLB_LDB].pp_addr =
> + (void *)(uintptr_t)(pp_dma_base + (PAGE_SIZE * response.id));
> +
> + dlb_port[response.id][DLB_LDB].cq_base =
> + (void *)(uintptr_t)(port_base + (2 * RTE_CACHE_LINE_SIZE));
> +
> + dlb_port[response.id][DLB_LDB].ldb_popcount =
> + (void *)(uintptr_t)port_base;
> + dlb_port[response.id][DLB_LDB].dir_popcount = (void *)(uintptr_t)
> + (port_base + RTE_CACHE_LINE_SIZE);
> + dlb_port[response.id][DLB_LDB].mz = mz;
> +
> + *(struct dlb_cmd_response *)cfg->response = response;
> +
> + DLB_INFO(dev->dlb_device, "Exiting %s() with ret=%d\n", __func__,
> ret);
> +
> +create_port_err:

Need to free the memzone if the PMD jumps to this label

> +
> + return ret;
> +}
> +
> +static int
> +dlb_pf_dir_port_create(struct dlb_hw_dev *handle,
> +struct dlb_create_dir_port_args *cfg,
> +enum dlb_cq_poll_modes poll_mode)
> +{
> + struct dlb_dev *dlb_dev = (struct dlb_dev *)handle->pf_dev;
> + struct dlb_cmd_response response = {0};
> + int ret;
> + uint8_t *port_base;
> + const struct rte_memzone *mz;
> + int alloc_sz, qe_sz;
> + rte_iova_t pp_dma_base;
> + rte_iova_t pc_dma_base;
> + rte_iova_t cq_dma_base;
> + int is_dir = true;
> +
> + DLB_INFO(dev->dlb_device, "Entering %s()\n", __func__);
> +
> + if (poll_mode == DLB_CQ_POLL_MODE_STD)
> + qe_sz = sizeof(struct dlb_dequeue_qe);
> + else
> + qe_sz = RTE_CACHE_LINE_SIZE;
> +
> + /* Calculate the port memory required, including two cache lines for
> +  * credit pop counts. Round up to the nearest cache line.
> +  */
> + alloc_sz = 2 * RTE_CACHE_LINE_SIZE + cfg->cq_depth * qe_sz;
> + alloc_sz = RTE_CACHE_LINE_ROUNDUP(alloc_sz);
> +
> + port_base = dlb_alloc_coherent_aligned(&mz, &pc_dma_base,
> +alloc_sz, PAGE_SIZE);
> + if (port_base == NULL)
> + return -ENOMEM;
> +
> + /* Lock the page in memory */
> + ret = rte_mem_lock_page(port_base);
> + if (ret < 0) {
> + DLB_LOG_ERR("dlb pf pmd could not lock page for device i/o\n");
> + goto create_port_err;
> + }
> +
> + memset(port_base, 0, alloc_sz);
> + cq_dma_base = (uintptr_t)(pc_dma_base + (2 *
> RTE_CACHE_LINE_SIZE));
> +
> + ret = dlb_hw_create_dir_port(&dlb_dev->hw,
> +  handle->domain_id,
> +  cfg,
> +  pc_dma_base,
> +

Re: [dpdk-dev] [PATCH v5 20/22] event/dlb: add queue and port release

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 20/22] event/dlb: add queue and port release
> 
> These entry points are NO-OPS. DLB does not support
> reconfiguring individual queues or ports. The entire device
> must be reconfigured.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 16/22] event/dlb: add dequeue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 16/22] event/dlb: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...
> 
> DLB does not currently support interrupts, but instead uses
> umonitor/umwait if supported by the processor. This allows
> the software to monitor and wait on writes to a cache-line.
> 
> DLB supports normal and sparse cq mode. In normal mode the
> hardware will pack 4 QEs into each cache line. In sparse cq
> mode, the hardware will only populate one QE per cache line.
> Software must be aware of the cq mode, and take the appropriate
> actions, based on the mode.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 08/22] event/dlb: add infos get and configure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 08/22] event/dlb: add infos get and configure
> 
> Add support for configuring the DLB hardware.
> In particular, this patch configures the DLB
> hardware's scheduling domain, such that it is provisioned with
> the requested number of ports and queues, provided sufficient
> resources are available. Individual queues and ports are
> configured later in port setup and eventdev start.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 07/22] event/dlb: add xstats

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 07/22] event/dlb: add xstats
> 
> Add support for DLB xstats.  Perform initialization and add
> standard xstats entry points
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 06/22] event/dlb: add probe

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> To: Burakov, Anatoly 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 06/22] event/dlb: add probe
> 
> The DLB hardware is a PCI device. This commit adds
> support for probe and other initialization. The
> dlb_iface.[ch] files implement a flexible interface
> that supports both the PF PMD and the bifurcated PMD.
> The bifurcated PMD will be released in a future
> patch set. Note that the flexible interface is only
> used for configuration, and is not used in the data
> path. The shared code is added in pf/base.
> Command line parameters are parsed at config time.
> 
> Signed-off-by: Timothy McDaniel 

Looks like my suggestions were addressed, but I think Jerin's comments
in the dlb2 "add probe" patch are applicable here as well.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 03/22] event/dlb: add private data structures and constants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 2:04 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v5 03/22] event/dlb: add private data structures and constants
> 
> Add headers used internally by the PMD.  They include constants,
> macros for device resources, structure definitions for hardware interfaces
> and software state, and various forward-declarations.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v5 01/22] event/dlb: add documentation and meson infrastructure

[Eads, Gage]

[...]

> diff --git a/doc/guides/eventdevs/index.rst b/doc/guides/eventdevs/index.rst
> index bb66a5e..f651818 100644
> --- a/doc/guides/eventdevs/index.rst
> +++ b/doc/guides/eventdevs/index.rst
> @@ -18,3 +18,4 @@ application through the eventdev API.
>  octeontx
>  octeontx2
>  opdl
> +dlb

Please move 'dlb' to put this list in alphabetical order

> diff --git a/drivers/event/dlb/meson.build b/drivers/event/dlb/meson.build
> new file mode 100644
> index 000..54ba2c8
> --- /dev/null
> +++ b/drivers/event/dlb/meson.build
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2019-2020 Intel Corporation
> +
> +sources = files(
> +)
> +
> +deps += ['mbuf', 'mempool', 'ring', 'pci', 'bus_pci']
> diff --git a/drivers/event/dlb/rte_pmd_dlb_event_version.map
> b/drivers/event/dlb/rte_pmd_dlb_event_version.map
> new file mode 100644
> index 000..299ae63
> --- /dev/null
> +++ b/drivers/event/dlb/rte_pmd_dlb_event_version.map
> @@ -0,0 +1,3 @@
> +DPDK_21.0 {

DPDK_21, per Ray's comment in the dlb2 patchset 

> + local: *;
> +};
> diff --git a/drivers/event/meson.build b/drivers/event/meson.build
> index ebe76a7..35060c6 100644
> --- a/drivers/event/meson.build
> +++ b/drivers/event/meson.build
> @@ -10,6 +10,10 @@ if not (toolchain == 'gcc' and
> cc.version().version_compare('<4.8.6') and
>   dpdk_conf.has('RTE_ARCH_ARM64'))
>   drivers += 'octeontx'
>  endif
> +if ((dpdk_conf.has('RTE_ARCH_X86_64') or dpdk_conf.has('RTE_ARCH_X86'))
> and
> + is_linux)
> + drivers += 'dlb'
> +endif

Move to event/dlb/meson.build, per Bruce's comment in the dlb2 patchset.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 18/22] event/dlb2: add PMD's token pop public interface

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> To: Mcnamara, John ; Kovacevic, Marko
> ; Ray Kinsella ; Neil Horman
> 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 18/22] event/dlb2: add PMD's token pop public interface
> 
> The PMD uses a public interface to allow applications to
> control the token pop mode. Supported token pop modes are
> as follows, and they impact core scheduling affinity for
> ldb ports.
> 
> AUTO_POP: Pop the CQ tokens immediately after dequeueing.
> DELAYED_POP: Pop CQ tokens after (dequeue_depth - 1) events
>  are released. Supported on load-balanced ports
>  only.
> DEFERRED_POP: Pop the CQ tokens during next dequeue operation.
> 
> Signed-off-by: Timothy McDaniel 

With Jerin and Ray's issues addressed:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 16/22] event/dlb2: add dequeue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 16/22] event/dlb2: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...
> 
> DLB2 does not currently support interrupts, but instead use
> umonitor/umwait if supported by the processor. This allows
> the software to monitor and wait on writes to a cache-line.
> 
> DLB2 supports normal and sparse cq mode. In normal mode the
> hardware will pack 4 QEs into each cache line. In sparse cq
> mode, the hardware will only populate one QE per cache line.
> Software must be aware of the cq mode, and take the appropriate
> actions, based on the mode.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 20/22] event/dlb2: add queue and port release

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 20/22] event/dlb2: add queue and port release
> 
> DLB does not support reconfiguring individual queues
> or ports on the fly. The entire device must be reconfigured.
> Previously allocated port QE ond memzone memory
> is freed in this patch.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2.c | 28 ++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 06a59a5..968923e 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -89,8 +89,8 @@ dlb2_free_qe_mem(struct dlb2_port *qm_port)
>   rte_free(qm_port->consume_qe);
>   qm_port->consume_qe = NULL;
> 
> - rte_free(dlb2_port[qm_port->id][PORT_TYPE(qm_port)].cq_base);
> - rte_free(dlb2_port[qm_port->id][PORT_TYPE(qm_port)].pp_addr);
> + rte_memzone_free(dlb2_port[qm_port-
> >id][PORT_TYPE(qm_port)].mz);
> + dlb2_port[qm_port->id][PORT_TYPE(qm_port)].mz = NULL;
>  }

This code should be a part of patch 8, when free_qe_mem is introduced. With 
that change:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 14/22] event/dlb2: add eventdev start

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 14/22] event/dlb2: add eventdev start
> 
> Add support for the eventdev start entry point.
> We delay initializing some resources until
> eventdev start, since the number of linked queues can be
> used to determine if we are dealing with a ldb or dir resource.
> If this is a device restart, then the previous configuration
> will be reapplied.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 08/22] event/dlb2: add infos get and configure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 08/22] event/dlb2: add infos get and configure
> 
> Add support for configuring the DLB2 hardware.
> In particular, this patch configures the DLB2
> hardware's scheduling domain, such that it is provisioned with
> the requested number of ports and queues, provided sufficient
> resources are available. Individual queues and ports are
> configured later in port setup and eventdev start.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 17/22] event/dlb2: add eventdev stop and close

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 17/22] event/dlb2: add eventdev stop and close
> 
> Add support for eventdev stop and close entry points.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 11/22] event/dlb2: add port setup

[Eads, Gage]

[...]

> +static int
> +dlb2_pf_ldb_port_create(struct dlb2_hw_dev *handle,
> + struct dlb2_create_ldb_port_args *cfg,
> + enum dlb2_cq_poll_modes poll_mode)
> +{
> + struct dlb2_dev *dlb2_dev = (struct dlb2_dev *)handle->pf_dev;
> + struct dlb2_cmd_response response = {0};
> + struct dlb2_port_memory port_memory;
> + int ret, cq_alloc_depth;
> + uint8_t *port_base;
> + const struct rte_memzone *mz;
> + int alloc_sz, qe_sz;
> + phys_addr_t cq_base;
> + phys_addr_t pp_base;
> + int is_dir = false;
> +
> + DLB2_INFO(dev->dlb2_device, "Entering %s()\n", __func__);
> +
> + if (poll_mode == DLB2_CQ_POLL_MODE_STD)
> + qe_sz = sizeof(struct dlb2_dequeue_qe);
> + else
> + qe_sz = RTE_CACHE_LINE_SIZE;
> +
> + /* Calculate the port memory required, and round up to the nearest
> +  * cache line.
> +  */
> + cq_alloc_depth = RTE_MAX(cfg->cq_depth,
> DLB2_MIN_HARDWARE_CQ_DEPTH);
> + alloc_sz = cq_alloc_depth * qe_sz;
> + alloc_sz = RTE_CACHE_LINE_ROUNDUP(alloc_sz);
> +
> + port_base = dlb2_alloc_coherent_aligned(&mz, &cq_base, alloc_sz,
> + PAGE_SIZE);
> + if (port_base == NULL)
> + return -ENOMEM;
> +
> + /* Lock the page in memory */
> + ret = rte_mem_lock_page(port_base);
> + if (ret < 0) {
> + DLB2_LOG_ERR("dlb2 pf pmd could not lock page for device
> i/o\n");
> + goto create_port_err;
> + }
> +
> +

Nit: extra newline

> + memset(port_base, 0, alloc_sz);
> +
> + ret = dlb2_pf_create_ldb_port(&dlb2_dev->hw,
> +   handle->domain_id,
> +   cfg,
> +   cq_base,
> +   &response);
> + if (ret)
> + goto create_port_err;
> +
> + pp_base = (uintptr_t)dlb2_dev->hw.func_kva + PP_BASE(is_dir);
> + dlb2_port[response.id][DLB2_LDB_PORT].pp_addr =
> + (void *)(pp_base + (PAGE_SIZE * response.id));
> +
> + dlb2_port[response.id][DLB2_LDB_PORT].cq_base = (void *)(port_base);
> + memset(&port_memory, 0, sizeof(port_memory));
> +
> + dlb2_port[response.id][DLB2_LDB_PORT].mz = mz;
> +
> + dlb2_list_init_head(&port_memory.list);
> +
> + cfg->response = response;
> +
> + return 0;
> +
> +create_port_err:
> +
> + rte_free(port_base);

This should be "rte_memzone_free(mz);"

> +
> + DLB2_INFO(dev->dlb2_device, "Exiting %s() with ret=%d\n",
> +   __func__, ret);
> + return ret;
> +}
> +
> +static int
> +dlb2_pf_dir_port_create(struct dlb2_hw_dev *handle,
> + struct dlb2_create_dir_port_args *cfg,
> + enum dlb2_cq_poll_modes poll_mode)
> +{
> + struct dlb2_dev *dlb2_dev = (struct dlb2_dev *)handle->pf_dev;
> + struct dlb2_cmd_response response = {0};
> + struct dlb2_port_memory port_memory;
> + int ret;
> + uint8_t *port_base;
> + const struct rte_memzone *mz;
> + int alloc_sz, qe_sz;
> + phys_addr_t cq_base;
> + phys_addr_t pp_base;
> + int is_dir = true;
> +
> + DLB2_INFO(dev->dlb2_device, "Entering %s()\n", __func__);
> +
> + if (poll_mode == DLB2_CQ_POLL_MODE_STD)
> + qe_sz = sizeof(struct dlb2_dequeue_qe);
> + else
> + qe_sz = RTE_CACHE_LINE_SIZE;
> +
> + /* Calculate the port memory required, and round up to the nearest
> +  * cache line.
> +  */
> + alloc_sz = cfg->cq_depth * qe_sz;
> + alloc_sz = RTE_CACHE_LINE_ROUNDUP(alloc_sz);
> +
> + port_base = dlb2_alloc_coherent_aligned(&mz, &cq_base, alloc_sz,
> + PAGE_SIZE);
> + if (port_base == NULL)
> + return -ENOMEM;
> +
> + /* Lock the page in memory */
> + ret = rte_mem_lock_page(port_base);
> + if (ret < 0) {
> + DLB2_LOG_ERR("dlb2 pf pmd could not lock page for device
> i/o\n");
> + goto create_port_err;
> + }
> +
> + memset(port_base, 0, alloc_sz);
> +
> + ret = dlb2_pf_create_dir_port(&dlb2_dev->hw,
> +   handle->domain_id,
> +   cfg,
> +   cq_base,
> +   &response);
> + if (ret)
> + goto create_port_err;
> +
> + pp_base = (uintptr_t)dlb2_dev->hw.func_kva + PP_BASE(is_dir);
> + dlb2_port[response.id][DLB2_DIR_PORT].pp_addr =
> + (void *)(pp_base + (PAGE_SIZE * response.id));
> +
> + dlb2_port[response.id][DLB2_DIR_PORT].cq_base =
> + (void *)(port_base);
> + memset(&port_memory, 0, sizeof(port_memory));
> +
> + dlb2_port[response.id][DLB2_DIR_PORT].mz = mz;
> +
> + dlb2_list_init_head(&port_memory.list);
> +
> + cfg->response = response;
> +
> + return 0;
> +
> +create_port_err:
> +
> +

Re: [dpdk-dev] [PATCH v2 07/22] event/dlb2: add xstats

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 07/22] event/dlb2: add xstats
> 
> Add support for DLB2 xstats.  Perform initialization and add
> standard xstats entry points.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

(Looks like you're missing Mike's Reviewed-by, also)

> ---
>  drivers/event/dlb2/dlb2.c|   35 +-
>  drivers/event/dlb2/dlb2_xstats.c | 1240
> ++
>  drivers/event/dlb2/meson.build   |1 +
>  3 files changed, 1273 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/event/dlb2/dlb2_xstats.c
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 26985b9..a7f8f68 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -74,6 +74,21 @@ static struct rte_event_dev_info evdev_dlb2_default_info
> = {
>  struct process_local_port_data
>  dlb2_port[DLB2_MAX_NUM_PORTS][DLB2_NUM_PORT_TYPES];
> 
> +/*
> + * DUMMY - added so that xstats path will compile/link.
> + * Will be replaced by real version in a subsequent
> + * patch.
> + */
> +uint32_t
> +dlb2_get_queue_depth(struct dlb2_eventdev *dlb2,
> +  struct dlb2_eventdev_queue *queue)
> +{
> + RTE_SET_USED(dlb2);
> + RTE_SET_USED(queue);
> +
> + return 0;
> +}
> +
>  /* override defaults with value(s) provided on command line */
>  static void
>  dlb2_init_queue_depth_thresholds(struct dlb2_eventdev *dlb2,
> @@ -341,9 +356,16 @@ set_qid_depth_thresh(const char *key __rte_unused,
>  static void
>  dlb2_entry_points_init(struct rte_eventdev *dev)
>  {
> - RTE_SET_USED(dev);
> -
> - /* Eventdev PMD entry points */
> + /* Expose PMD's eventdev interface */
> + static struct rte_eventdev_ops dlb2_eventdev_entry_ops = {
> + .dump = dlb2_eventdev_dump,
> + .xstats_get   = dlb2_eventdev_xstats_get,
> + .xstats_get_names = dlb2_eventdev_xstats_get_names,
> + .xstats_get_by_name = dlb2_eventdev_xstats_get_by_name,
> + .xstats_reset   = dlb2_eventdev_xstats_reset,
> + };
> +
> + dev->dev_ops = &dlb2_eventdev_entry_ops;
>  }
> 
>  int
> @@ -395,6 +417,13 @@ dlb2_primary_eventdev_probe(struct rte_eventdev
> *dev,
>   return err;
>   }
> 
> + /* Complete xtstats runtime initialization */
> + err = dlb2_xstats_init(dlb2);
> + if (err) {
> + DLB2_LOG_ERR("dlb2: failed to init xstats, err=%d\n", err);
> + return err;
> + }
> +
>   rte_spinlock_init(&dlb2->qm_instance.resource_lock);
> 
>   dlb2_iface_low_level_io_init();
> diff --git a/drivers/event/dlb2/dlb2_xstats.c 
> b/drivers/event/dlb2/dlb2_xstats.c
> new file mode 100644
> index 000..21391f7
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2_xstats.c
> @@ -0,0 +1,1240 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +
> +#include "dlb2_priv.h"
> +#include "dlb2_inline_fns.h"
> +
> +enum dlb2_xstats_type {
> + /* common to device and port */
> + rx_ok,  /**< Receive an event */
> + rx_drop,/**< Error bit set in received QE */
> + rx_interrupt_wait,  /**< Wait on an interrupt */
> + rx_umonitor_umwait, /**< Block using umwait */
> + tx_ok,  /**< Transmit an event */
> + total_polls,/**< Call dequeue_burst */
> + zero_polls, /**< Call dequeue burst and return 0 */
> + tx_nospc_ldb_hw_credits,/**< Insufficient LDB h/w credits */
> + tx_nospc_dir_hw_credits,/**< Insufficient DIR h/w credits */
> + tx_nospc_inflight_max,  /**< Reach the
> new_event_threshold */
> + tx_nospc_new_event_limit,   /**< Insufficient s/w credits */
> + tx_nospc_inflight_credits,  /**< Port has too few s/w credits */
> + /* device specific */
> + nb_events_limit,
> + inflight_events,
> + ldb_pool_size,
> + dir_pool_size,
> + /* port specific */
> + tx_new, /**< Send an OP_NEW event */
> + tx_fwd, /**< Send an OP_FORWARD
> event */
> + tx_rel, /**< Send an OP_RELEASE 

Re: [dpdk-dev] [PATCH v2 09/22] event/dlb2: add queue and port default conf

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 09/22] event/dlb2: add queue and port default conf
> 
> Add support for getting the queue and port default configuration.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 12/22] event/dlb2: add port link

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 12/22] event/dlb2: add port link
> 
> Add port link entry point. Directed queues are identified and created
> at this stage. Their setup deferred until link-time, at which
> point we know the directed port ID. Directed queue setup
> will only fail if this queue is already setup or there are
> no directed queues left to configure.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2.c  | 308 +-
>  drivers/event/dlb2/dlb2_iface.c|   6 +
>  drivers/event/dlb2/dlb2_iface.h|   6 +
>  drivers/event/dlb2/pf/base/dlb2_resource.c | 633
> +
>  drivers/event/dlb2/pf/dlb2_main.c  |  10 +
>  drivers/event/dlb2/pf/dlb2_pf.c|  50 +++
>  6 files changed, 1009 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index e956a7a..b448f59 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -828,9 +828,8 @@ dlb2_hw_create_ldb_queue(struct dlb2_eventdev *dlb2,
>   sched_type = RTE_SCHED_TYPE_ORDERED;
>   else
>   sched_type = RTE_SCHED_TYPE_PARALLEL;
> - } else {
> + } else
>   sched_type = evq_conf->schedule_type;
> - }
> 
>   cfg.num_atomic_inflights =
> DLB2_NUM_ATOMIC_INFLIGHTS_PER_QUEUE;
>   cfg.num_sequence_numbers = evq_conf-
> >nb_atomic_order_sequences;
> @@ -866,9 +865,8 @@ dlb2_hw_create_ldb_queue(struct dlb2_eventdev *dlb2,
>   if (ev_queue->depth_threshold == 0) {
>   cfg.depth_threshold =
> RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH;
>   ev_queue->depth_threshold =
> RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH;
> - } else {
> + } else
>   cfg.depth_threshold = ev_queue->depth_threshold;
> - }

These two changes should be in patch 10 ("event/dlb2: add queue setup"). 
Besides that:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 10/22] event/dlb2: add queue setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 10/22] event/dlb2: add queue setup
> 
> Load balanced (ldb) queues are setup here.
> Directed queues are not set up until link time, at which
> point we know the directed port ID. Directed queue setup
> will only fail if this queue is already setup or there are
> no directed queues left to configure.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 03/22] event/dlb2: add private data structures and constants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Saturday, October 17, 2020 1:21 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v2 03/22] event/dlb2: add private data structures and 
> constants
> 
> The header file dlb2_priv.h is used internally by the PMD.
> It include constants, macros for device resources,
> structure definitions for hardware interfaces and
> software state, and various forward-declarations.
> The header file rte_pmd_dlb2.h will be exported in a
> subsequent patch, but is included here due to a data
> structure dependency.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH] eventdev: add PCI probe named convenience function

[Eads, Gage]

> -Original Message-
> From: Kinsella, Ray 
> Sent: Wednesday, October 14, 2020 4:31 AM
> To: McDaniel, Timothy ; Jerin Jacob
> ; Neil Horman 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry 
> Subject: Re: [PATCH] eventdev: add PCI probe named convenience function
> 
> 
> 
> On 12/10/2020 19:42, Timothy McDaniel wrote:
> > Add new internal wrapper function for use by pci drivers as a
> > .probe function to attach to an event interface.  Same as
> > rte_event_pmd_pci_probe, except the caller can specify the name.
> >
> > Updated rte_event_pmd_pci_probe so as to not duplicate
> > code.
> 
> Any reason why this couldn't be __rte_internal then ?

On a related note...should a static function defined in an internal header file
be included the version.map file? Doesn't that only pertain to symbols in the
.so? Probably harmless if so, but now may be a good time to correct it.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 22/22] doc: Add new DLB eventdev driver to relnotes

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> To: Mcnamara, John ; Kovacevic, Marko
> 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 22/22] doc: Add new DLB eventdev driver to relnotes
> 
> Added announcement of availabililty for the new driver
> for Intel Dynamic Load Balancer V1.0 hardware.

The series is not entirely consistent in the version numbering -- I believe
simply "1.0" is the correct one.

> 
> Signed-off-by: Timothy McDaniel 
> ---
>  doc/guides/rel_notes/release_20_11.rst | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index df227a1..8414e49 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -55,6 +55,11 @@ New Features
>   Also, make sure to start the actual text at the margin.
>   ===
> 
> +* **Added a new driver for the Intel Dynamic Load Balancer v1.0 device.**
> +
> +  Added the new ``dlb`` eventdev driver for the Intel DLB V1.0 device. See 
> the
> +  :doc:`../eventdevs/dlb` eventdev guide for more details on this new driver.
> +
> 
>  Removed Items
>  -
> --
> 2.6.4

Please also add a dlb entry to the MAINTAINERS file.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 21/22] event/dlb: add timeout ticks entry point

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 21/22] event/dlb: add timeout ticks entry point
> 
> Adds the timeout ticks conversion function.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> index d9613ce..b7fa0ca 100644
> --- a/drivers/event/dlb/dlb.c
> +++ b/drivers/event/dlb/dlb.c
> @@ -3895,6 +3895,18 @@ dlb_eventdev_queue_release(struct rte_eventdev
> *dev, uint8_t id)
>*/
>  }
> 
> +static int
> +dlb_eventdev_timeout_ticks(struct rte_eventdev *dev, uint64_t ns,
> +uint64_t *timeout_ticks)
> +{
> + RTE_SET_USED(dev);
> + uint64_t cycles_per_ns = cycles_per_ns = rte_get_timer_hz() / 1E9;

I'm a little surprised GCC allowed this :P. With that self-assignment fixed:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 20/22] event/dlb: add queue and port release

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 20/22] event/dlb: add queue and port release
> 
> These entry points are NO-OPS. DLB does not support
> reconfiguring individual queues or ports. The entire device
> must be reconfigured.
> 
> Signed-off-by: Timothy McDaniel 
> ---

My two comments on the dlb2 patch 
(http://mails.dpdk.org/archives/dev/2020-October/185000.html)
apply here as well.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 19/22] event/dlb: add PMD self-tests

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> To: Jerin Jacob 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry 
> Subject: [PATCH v4 19/22] event/dlb: add PMD self-tests
> 
> Add a variety of self-tests for both ldb and directed
> ports/queues, as well as configure, start, stop, link, etc...
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  app/test/test_eventdev.c |8 +
>  drivers/event/dlb/dlb.c  |1 +
>  drivers/event/dlb/dlb_selftest.c | 1563
> ++
>  drivers/event/dlb/meson.build|1 +
>  4 files changed, 1573 insertions(+)
>  create mode 100644 drivers/event/dlb/dlb_selftest.c
> 
> diff --git a/app/test/test_eventdev.c b/app/test/test_eventdev.c
> index 62019c1..2f6ad49 100644
> --- a/app/test/test_eventdev.c
> +++ b/app/test/test_eventdev.c
> @@ -1030,6 +1030,13 @@ test_eventdev_selftest_dpaa2(void)
>   return test_eventdev_selftest_impl("event_dpaa2", "");
>  }
> 
> +static int
> +test_eventdev_selftest_dlb(void)
> +{
> + return test_eventdev_selftest_impl("dlb_event", "");
> +}
> +
> +

Nit: extra newline

[...]

> +/* destruction */
> +static inline int
> +cleanup(struct test *t __rte_unused)

No need for 't'

> +{
> + int ret;
> +
> + rte_event_dev_stop(evdev);
> + ret = rte_event_dev_close(evdev);
> + if (ret)

This can simply be if (rte_event_dev_close(evdev))

> + return -1;
> +
> + return 0;
> +};
> +

With those and the whitespace issue fixed:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 18/22] event/dlb: add PMD's token pop public interface

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> To: Ray Kinsella ; Neil Horman 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 18/22] event/dlb: add PMD's token pop public interface
> 
> The PMD uses a public interface to allow applications to
> control the token pop mode. Supported token pop modes are
> as follows, and they impact core scheduling affinity for
> ldb ports.
> 
> AUTO_POP: Pop the CQ tokens immediately after dequeueing.
> DELAYED_POP: Pop CQ tokens after (dequeue_depth - 1) events
>are released. Supported on load-balanced ports
>only.
> DEFERRED_POP: Pop the CQ tokens during next dequeue operation.
> 
> Signed-off-by: Timothy McDaniel 
> ---

Should rte_pmd_dlb.h be listed in doc/api/doxy-api-index.md, so it's included
in the doxygen docs? (Might apply to patch #3 rather than this one).

Besides that:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 17/22] event/dlb: add eventdev stop and close

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 17/22] event/dlb: add eventdev stop and close
> 
> Add support for eventdev stop and close entry points.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 16/22] event/dlb: add dequeue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 16/22] event/dlb: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...

This message could use some more detail.

[...]

> +static inline int
> +dlb_process_dequeue_qes(struct dlb_eventdev_port *ev_port,
> + struct dlb_port *qm_port,
> + struct rte_event *events,
> + struct dlb_dequeue_qe *qes,
> + int cnt)
> +{
> + uint8_t *qid_mappings = qm_port->qid_mappings;
> + int i, num;
> +
> + RTE_SET_USED(ev_port);  /* avoids unused variable error */
> +
> + for (i = 0, num = 0; i < cnt; i++) {
> + struct dlb_dequeue_qe *qe = &qes[i];
> + int sched_type_map[4] = {
> + [DLB_SCHED_ATOMIC] = RTE_SCHED_TYPE_ATOMIC,
> + [DLB_SCHED_UNORDERED] =
> RTE_SCHED_TYPE_PARALLEL,
> + [DLB_SCHED_ORDERED] = RTE_SCHED_TYPE_ORDERED,
> + [DLB_SCHED_DIRECTED] = RTE_SCHED_TYPE_ATOMIC,
> + };
> +
> + DLB_LOG_DBG("dequeue success, data = 0x%llx, qid=%d,
> event_type=%d, subevent=%d\npp_id = %d, sched_type = %d, qid = %d,
> err=%d\n",
> + (long long)qe->data, qe->qid,
> + qe->u.event_type.major,
> + qe->u.event_type.sub,
> + qe->pp_id, qe->sched_type, qe->qid, qe->error);
> +
> + /* Fill in event information.
> +  * Note that flow_id must be embedded in the data by
> +  * the app, such as the mbuf RSS hash field if the data
> +  * buffer is a mbuf.
> +  */
> + if (unlikely(qe->error)) {
> + DLB_LOG_ERR("QE error bit ON\n");
> + DLB_INC_STAT(ev_port->stats.traffic.rx_drop, 1);
> + dlb_consume_qe_immediate(qm_port, 1);
> + continue; /* Ignore */
> + }
> +
> + events[num].u64 = qe->data;
> + events[num].queue_id = qid_mappings[qe->qid];
> + events[num].priority = DLB_TO_EV_PRIO((uint8_t)qe->priority);
> + events[num].event_type = qe->u.event_type.major;
> + events[num].sub_event_type = qe->u.event_type.sub;
> + events[num].sched_type = sched_type_map[qe->sched_type];
> + DLB_INC_STAT(ev_port->stats.rx_sched_cnt[qe->sched_type],
> 1);
> +
> + DLB_INC_STAT(ev_port->stats.traffic.rx_ok, 1);

Move this outside the loop and increment by num rather than 1?

> +
> + num++;
> + }
> +
> + return num;
> +}

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 15/22] event/dlb: add enqueue and its burst variants

[Eads, Gage]

> +static inline void
> +dlb_hw_do_enqueue(struct dlb_port *qm_port,
> +   bool do_sfence,
> +   struct process_local_port_data *port_data)
> +{
> + DLB_LOG_DBG("dlb: Flushing QE(s) to DLB\n");
> +
> + /* Since MOVDIR64B is weakly-ordered, use an SFENCE to ensure that
> +  * application writes complete before enqueueing the release HCW.
> +  */
> + if (do_sfence)
> + rte_wmb();
> +
> +

Nit: extra newline. With that fixed:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 14/22] event/dlb: add eventdev start

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 14/22] event/dlb: add eventdev start
> 
> Add support for the eventdev start entry point.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c  | 225 
> +--
>  drivers/event/dlb/dlb_iface.c|   3 +
>  drivers/event/dlb/dlb_iface.h|   3 +
>  drivers/event/dlb/pf/base/dlb_resource.c | 142 +++
>  drivers/event/dlb/pf/dlb_pf.c|  23 
>  5 files changed, 352 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> index 4f56869..5f2a7fa 100644
> --- a/drivers/event/dlb/dlb.c
> +++ b/drivers/event/dlb/dlb.c
> @@ -1443,6 +1443,7 @@ dlb_eventdev_ldb_queue_setup(struct rte_eventdev
> *dev,
>   return 0;
>  }
> 
> +

^^^ Extra newline

Same with the dlb2 patch, I think this warrants a detailed commit message. The 
code looks
fine though. With that:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 13/22] event/dlb: add port unlink and port unlinks in progress

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 13/22] event/dlb: add port unlink and port unlinks in 
> progress
> 
> Add supports for the port unlink(s) eventdev entry points.
> The unlink operation is an asynchronous operation executed by
> a control thread, and the unlinks-in-progress function reads
> a counter shared with the control thread.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 12/22] event/dlb: add port link

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:19 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 12/22] event/dlb: add port link
> 
> Add port link entry point. Directed queues are identified and created
> at this stage. Their setup defered until link-time, at which
> point we know the directed port ID. Directed queue setup
> will only fail if this queue is already setup or there are
> no directed queues left to configure.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 11/22] event/dlb: add port setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 11/22] event/dlb: add port setup
> 
> Configure the load balanded (ldb) or directed (dir) port.
> The consumer queue (CQ) and producer port (PP) are also
> set up here.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c  |  539 +++
>  drivers/event/dlb/dlb_iface.c|   11 +
>  drivers/event/dlb/dlb_iface.h|   14 +
>  drivers/event/dlb/pf/base/dlb_resource.c | 1430
> ++
>  drivers/event/dlb/pf/dlb_pf.c|  204 +
>  5 files changed, 2198 insertions(+)
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> index 0b474a5..e90a088 100644
> --- a/drivers/event/dlb/dlb.c
> +++ b/drivers/event/dlb/dlb.c
> @@ -157,6 +157,75 @@ dlb_free_qe_mem(struct dlb_port *qm_port)
>   }
>  }
> 
> +static int
> +dlb_init_consume_qe(struct dlb_port *qm_port, char *mz_name)
> +{
> + struct dlb_cq_pop_qe *qe;
> +
> + qe = rte_malloc(mz_name,
> + DLB_NUM_QES_PER_CACHE_LINE *
> + sizeof(struct dlb_cq_pop_qe),
> + RTE_CACHE_LINE_SIZE);
> +
> + if (qe == NULL) {
> + DLB_LOG_ERR("dlb: no memory for consume_qe\n");
> + return -ENOMEM;
> + }
> +
> + qm_port->consume_qe = qe;
> +
> + memset(qe, 0, DLB_NUM_QES_PER_CACHE_LINE *
> +sizeof(struct dlb_cq_pop_qe));

This is a good candidate for rte_zmalloc().

> +
> + qe->qe_valid = 0;
> + qe->qe_frag = 0;
> + qe->qe_comp = 0;
> + qe->cq_token = 1;
> + /* Tokens value is 0-based; i.e. '0' returns 1 token, '1' returns 2,
> +  * and so on.
> +  */
> + qe->tokens = 0; /* set at run time */
> + qe->meas_lat = 0;
> + qe->no_dec = 0;
> + /* Completion IDs are disabled */
> + qe->cmp_id = 0;
> +
> + return 0;
> +}
> +
> +int
> +dlb_init_qe_mem(struct dlb_port *qm_port, char *mz_name)
> +{
> + int ret, sz;
> +
> + sz = DLB_NUM_QES_PER_CACHE_LINE * sizeof(struct dlb_enqueue_qe);
> +
> + qm_port->qe4 = rte_malloc(mz_name, sz, RTE_CACHE_LINE_SIZE);
> +
> + if (qm_port->qe4 == NULL) {
> + DLB_LOG_ERR("dlb: no qe4 memory\n");
> + ret = -ENOMEM;
> + goto error_exit;
> + }
> +
> + memset(qm_port->qe4, 0, sz);
> +
> + ret = dlb_init_consume_qe(qm_port, mz_name);
> + if (ret < 0) {
> + DLB_LOG_ERR("dlb: dlb_init_consume_qe ret=%d\n",
> + ret);

This can fit on one line

> + goto error_exit;
> + }
> +
> + return 0;
> +
> +error_exit:
> +
> + dlb_free_qe_mem(qm_port);
> +
> + return ret;
> +}
> +
>  /* Wrapper for string to int conversion. Substituted for atoi(...), which is
>   * unsafe.
>   */
> @@ -662,6 +731,348 @@ dlb_eventdev_queue_default_conf_get(struct
> rte_eventdev *dev,
>   queue_conf->priority = 0;
>  }
> 
> +static int
> +dlb_hw_create_ldb_port(struct dlb_eventdev *dlb,
> +struct dlb_eventdev_port *ev_port,
> +uint32_t dequeue_depth,
> +uint32_t cq_depth,
> +uint32_t enqueue_depth,
> +uint16_t rsvd_tokens,
> +bool use_rsvd_token_scheme)
> +{
> + struct dlb_hw_dev *handle = &dlb->qm_instance;
> + struct dlb_create_ldb_port_args cfg = {0};
> + struct dlb_cmd_response response = {0};
> + int ret;
> + struct dlb_port *qm_port = NULL;
> + char mz_name[RTE_MEMZONE_NAMESIZE];
> + uint32_t qm_port_id;
> +
> + if (handle == NULL)
> + return -EINVAL;
> +
> + if (cq_depth < DLB_MIN_LDB_CQ_DEPTH ||
> + cq_depth > DLB_MAX_INPUT_QUEUE_DEPTH) {
> + DLB_LOG_ERR("dlb: invalid cq_depth, must be %d-%d\n",
> + DLB_MIN_LDB_CQ_DEPTH,
> DLB_MAX_INPUT_QUEUE_DEPTH);
> + return -EINVAL;
> + }
> +
> + if (enqueue_depth < DLB_MIN_ENQUEUE_DEPTH) {
> + DLB_LOG_ERR("dlb: invalid enqueue_depth, must be at least
> %d\n",
> + DLB_MIN_ENQUEUE_DEPTH);
> + return -EINVAL;
> + }

Like my comment in the dlb2 "add port setup" patch, looks like the cq depth
up

Re: [dpdk-dev] [PATCH v4 10/22] event/dlb: add queue setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 10/22] event/dlb: add queue setup
> 
> Load balanced (ldb) queues are setup here.
> Directed queues are not set up until link time, at which
> point we know the directed port ID. Directed queue setup
> will only fail if this queue is already setup or there are
> no directed queues left to configure.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c  | 294 +
>  drivers/event/dlb/dlb_iface.c|  12 +
>  drivers/event/dlb/dlb_iface.h|  12 +
>  drivers/event/dlb/pf/base/dlb_resource.c | 710
> ---
>  drivers/event/dlb/pf/dlb_pf.c|  81 
>  5 files changed, 947 insertions(+), 162 deletions(-)
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> index fa9213c..0b474a5 100644
> --- a/drivers/event/dlb/dlb.c
> +++ b/drivers/event/dlb/dlb.c
> @@ -662,6 +662,299 @@ dlb_eventdev_queue_default_conf_get(struct
> rte_eventdev *dev,
>   queue_conf->priority = 0;
>  }
> 
> +static int32_t
> +dlb_hw_create_ldb_queue(struct dlb_eventdev *dlb,
> + struct dlb_queue *queue,
> + const struct rte_event_queue_conf *evq_conf)
> +{
> + struct dlb_hw_dev *handle = &dlb->qm_instance;
> + struct dlb_create_ldb_queue_args cfg;
> + struct dlb_cmd_response response;
> + int32_t ret;
> + uint32_t qm_qid;
> + int sched_type = -1;
> +
> + if (evq_conf == NULL)
> + return -EINVAL;
> +
> + if (evq_conf->event_queue_cfg & RTE_EVENT_QUEUE_CFG_ALL_TYPES)
> {
> + if (evq_conf->nb_atomic_order_sequences != 0)
> + sched_type = RTE_SCHED_TYPE_ORDERED;
> + else
> + sched_type = RTE_SCHED_TYPE_PARALLEL;
> + } else {
> + sched_type = evq_conf->schedule_type;
> + }

Nit: single-statement conditional. Besides that:

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 09/22] event/dlb: add queue and port default conf

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 09/22] event/dlb: add queue and port default conf
> 
> Add support for getting the queue and port default configuration.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 08/22] event/dlb: add infos get and configure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 08/22] event/dlb: add infos get and configure
> 
> Add support for configuring the DLB hardware.

Please expand the commit message.

> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c  |  402 +++
>  drivers/event/dlb/dlb_iface.c|   11 +
>  drivers/event/dlb/dlb_iface.h|   11 +
>  drivers/event/dlb/pf/base/dlb_resource.c | 4098
> +-
>  drivers/event/dlb/pf/dlb_pf.c|   88 +
>  5 files changed, 4517 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> index f89edf2..2dba396 100644
> --- a/drivers/event/dlb/dlb.c
> +++ b/drivers/event/dlb/dlb.c
> @@ -140,6 +140,23 @@ dlb_hw_query_resources(struct dlb_eventdev *dlb)
>   return 0;
>  }
> 
> +void
> +dlb_free_qe_mem(struct dlb_port *qm_port)
> +{
> + if (qm_port == NULL)
> + return;
> +
> + if (qm_port->qe4) {
> + rte_free(qm_port->qe4);
> + qm_port->qe4 = NULL;
> + }
> +
> + if (qm_port->consume_qe) {
> + rte_free(qm_port->consume_qe);
> + qm_port->consume_qe = NULL;
> + }
> +}

Checking for NULL is not strictly required, rte_free() will simply return in 
that case.

> +
>  /* Wrapper for string to int conversion. Substituted for atoi(...), which is
>   * unsafe.
>   */
> @@ -231,6 +248,389 @@ set_num_dir_credits(const char *key __rte_unused,
>   DLB_MAX_NUM_DIR_CREDITS);
>   return -EINVAL;
>   }
> + return 0;
> +}
> +
> +/* VDEV-only notes:
> + * This function first unmaps all memory mappings and closes the
> + * domain's file descriptor, which causes the driver to reset the
> + * scheduling domain. Once that completes (when close() returns), we
> + * can safely free the dynamically allocated memory used by the
> + * scheduling domain.
> + *
> + * PF-only notes:
> + * We will maintain a use count and use that to determine when
> + * a reset is required.  In PF mode, we never mmap, or munmap
> + * device memory,  and we own the entire physical PCI device.
> + */
> +
> +static void
> +dlb_hw_reset_sched_domain(const struct rte_eventdev *dev, bool reconfig)
> +{
> + struct dlb_eventdev *dlb = dlb_pmd_priv(dev);
> + enum dlb_configuration_state config_state;
> + int i, j;
> +
> + /* Close and reset the domain */
> + dlb_iface_domain_close(dlb);
> +
> + /* Free all dynamically allocated port memory */
> + for (i = 0; i < dlb->num_ports; i++)
> + dlb_free_qe_mem(&dlb->ev_ports[i].qm_port);
> +
> + /* If reconfiguring, mark the device's queues and ports as "previously
> +  * configured." If the user does not reconfigure them, the PMD will
> +  * reapply their previous configuration when the device is started.
> +  */
> + config_state = (reconfig) ? DLB_PREV_CONFIGURED :
> DLB_NOT_CONFIGURED;
> +
> + for (i = 0; i < dlb->num_ports; i++) {
> + dlb->ev_ports[i].qm_port.config_state = config_state;
> + /* Reset setup_done so ports can be reconfigured */
> + dlb->ev_ports[i].setup_done = false;
> + for (j = 0; j < DLB_MAX_NUM_QIDS_PER_LDB_CQ; j++)
> + dlb->ev_ports[i].link[j].mapped = false;
> + }
> +
> + for (i = 0; i < dlb->num_queues; i++)
> + dlb->ev_queues[i].qm_queue.config_state = config_state;
> +
> + for (i = 0; i < DLB_MAX_NUM_QUEUES; i++)
> + dlb->ev_queues[i].setup_done = false;
> +
> + dlb->num_ports = 0;
> + dlb->num_ldb_ports = 0;
> + dlb->num_dir_ports = 0;
> + dlb->num_queues = 0;
> + dlb->num_ldb_queues = 0;
> + dlb->num_dir_queues = 0;
> + dlb->configured = false;
> +}
> +
> +static int
> +dlb_ldb_credit_pool_create(struct dlb_hw_dev *handle)
> +{
> + struct dlb_create_ldb_pool_args cfg;
> + struct dlb_cmd_response response;
> + int ret;
> +
> + if (handle == NULL)
> + return -EINVAL;
> +
> + if (!handle->cfg.resources.num_ldb_credits) {
> + handle->cfg.ldb_credit_pool_id = 0;
> + handle->cfg.num_ldb_credits = 0;
> + return 0;
> + }
> +
> + cfg.response = (uintptr_t)&response;
> + cfg.num_ldb_credit

Re: [dpdk-dev] [PATCH v4 07/22] event/dlb: add xstats

[Eads, Gage]

> diff --git a/drivers/event/dlb/dlb_xstats.c b/drivers/event/dlb/dlb_xstats.c
> new file mode 100644
> index 000..4d01cc0
> --- /dev/null
> +++ b/drivers/event/dlb/dlb_xstats.c
> @@ -0,0 +1,1252 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +

+inttypes.h for the printf format specifiers

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Besides rte_malloc.h and rte_eventdev.h, I suspect the rest are unnecessary.

[...]

> +int
> +dlb_eventdev_xstats_get_names(const struct rte_eventdev *dev,
> + enum rte_event_dev_xstats_mode mode, uint8_t
> queue_port_id,
> + struct rte_event_dev_xstats_name *xstats_names,
> + unsigned int *ids, unsigned int size)
> +{
> + const struct dlb_eventdev *dlb = dlb_pmd_priv(dev);
> + unsigned int i;
> + unsigned int xidx = 0;
> +
> + RTE_SET_USED(mode);
> + RTE_SET_USED(queue_port_id);

RTE_SET_USED not needed for mode and queue_port_id

> +
> + uint32_t xstats_mode_count = 0;
> + uint32_t start_offset = 0;
> +
> + switch (mode) {
> + case RTE_EVENT_DEV_XSTATS_DEVICE:
> + xstats_mode_count = dlb->xstats_count_mode_dev;
> + break;
> + case RTE_EVENT_DEV_XSTATS_PORT:
> + if (queue_port_id >= DLB_MAX_NUM_PORTS)
> + break;
> + xstats_mode_count = dlb-
> >xstats_count_per_port[queue_port_id];
> + start_offset = dlb->xstats_offset_for_port[queue_port_id];
> + break;
> + case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

If this macro is tied to the hardware definition, will it change?

> + if (queue_port_id >= DLB_MAX_NUM_QUEUES)
> + break;
> +#endif
> + xstats_mode_count = dlb-
> >xstats_count_per_qid[queue_port_id];
> + start_offset = dlb->xstats_offset_for_qid[queue_port_id];
> + break;
> + default:
> + return -EINVAL;
> + };
> +
> + if (xstats_mode_count > size || !ids || !xstats_names)
> + return xstats_mode_count;
> +
> + for (i = 0; i < dlb->xstats_count && xidx < size; i++) {
> + if (dlb->xstats[i].mode != mode)
> + continue;
> +
> + if (mode != RTE_EVENT_DEV_XSTATS_DEVICE &&
> + queue_port_id != dlb->xstats[i].obj_idx)
> + continue;
> +
> + xstats_names[xidx] = dlb->xstats[i].name;
> + if (ids)
> + ids[xidx] = start_offset + xidx;
> + xidx++;
> + }
> + return xidx;
> +}
> +
> +static int
> +dlb_xstats_update(struct dlb_eventdev *dlb,
> + enum rte_event_dev_xstats_mode mode,
> + uint8_t queue_port_id, const unsigned int ids[],
> + uint64_t values[], unsigned int n, const uint32_t reset)
> +{
> + unsigned int i;
> + unsigned int xidx = 0;
> +
> + RTE_SET_USED(mode);
> + RTE_SET_USED(queue_port_id);

RTE_SET_USED not needed for mode and queue_port_id

> +
> + uint32_t xstats_mode_count = 0;
> +
> + switch (mode) {
> + case RTE_EVENT_DEV_XSTATS_DEVICE:
> + xstats_mode_count = dlb->xstats_count_mode_dev;
> + break;
> + case RTE_EVENT_DEV_XSTATS_PORT:
> + if (queue_port_id >= DLB_MAX_NUM_PORTS)
> + goto invalid_value;
> + xstats_mode_count = dlb-
> >xstats_count_per_port[queue_port_id];
> + break;
> + case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

(See comment above)

[...]

> +void
> +dlb_eventdev_dump(struct rte_eventdev *dev, FILE *f)
> +{
> + struct dlb_eventdev *dlb;
> + struct dlb_hw_dev *handle;
> + int i;
> +
> + if (!f) {

Like Mike pointed out the dlb2 review, avoid !pointer checks.

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 06/22] event/dlb: add probe

[Eads, Gage]

My "event/dlb2: add probe" review, referenced below:
[1] http://mails.dpdk.org/archives/dev/2020-October/184962.html

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> To: Burakov, Anatoly 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 06/22] event/dlb: add probe
> 
> The DLB hardware is a PCI device. This commit adds
> support for probe and other initialization.

Given the size, this commit warrants a more detailed commit message --
like the dlb2 PMD's "add probe" commit.

> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb/dlb.c  |  493 ++
>  drivers/event/dlb/dlb_iface.c|   41 +
>  drivers/event/dlb/dlb_iface.h|   27 +
>  drivers/event/dlb/dlb_priv.h |   20 +-
>  drivers/event/dlb/meson.build|6 +-
>  drivers/event/dlb/pf/base/dlb_hw_types.h |  334 
>  drivers/event/dlb/pf/base/dlb_osdep.h|  326 
>  drivers/event/dlb/pf/base/dlb_osdep_bitmap.h |  441 +
>  drivers/event/dlb/pf/base/dlb_osdep_list.h   |  131 ++
>  drivers/event/dlb/pf/base/dlb_osdep_types.h  |   31 +
>  drivers/event/dlb/pf/base/dlb_regs.h | 2368
> ++
>  drivers/event/dlb/pf/base/dlb_resource.c |  302 
>  drivers/event/dlb/pf/base/dlb_resource.h |  876 ++
>  drivers/event/dlb/pf/dlb_main.c  |  591 +++
>  drivers/event/dlb/pf/dlb_main.h  |   52 +
>  drivers/event/dlb/pf/dlb_pf.c|  232 +++
>  16 files changed, 6254 insertions(+), 17 deletions(-)
>  create mode 100644 drivers/event/dlb/dlb.c
>  create mode 100644 drivers/event/dlb/dlb_iface.c
>  create mode 100644 drivers/event/dlb/dlb_iface.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_hw_types.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_osdep.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_osdep_bitmap.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_osdep_list.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_osdep_types.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_regs.h
>  create mode 100644 drivers/event/dlb/pf/base/dlb_resource.c
>  create mode 100644 drivers/event/dlb/pf/base/dlb_resource.h
>  create mode 100644 drivers/event/dlb/pf/dlb_main.c
>  create mode 100644 drivers/event/dlb/pf/dlb_main.h
>  create mode 100644 drivers/event/dlb/pf/dlb_pf.c
> 
> diff --git a/drivers/event/dlb/dlb.c b/drivers/event/dlb/dlb.c
> new file mode 100644
> index 000..e2acb61
> --- /dev/null
> +++ b/drivers/event/dlb/dlb.c
> @@ -0,0 +1,493 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include "dlb_priv.h"
> +#include "dlb_iface.h"
> +#include "dlb_inline_fns.h"
> +
> +/*
> + * Resources exposed to eventdev.
> + */
> +#if (RTE_EVENT_MAX_QUEUES_PER_DEV > UINT8_MAX)
> +#error "RTE_EVENT_MAX_QUEUES_PER_DEV cannot fit in member
> max_event_queues"
> +#endif
> +static struct rte_event_dev_info evdev_dlb_default_info = {
> + .driver_name = "", /* probe will set */
> + .min_dequeue_timeout_ns = DLB_MIN_DEQUEUE_TIMEOUT_NS,
> + .max_dequeue_timeout_ns = DLB_MAX_DEQUEUE_TIMEOUT_NS,
> +#if (RTE_EVENT_MAX_QUEUES_PER_DEV < DLB_MAX_NUM_LDB_QUEUES)
> + .max_event_queues = RTE_EVENT_MAX_QUEUES_PER_DEV,
> +#else
> + .max_event_queues = DLB_MAX_NUM_LDB_QUEUES,
> +#endif
> + .max_event_queue_flows = DLB_MAX_NUM_FLOWS,
> + .max_event_queue_priority_levels = DLB_QID_PRIORITIES,
> + .max_event_priority_levels = DLB_QID_PRIORITIES,
> + .max_event_ports = DLB_MAX_NUM_LDB_PORTS,
> + .max_event_port_dequeue_depth = DLB_MAX_CQ_DEPTH,
> + .max_event_port_enqueue_depth = DLB_MAX_ENQUEUE_DEPTH,
> + .max_event_port_links = DLB_MAX_NUM_QIDS_PER_LDB_CQ,
> + .max_num_events = DLB_MAX_NUM_LDB_CREDITS,
> + .max_single_link_event_port_queue_pairs =
> DLB_MAX_NUM_DIR_PORTS,
> + .event_dev_cap = (RTE_EVENT_DEV_CAP_QUEUE_QOS |
> +   RTE_EVENT_DEV_CAP_EVENT_QOS |
> +   RTE_EVENT_DEV_CAP_BURST_MOD

Re: [dpdk-dev] [PATCH v4 05/22] event/dlb: add inline functions

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 05/22] event/dlb: add inline functions
> 
> Add miscellaneous inline functions that may be called
> from multiple files.  These functions include inline
> assembly of new x86 instructions, such as movdir64b,
> since they are not available as builtin functions in
> the minimum supported GCC version.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 03/22] event/dlb: add private data structures and constants

[Eads, Gage]

> +/*!
> + * Configure the token pop mode for an DLB port. By default, all ports use
> + * AUTO_POP. This function must be called before calling
> rte_event_port_setup()
> + * for the port, but after calling rte_event_dev_configure().
> + *
> + * @note
> + *The defer_sched vdev arg, which configures all load-balanced ports with
> + *dequeue_depth == 1 for DEFERRED_POP mode, takes precedence over this
> + *function.
> + *
> + * @param dev_id
> + *The identifier of the event device.
> + * @param port_id
> + *The identifier of the event port.
> + * @param mode
> + *The token pop mode.
> + *
> + * @return
> + * - 0: Success
> + * - EINVAL: Invalid dev_id, port_id, or mode
> + * - EINVAL: The DLB is not configured, is already running, or the port is
> + *   already setup
> + */

Experimental functions need this at the top of their documentation comment:

 * @warning
 * @b EXPERIMENTAL: this API may change without prior notice.
 *

Thanks,
Gage

Re: [dpdk-dev] [PATCH 22/22] doc: add new DLB2 eventdev driver to relnotes

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> To: Mcnamara, John ; Kovacevic, Marko
> 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 22/22] doc: add new DLB2 eventdev driver to relnotes
> 
> Added announcement of availabililty for the new driver
> for Intel Dynamic Load Balancer V2.0 hardware.

There's some inconsistency in the version numbering (2.0, V2.0, v2.0) in this 
series 
-- I believe 2.0 is the marketing-approved one.

> 
> Signed-off-by: Timothy McDaniel 
> ---
>  doc/guides/rel_notes/release_20_11.rst | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index df227a1..f80df21 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -55,6 +55,11 @@ New Features
>   Also, make sure to start the actual text at the margin.
>   ===
> 
> +* **Added a new driver for the Intel Dynamic Load Balancer v2.0 device.**
> +
> +  Added the new ``dlb2`` eventdev driver for the Intel DLB V2.0 device. See 
> the
> +  :doc:`../eventdevs/dlb2` eventdev guide for more details on this new 
> driver.

Looks like the guide didn't make it into this series ;)

Please also add an entry MAINTAINERS entry for the PMD.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 21/22] event/dlb2: add timeout ticks entry point

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 21/22] event/dlb2: add timeout ticks entry point
> 
> Adds the timeout ticks conversion function.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Re: [dpdk-dev] [PATCH 20/22] event/dlb2: add queue and port release

[Eads, Gage]

>  static void
> +dlb2_eventdev_queue_release(struct rte_eventdev *dev, uint8_t id)
> +{
> + RTE_SET_USED(dev);
> + RTE_SET_USED(id);
> +
> + /* This function intentionally left blank. dlb2 does not support
> +  * reconfiguring individual queues or ports -- the entire device
> +  * must be reconfigured.
> +  */

I think the 2nd sentence in the comment need to be reconsidered (or just
removed). The release callbacks are called (for every port and queue) when
rte_event_dev_configure() is called a 2nd/3rd/etc. time to reconfigure the
device, and the callbacks give the PMD the opportunity clean-up the individual
port/queue. Implementing the logic can be useful for
entire-device-reconfiguration.

> +}
> +
> +static void
> +dlb2_eventdev_port_release(void *port)
> +{
> + RTE_SET_USED(port);
> +
> + /* This function intentionally left blank. dlb2 does not support
> +  * reconfiguring individual queues or ports -- the entire device
> +  * must be reconfigured.
> +  */

This might be the appropriate place to free each port's memzone (see my
comments in patch 11).

Thanks,
Gage

Re: [dpdk-dev] [PATCH 19/22] event/dlb2: add PMD self-tests

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> To: Jerin Jacob 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry 
> Subject: [PATCH 19/22] event/dlb2: add PMD self-tests
> 
> Add a variety of self-tests for both ldb and directed
> ports/queues, as well as configure, start, stop, link, etc...
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  app/test/test_eventdev.c   |9 +
>  drivers/event/dlb2/dlb2.c  |1 +
>  drivers/event/dlb2/dlb2_selftest.c | 1570
> 
>  drivers/event/dlb2/meson.build |3 +-
>  4 files changed, 1582 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/event/dlb2/dlb2_selftest.c
> 
> diff --git a/app/test/test_eventdev.c b/app/test/test_eventdev.c
> index 43ccb1c..b8d8df8 100644
> --- a/app/test/test_eventdev.c
> +++ b/app/test/test_eventdev.c
> @@ -1030,6 +1030,13 @@ test_eventdev_selftest_dpaa2(void)
>   return test_eventdev_selftest_impl("event_dpaa2", "");
>  }
> 
> +static int
> +test_eventdev_selftest_dlb2(void)
> +{
> + return test_eventdev_selftest_impl("dlb2_event", "");
> +}
> +
> +

Nit: extra newline

>  REGISTER_TEST_COMMAND(eventdev_common_autotest,
> test_eventdev_common);
>  REGISTER_TEST_COMMAND(eventdev_selftest_sw,
> test_eventdev_selftest_sw);
>  REGISTER_TEST_COMMAND(eventdev_selftest_octeontx,
> @@ -1037,3 +1044,5 @@
> REGISTER_TEST_COMMAND(eventdev_selftest_octeontx,
>  REGISTER_TEST_COMMAND(eventdev_selftest_octeontx2,
>   test_eventdev_selftest_octeontx2);
>  REGISTER_TEST_COMMAND(eventdev_selftest_dpaa2,
> test_eventdev_selftest_dpaa2);
> +REGISTER_TEST_COMMAND(eventdev_selftest_dlb2,
> test_eventdev_selftest_dlb2);
> +

Nit: newline at the end of the file

> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 43b85d7..620a0a5 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c

[...]

> +/* destruction */
> +static inline void
> +cleanup(struct test *t __rte_unused)

Since 't' is unused, just drop the parameter.

With that and the whitespace issues fixed:
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 18/22] event/dlb2: add PMD's token pop public interface

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> To: Ray Kinsella ; Neil Horman 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 18/22] event/dlb2: add PMD's token pop public interface
> 
> The PMD uses a public interface to allow applications to
> control the token pop mode. Supported token pop modes are
> as follows, and they impact core scheduling affinity for
> ldb ports.
> 
> AUTO_POP: Pop the CQ tokens immediately after dequeueing.
> DELAYED_POP: Pop CQ tokens after (dequeue_depth - 1) events
>  are released. Supported on load-balanced ports
>  only.
> DEFERRED_POP: Pop the CQ tokens during next dequeue operation.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/meson.build|  5 ++-
>  drivers/event/dlb2/rte_pmd_dlb2.c | 39 
> +++
>  drivers/event/dlb2/rte_pmd_dlb2_event_version.map |  6 

Should rte_pmd_dlb2.h be listed in doc/api/doxy-api-index.md, so it's included
in the doxygen docs?

Besides that, and the build error related to the experimental tag mentioned in
another review, this looks good to me.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 17/22] event/dlb2: add eventdev stop and close

[Eads, Gage]

> +static void
> +dlb2_eventdev_stop(struct rte_eventdev *dev)
> +{
> + /* FIXME: Handle the case that app threads are waiting in
> +  * rte_event_dequeue_burst() (either blocked on interrupt or in umwait,
> +  * or waiting with a timeout). Looking into proposing a new function to
> +  * wake blocked threads that the app must call before stopping a device.
> +  */

Leftover FIXME -- otherwise looks good.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 16/22] event/dlb2: add dequeue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 16/22] event/dlb2: add dequeue and its burst variants
> 
> Add support for dequeue, dequeue_burst, ...

Please elaborate -- this commit message doesn't mention (e.g.) the use of
umonitor/umwait or the "sparse" dequeue function variants. I think the
average reviewer needs some more context in order to understand this change.

[...]

> +
> +static inline int
> +dlb2_process_dequeue_qes(struct dlb2_eventdev_port *ev_port,
> +  struct dlb2_port *qm_port,
> +  struct rte_event *events,
> +  struct dlb2_dequeue_qe *qes,
> +  int cnt)
> +{
> + uint8_t *qid_mappings = qm_port->qid_mappings;
> + int i, num, evq_id;
> +
> + RTE_SET_USED(ev_port);  /* avoids unused variable error if stats off */

Looks like ev_port is used unconditionally later on: "evq_id = 
ev_port->link[0].queue_id;"

> +
> + for (i = 0, num = 0; i < cnt; i++) {
> + struct dlb2_dequeue_qe *qe = &qes[i];
> + int sched_type_map[DLB2_NUM_HW_SCHED_TYPES] = {
> + [DLB2_SCHED_ATOMIC] = RTE_SCHED_TYPE_ATOMIC,
> + [DLB2_SCHED_UNORDERED] =
> RTE_SCHED_TYPE_PARALLEL,
> + [DLB2_SCHED_ORDERED] = RTE_SCHED_TYPE_ORDERED,
> + [DLB2_SCHED_DIRECTED] = RTE_SCHED_TYPE_ATOMIC,
> + };
> +
> + /* Fill in event information.
> +  * Note that flow_id must be embedded in the data by
> +  * the app, such as the mbuf RSS hash field if the data
> +  * buffer is a mbuf.
> +  */
> + if (unlikely(qe->error)) {
> + DLB2_LOG_ERR("QE error bit ON\n");
> + DLB2_INC_STAT(ev_port->stats.traffic.rx_drop, 1);
> + dlb2_consume_qe_immediate(qm_port, 1);
> + continue; /* Ignore */
> + }
> +
> + events[num].u64 = qe->data;
> + events[num].flow_id = qe->flow_id;
> + events[num].priority = DLB2_TO_EV_PRIO((uint8_t)qe->priority);
> + events[num].event_type = qe->u.event_type.major;
> + events[num].sub_event_type = qe->u.event_type.sub;
> + events[num].sched_type = sched_type_map[qe->sched_type];
> + events[num].impl_opaque = qe->qid_depth;
> +
> + /* qid not preserved for directed queues */
> + if (qm_port->is_directed)
> + evq_id = ev_port->link[0].queue_id;
> + else
> + evq_id = qid_mappings[qe->qid];
> +
> + events[num].queue_id = evq_id;
> + DLB2_INC_STAT(
> + ev_port->stats.queue[evq_id].qid_depth[qe-
> >qid_depth],
> + 1);
> + DLB2_INC_STAT(ev_port->stats.rx_sched_cnt[qe->sched_type],
> 1);
> + DLB2_INC_STAT(ev_port->stats.traffic.rx_ok, 1);

Move this outside the loop and increment by 'num' rather than '1'?

> + num++;
> + }
> +
> + return num;
> +}
> +
> +static inline int
> +dlb2_process_dequeue_four_qes(struct dlb2_eventdev_port *ev_port,
> +   struct dlb2_port *qm_port,
> +   struct rte_event *events,
> +   struct dlb2_dequeue_qe *qes)
> +{
> + int sched_type_map[] = {
> + [DLB2_SCHED_ATOMIC] = RTE_SCHED_TYPE_ATOMIC,
> + [DLB2_SCHED_UNORDERED] = RTE_SCHED_TYPE_PARALLEL,
> + [DLB2_SCHED_ORDERED] = RTE_SCHED_TYPE_ORDERED,
> + [DLB2_SCHED_DIRECTED] = RTE_SCHED_TYPE_ATOMIC,
> + };
> + const int num_events = DLB2_NUM_QES_PER_CACHE_LINE;
> + uint8_t *qid_mappings = qm_port->qid_mappings;
> + __m128i sse_evt[2];
> +
> + RTE_SET_USED(ev_port);  /* avoids unused variable error, if stats off */

ev_port gets passed to dlb2_process_dequeue_qes, I don't think this line is 
necessary.

> +
> + /* In the unlikely case that any of the QE error bits are set, process
> +  * them one at a time.
> +  */
> + if (unlikely(qes[0].error || qes[1].error ||
> +  qes[2].error || qes[3].error))
> + return dlb2_process_dequeue_qes(ev_port, qm_port, events,
> +  qes, num_events);

Thanks,
Gage

Re: [dpdk-dev] [PATCH 15/22] event/dlb2: add enqueue and its burst variants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 15/22] event/dlb2: add enqueue and its burst variants
> 
> Add support for enqueue and its variants.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 14/22] event/dlb2: add eventdev start

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 14/22] event/dlb2: add eventdev start
> 
> Add support for the eventdev start entry point.
> 

Code looks fine (one whitespace nit below), but it could use a more detailed
commit message -- e.g. describing how the PMD delays port links until
eventdev start time, how the configuration will be reapplied at this time if
this is a reconfiguration scenario, etc.

[...]

> +static int
> +dlb2_eventdev_start(struct rte_eventdev *dev)
> +{
> + struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> + struct dlb2_start_domain_args cfg;
> + int ret, i;
> +
> + rte_spinlock_lock(&dlb2->qm_instance.resource_lock);
> + if (dlb2->run_state != DLB2_RUN_STATE_STOPPED) {
> + DLB2_LOG_ERR("bad state %d for dev_start\n",
> +  (int)dlb2->run_state);
> + rte_spinlock_unlock(&dlb2->qm_instance.resource_lock);
> + return -EINVAL;
> + }
> + dlb2->run_state = DLB2_RUN_STATE_STARTING;

Looks like there's a tab between run_state and the equals sign.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 13/22] event/dlb2: add port unlink and port unlinks in progress

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 13/22] event/dlb2: add port unlink and port unlinks in 
> progress
> 
> Add supports for the port unlink(s) eventdev entry points.
> The unlink operation is an asynchronous operation executed by
> a control thread, and the unlinks-in-progress function reads
> a counter shared with the control thread.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 12/22] event/dlb2: add port link

[Eads, Gage]

> +static int
> +dlb2_event_queue_join_ldb(struct dlb2_eventdev *dlb2,
> +   struct dlb2_eventdev_port *ev_port,
> +   struct dlb2_eventdev_queue *ev_queue,
> +   uint8_t priority)
> +{
> + int first_avail = -1;
> + int ret, i;
> +
> + for (i = 0; i < DLB2_MAX_NUM_QIDS_PER_LDB_CQ; i++) {
> + if (ev_port->link[i].valid) {
> + if (ev_port->link[i].queue_id == ev_queue->id &&
> + ev_port->link[i].priority == priority) {
> + if (ev_port->link[i].mapped)
> + return 0; /* already mapped */
> + first_avail = i;
> + }
> + } else if (first_avail == -1) {
> + first_avail = i;
> + }

Nit: braces discouraged on a single-statement conditionals

> + }
> + if (first_avail == -1) {
> + DLB2_LOG_ERR("dlb2: qm_port %d has no available QID slots.\n",
> +  ev_port->qm_port.id);
> + return -EINVAL;
> + }
> +
> + ret = dlb2_hw_map_ldb_qid_to_port(&dlb2->qm_instance,
> +   ev_port->qm_port.id,
> +   ev_queue->qm_queue.id,
> +   priority);
> +
> + if (!ret)
> + ev_port->link[first_avail].mapped = true;
> +
> + return ret;
> +}
> +
> +static int32_t
> +dlb2_hw_create_dir_queue(struct dlb2_eventdev *dlb2,
> +  struct dlb2_eventdev_queue *ev_queue,
> +  int32_t qm_port_id)
> +{
> + struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> + struct dlb2_create_dir_queue_args cfg;
> + int32_t ret;
> +
> + /* The directed port is always configured before its queue */
> + cfg.port_id = qm_port_id;
> +
> + if (ev_queue->depth_threshold == 0) {
> + cfg.depth_threshold =
> RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH;
> + ev_queue->depth_threshold =
> RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH;
> + } else {
> + cfg.depth_threshold = ev_queue->depth_threshold;
> + }

Nit: braces discouraged on a single-statement conditionals

> +
> + ret = dlb2_iface_dir_queue_create(handle, &cfg);
> + if (ret < 0) {
> + DLB2_LOG_ERR("dlb2: create DIR event queue error, ret=%d
> (driver status: %s)\n",
> +  ret, dlb2_error_strings[cfg.response.status]);
> + return -EINVAL;
> + }
> +
> + return cfg.response.id;
> +}
> +
> +static int
> +dlb2_eventdev_dir_queue_setup(struct dlb2_eventdev *dlb2,
> +   struct dlb2_eventdev_queue *ev_queue,
> +   struct dlb2_eventdev_port *ev_port)
> +{
> + int32_t qm_qid;
> +
> + qm_qid = dlb2_hw_create_dir_queue(dlb2, ev_queue, ev_port-
> >qm_port.id);
> +
> + if (qm_qid < 0) {
> + DLB2_LOG_ERR("Failed to create the DIR queue\n");
> + return qm_qid;
> + }
> +
> + dlb2->qm_dir_to_ev_queue_id[qm_qid] = ev_queue->id;
> +
> + ev_queue->qm_queue.id = qm_qid;
> +
> + return 0;
> +}
> +
> +static int
> +dlb2_do_port_link(struct rte_eventdev *dev,
> +   struct dlb2_eventdev_queue *ev_queue,
> +   struct dlb2_eventdev_port *ev_port,
> +   uint8_t prio)
> +{
> + struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + int err;
> +
> + /* Don't link until start time. */
> + if (dlb2->run_state == DLB2_RUN_STATE_STOPPED)
> + return 0;
> +
> + if (ev_queue->qm_queue.is_directed)
> + err = dlb2_eventdev_dir_queue_setup(dlb2, ev_queue,
> ev_port);
> + else
> + err = dlb2_event_queue_join_ldb(dlb2, ev_port, ev_queue,
> prio);
> +
> + if (err) {
> + DLB2_LOG_ERR("port link failure for %s ev_q %d, ev_port %d\n",
> +  ev_queue->qm_queue.is_directed ? "DIR" : "LDB",
> +  ev_queue->id, ev_port->id);
> +
> + rte_errno = err;
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +dlb2_validate_port_link(struct dlb2_eventdev_port *ev_port,
> + uint8_t queue_id,
> + bool link_exists,
> + int index)
> +{
> + struct dlb2_eventdev *dlb2 = ev_port->dlb2;
> + struct dlb2_eventdev_queue *ev_queue;
> + bool port_is_dir, queue_is_dir;
> +
> + if (queue_id > dlb2->num_queues) {
> + rte_errno = -EINVAL;
> + return -1;
> + }
> +
> + ev_queue = &dlb2->ev_queues[queue_id];
> +
> + if (!ev_queue->setup_done &&
> + ev_queue->qm_queue.config_state != DLB2_PREV_CONFIGURED) {
> + rte_errno = -EINVAL;
> + return -1;
> + }
> +
> + port_is_dir = ev_port->qm_port.is_directed;
> + queue_is_dir = ev_queue->qm_queue.is_di

Re: [dpdk-dev] [PATCH 11/22] event/dlb2: add port setup

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 11/22] event/dlb2: add port setup
> 
> Configure the load balanded (ldb) or directed (dir) port.
> The consumer queue (CQ) and producer port (PP) are also
> set up here.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2.c  | 527 +
>  drivers/event/dlb2/dlb2_iface.c|   9 +
>  drivers/event/dlb2/dlb2_iface.h|   8 +
>  drivers/event/dlb2/pf/base/dlb2_resource.c | 921
> +
>  drivers/event/dlb2/pf/dlb2_main.c  |  28 +
>  drivers/event/dlb2/pf/dlb2_pf.c| 179 ++
>  6 files changed, 1672 insertions(+)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 366e194..a4c8833 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -1043,6 +1043,532 @@ dlb2_eventdev_queue_setup(struct rte_eventdev
> *dev,
>   return ret;
>  }
> 
> +static int
> +dlb2_init_consume_qe(struct dlb2_port *qm_port, char *mz_name)
> +{
> + struct dlb2_cq_pop_qe *qe;
> +
> + qe = rte_malloc(mz_name,
> + DLB2_NUM_QES_PER_CACHE_LINE *
> + sizeof(struct dlb2_cq_pop_qe),
> + RTE_CACHE_LINE_SIZE);
> +
> + if (qe == NULL) {
> + DLB2_LOG_ERR("dlb2: no memory for consume_qe\n");
> + return -ENOMEM;
> + }
> + qm_port->consume_qe = qe;
> +
> + memset(qe, 0, DLB2_NUM_QES_PER_CACHE_LINE *
> +sizeof(struct dlb2_cq_pop_qe));

You can use rte_zmalloc() instead (applies to other init_*_qe functions below 
too), and
no need to zero-init the fields again after the memset.

> +
> + qe->qe_valid = 0;
> + qe->qe_frag = 0;
> + qe->qe_comp = 0;
> + qe->cq_token = 1;
> + /* Tokens value is 0-based; i.e. '0' returns 1 token, '1' returns 2,
> +  * and so on.
> +  */
> + qe->tokens = 0; /* set at run time */
> + qe->meas_lat = 0;
> + qe->no_dec = 0;
> + /* Completion IDs are disabled */
> + qe->cmp_id = 0;
> +
> + return 0;
> +}
> +
> +static int
> +dlb2_init_int_arm_qe(struct dlb2_port *qm_port, char *mz_name)
> +{
> + struct dlb2_enqueue_qe *qe;
> +
> + qe = rte_malloc(mz_name,
> + DLB2_NUM_QES_PER_CACHE_LINE *
> + sizeof(struct dlb2_enqueue_qe),
> + RTE_CACHE_LINE_SIZE);
> +
> + if (qe == NULL) {
> + DLB2_LOG_ERR("dlb2: no memory for complete_qe\n");
> + return -ENOMEM;
> + }
> + qm_port->int_arm_qe = qe;
> +
> + memset(qe, 0, DLB2_NUM_QES_PER_CACHE_LINE *
> +sizeof(struct dlb2_enqueue_qe));
> +
> + /* V2 - INT ARM is CQ_TOKEN + FRAG */
> + qe->qe_valid = 0;
> + qe->qe_frag = 1;
> + qe->qe_comp = 0;
> + qe->cq_token = 1;
> + qe->meas_lat = 0;
> + qe->no_dec = 0;
> + /* Completion IDs are disabled */
> + qe->cmp_id = 0;
> +
> + return 0;
> +}
> +
> +static int
> +dlb2_init_qe_mem(struct dlb2_port *qm_port, char *mz_name)
> +{
> + int ret, sz;
> +
> + sz = DLB2_NUM_QES_PER_CACHE_LINE * sizeof(struct
> dlb2_enqueue_qe);
> +
> + qm_port->qe4 = rte_malloc(mz_name, sz, RTE_CACHE_LINE_SIZE);
> +
> + if (qm_port->qe4 == NULL) {
> + DLB2_LOG_ERR("dlb2: no qe4 memory\n");
> + ret = -ENOMEM;
> + goto error_exit;
> + }
> +
> + memset(qm_port->qe4, 0, sz);
> +
> + ret = dlb2_init_int_arm_qe(qm_port, mz_name);
> + if (ret < 0) {
> + DLB2_LOG_ERR("dlb2: dlb2_init_int_arm_qe ret=%d\n",
> +  ret);

This can fit on one line

> + goto error_exit;
> + }
> +
> + ret = dlb2_init_consume_qe(qm_port, mz_name);
> + if (ret < 0) {
> + DLB2_LOG_ERR("dlb2: dlb2_init_consume_qe ret=%d\n",
> +  ret);

This can fit on one line

> + goto error_exit;
> + }
> +
> + return 0;
> +
> +error_exit:
> +
> + dlb2_free_qe_mem(qm_port);
> +
> + return ret;
> +}
> +
> +static int
> +dlb2_hw_create_ldb_port(struct dlb2_eventdev *dlb2,
> + struct dlb2_eventdev_port *ev_port,
> + uint32_t 

Re: [dpdk-dev] [PATCH 10/22] event/dlb2: add queue setup

[Eads, Gage]

> +static int
> +dlb2_eventdev_ldb_queue_setup(struct rte_eventdev *dev,
> +   struct dlb2_eventdev_queue *ev_queue,
> +   const struct rte_event_queue_conf *queue_conf)
> +{
> + struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + int32_t qm_qid;
> +
> + if (queue_conf->nb_atomic_order_sequences)
> + dlb2_program_sn_allocation(dlb2, queue_conf);
> +
> + qm_qid = dlb2_hw_create_ldb_queue(dlb2,
> +   ev_queue,
> +   queue_conf);

Nit: this can fit on one line

Thanks,
Gage

Re: [dpdk-dev] [PATCH 08/22] event/dlb2: add infos get and configure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 08/22] event/dlb2: add infos get and configure
> 
> Add support for configuring the DLB hardware.

This is a large patch, even if most of the code is in the 
low-level/hardware-specific
base directory, and could use more explanation in the commit message.

> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2.c  |  334 +++
>  drivers/event/dlb2/dlb2_iface.c|7 +-
>  drivers/event/dlb2/dlb2_iface.h|5 +
>  drivers/event/dlb2/pf/base/dlb2_resource.c | 3234
> 
>  drivers/event/dlb2/pf/dlb2_main.c  |   14 +
>  drivers/event/dlb2/pf/dlb2_pf.c|   44 +
>  6 files changed, 3637 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 0d6fea4..58e953b 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -92,6 +92,28 @@ dlb2_get_queue_depth(struct dlb2_eventdev *dlb2,
>   return 0;
>  }
> 
> +static void
> +dlb2_free_qe_mem(struct dlb2_port *qm_port)
> +{
> + if (qm_port == NULL)
> + return;
> +
> + if (qm_port->qe4) {
> + rte_free(qm_port->qe4);
> + qm_port->qe4 = NULL;
> + }
> +
> + if (qm_port->int_arm_qe) {
> + rte_free(qm_port->int_arm_qe);
> + qm_port->int_arm_qe = NULL;
> + }
> +
> + if (qm_port->consume_qe) {
> + rte_free(qm_port->consume_qe);
> + qm_port->consume_qe = NULL;
> + }

You can pass a NULL pointer to rte_free() -- it's guaranteed to do nothing in 
that case.

> +}
> +
>  /* override defaults with value(s) provided on command line */
>  static void
>  dlb2_init_queue_depth_thresholds(struct dlb2_eventdev *dlb2,
> @@ -366,10 +388,322 @@ set_qid_depth_thresh(const char *key __rte_unused,
>  }
> 
>  static void
> +dlb2_eventdev_info_get(struct rte_eventdev *dev,
> +struct rte_event_dev_info *dev_info)
> +{
> + struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + int ret;
> +
> + ret = dlb2_hw_query_resources(dlb2);
> + if (ret) {
> + const struct rte_eventdev_data *data = dev->data;
> +
> + DLB2_LOG_ERR("get resources err=%d, devid=%d\n",
> +  ret, data->dev_id);
> + /* fn is void, so fall through and return values set up in
> +  * probe
> +  */
> + }
> +
> + /* Add num resources currently owned by this domain.
> +  * These would become available if the scheduling domain were reset
> due
> +  * to the application recalling eventdev_configure to *reconfigure* the
> +  * domain.
> +  */
> + evdev_dlb2_default_info.max_event_ports += dlb2->num_ldb_ports;
> + evdev_dlb2_default_info.max_event_queues += dlb2-
> >num_ldb_queues;
> + evdev_dlb2_default_info.max_num_events += dlb2->max_ldb_credits;
> +
> + evdev_dlb2_default_info.max_event_queues =
> + RTE_MIN(evdev_dlb2_default_info.max_event_queues,
> + RTE_EVENT_MAX_QUEUES_PER_DEV);
> +
> + evdev_dlb2_default_info.max_num_events =
> + RTE_MIN(evdev_dlb2_default_info.max_num_events,
> + dlb2->max_num_events_override);
> +
> + *dev_info = evdev_dlb2_default_info;
> +}
> +
> +static int
> +dlb2_hw_create_sched_domain(struct dlb2_hw_dev *handle,
> + const struct dlb2_hw_rsrcs *resources_asked)
> +{
> + int ret = 0;
> + struct dlb2_create_sched_domain_args *config_params;
> +
> + if (resources_asked == NULL) {
> + DLB2_LOG_ERR("dlb2: dlb2_create NULL parameter\n");
> + ret = EINVAL;
> + goto error_exit;
> + }
> +
> + /* Map generic qm resources to dlb2 resources */
> + config_params = &handle->cfg.resources;
> +
> + /* DIR ports and queues */
> +
> + config_params->num_dir_ports =
> + resources_asked->num_dir_ports;
> +
> + config_params->num_dir_credits =
> + resources_asked->num_dir_credits;
> +
> + /* LDB queues */
> +
> + config_params->num_ldb_queues =
> + resources_asked->num_ldb_queues;
> +
> + /* LDB ports */
> +
> + config_params->cos_strict = 0; /* Best effort */
> +

Re: [dpdk-dev] [PATCH 09/22] event/dlb2: add queue and port default conf

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 09/22] event/dlb2: add queue and port default conf
> 
> Add support for getting the queue and port default configuration.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2.c | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 58e953b..9ff371a 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -698,12 +698,45 @@ dlb2_eventdev_configure(const struct rte_eventdev
> *dev)
>  }
> 
>  static void
> +dlb2_eventdev_port_default_conf_get(struct rte_eventdev *dev,
> + uint8_t port_id,
> + struct rte_event_port_conf *port_conf)
> +{
> + RTE_SET_USED(port_id);
> + struct dlb2_eventdev *dlb2;
> +
> + dlb2 = dlb2_pmd_priv(dev);
> +
> + /* FIXME: Arbitrary values */

Leftover FIXME. Hopefully not too arbitrary :p.

> + port_conf->new_event_threshold = dlb2->new_event_limit;
> + port_conf->dequeue_depth = 32;
> + port_conf->enqueue_depth = DLB2_MAX_ENQUEUE_DEPTH;
> + port_conf->event_port_cfg = 0;
> +}
> +
> +static void
> +dlb2_eventdev_queue_default_conf_get(struct rte_eventdev *dev,
> +  uint8_t queue_id,
> +  struct rte_event_queue_conf *queue_conf)
> +{
> + RTE_SET_USED(dev);
> + RTE_SET_USED(queue_id);
> + /* FIXME: Arbitrary values */

Ditto

> + queue_conf->nb_atomic_flows = 1024;
> + queue_conf->nb_atomic_order_sequences = 64;
> + queue_conf->event_queue_cfg = 0;
> + queue_conf->priority = 0;
> +}

Thanks,
Gage

Re: [dpdk-dev] [PATCH 07/22] event/dlb2: add xstats

[Eads, Gage]

> diff --git a/drivers/event/dlb2/dlb2_xstats.c 
> b/drivers/event/dlb2/dlb2_xstats.c
> new file mode 100644
> index 000..9a69d78
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2_xstats.c
> @@ -0,0 +1,1269 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

+inttypes.h for the printf format specifiers (PRIu64, etc.)

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Besides rte_malloc.h and rte_eventdev.h, I suspect the rest are unnecessary.

[...]

> +int
> +dlb2_eventdev_xstats_get_names(const struct rte_eventdev *dev,
> + enum rte_event_dev_xstats_mode mode, uint8_t
> queue_port_id,
> + struct rte_event_dev_xstats_name *xstats_names,
> + unsigned int *ids, unsigned int size)
> +{
> + const struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + unsigned int i;
> + unsigned int xidx = 0;
> +
> + RTE_SET_USED(mode);
> + RTE_SET_USED(queue_port_id);

Not needed, these variables are used below.

> +
> + uint32_t xstats_mode_count = 0;
> + uint32_t start_offset = 0;
> +
> + switch (mode) {
> + case RTE_EVENT_DEV_XSTATS_DEVICE:
> + xstats_mode_count = dlb2->xstats_count_mode_dev;
> + break;
> + case RTE_EVENT_DEV_XSTATS_PORT:
> + if (queue_port_id >= DLB2_MAX_NUM_PORTS)
> + break;
> + xstats_mode_count = dlb2-
> >xstats_count_per_port[queue_port_id];
> + start_offset = dlb2->xstats_offset_for_port[queue_port_id];
> + break;
> + case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB2_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

Looks like this macro is tied to the h/w definition, is it subject to grow?

> + if (queue_port_id >= DLB2_MAX_NUM_QUEUES)
> + break;
> +#endif
> + xstats_mode_count = dlb2-
> >xstats_count_per_qid[queue_port_id];
> + start_offset = dlb2->xstats_offset_for_qid[queue_port_id];
> + break;
> + default:
> + return -EINVAL;
> + };
> +
> + if (xstats_mode_count > size || !ids || !xstats_names)
> + return xstats_mode_count;
> +
> + for (i = 0; i < dlb2->xstats_count && xidx < size; i++) {
> + if (dlb2->xstats[i].mode != mode)
> + continue;
> +
> + if (mode != RTE_EVENT_DEV_XSTATS_DEVICE &&
> + queue_port_id != dlb2->xstats[i].obj_idx)
> + continue;
> +
> + xstats_names[xidx] = dlb2->xstats[i].name;
> + if (ids)
> + ids[xidx] = start_offset + xidx;
> + xidx++;
> + }
> + return xidx;
> +}
> +
> +static int
> +dlb2_xstats_update(struct dlb2_eventdev *dlb2,
> + enum rte_event_dev_xstats_mode mode,
> + uint8_t queue_port_id, const unsigned int ids[],
> + uint64_t values[], unsigned int n, const uint32_t reset)
> +{
> + unsigned int i;
> + unsigned int xidx = 0;
> +
> + RTE_SET_USED(mode);
> + RTE_SET_USED(queue_port_id);
> +

Not needed, these variables are used below.

> + uint32_t xstats_mode_count = 0;
> +
> + switch (mode) {
> + case RTE_EVENT_DEV_XSTATS_DEVICE:
> + xstats_mode_count = dlb2->xstats_count_mode_dev;
> + break;
> + case RTE_EVENT_DEV_XSTATS_PORT:
> + if (queue_port_id >= DLB2_MAX_NUM_PORTS)
> + goto invalid_value;
> + xstats_mode_count = dlb2-
> >xstats_count_per_port[queue_port_id];
> + break;
> + case RTE_EVENT_DEV_XSTATS_QUEUE:
> +#if (DLB2_MAX_NUM_QUEUES <= 255) /* max 8 bit value */

(Same comment as above)

[...]

> +int
> +dlb2_eventdev_xstats_reset(struct rte_eventdev *dev,
> +enum rte_event_dev_xstats_mode mode,
> +int16_t queue_port_id,
> +const uint32_t ids[],
> +uint32_t nb_ids)
> +{
> + struct dlb2_eventdev *dlb2 = dlb2_pmd_priv(dev);
> + uint32_t i;
> +
> + /* handle -1 for queue_port_id here, looping over all ports/queues */
> + switch (mode) {
> + case RTE_EVENT_DEV_XSTATS_DEVICE:
> + if (dlb2_xstats_reset_dev(dlb2, ids, nb_ids))
> + return -EINVAL;
> + break;
> + case RTE_EVENT_DEV_XSTATS_PORT:
> + if (queue_port_id == -1) {
> + for (i = 0; i < DLB2_MAX_NUM_PORTS; i++) {
> + if (dlb2_xstats_reset_port(dlb2, i,
> + ids, nb_ids))

Nit: argument alignment (occurs below as well)

> + return -EINVAL;
> +  

Re: [dpdk-dev] [PATCH 06/22] event/dlb2: add probe

[Eads, Gage]

> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> new file mode 100644
> index 000..7ff7dac
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -0,0 +1,557 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Nit: flip the order of rte_kvargs.h and rte_io.h

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "dlb2_priv.h"
> +#include "dlb2_iface.h"
> +#include "dlb2_inline_fns.h"
> +
> +#if !defined RTE_ARCH_X86_64
> +#error "This implementation only supports RTE_ARCH_X86_64 architecture."
> +#endif

In patch #1, meson.build allows RTE_ARCH_X86 -- should that be removed?

> +
> +/*
> + * Resources exposed to eventdev. Some values overridden at runtime using
> + * values returned by the DLB kernel driver.
> + */
> +#if (RTE_EVENT_MAX_QUEUES_PER_DEV > UINT8_MAX)
> +#error "RTE_EVENT_MAX_QUEUES_PER_DEV cannot fit in member
> max_event_queues"
> +#endif
> +static struct rte_event_dev_info evdev_dlb2_default_info = {
> + .driver_name = "", /* probe will set */
> + .min_dequeue_timeout_ns = DLB2_MIN_DEQUEUE_TIMEOUT_NS,
> + .max_dequeue_timeout_ns = DLB2_MAX_DEQUEUE_TIMEOUT_NS,
> +#if (RTE_EVENT_MAX_QUEUES_PER_DEV < DLB2_MAX_NUM_LDB_QUEUES)
> + .max_event_queues = RTE_EVENT_MAX_QUEUES_PER_DEV,
> +#else
> + .max_event_queues = DLB2_MAX_NUM_LDB_QUEUES,
> +#endif
> + .max_event_queue_flows = DLB2_MAX_NUM_FLOWS,
> + .max_event_queue_priority_levels = DLB2_QID_PRIORITIES,
> + .max_event_priority_levels = DLB2_QID_PRIORITIES,
> + .max_event_ports = DLB2_MAX_NUM_LDB_PORTS,
> + .max_event_port_dequeue_depth = DLB2_MAX_CQ_DEPTH,
> + .max_event_port_enqueue_depth = DLB2_MAX_ENQUEUE_DEPTH,
> + .max_event_port_links = DLB2_MAX_NUM_QIDS_PER_LDB_CQ,
> + .max_num_events = DLB2_MAX_NUM_LDB_CREDITS,
> + .max_single_link_event_port_queue_pairs =
> DLB2_MAX_NUM_DIR_PORTS,
> + .event_dev_cap = (RTE_EVENT_DEV_CAP_QUEUE_QOS |
> +   RTE_EVENT_DEV_CAP_EVENT_QOS |
> +   RTE_EVENT_DEV_CAP_BURST_MODE |
> +   RTE_EVENT_DEV_CAP_DISTRIBUTED_SCHED |
> +   RTE_EVENT_DEV_CAP_IMPLICIT_RELEASE_DISABLE |
> +   RTE_EVENT_DEV_CAP_QUEUE_ALL_TYPES),
> +};
> +
> +/* These functions will vary based on processor capabilities */
> +static struct dlb2_port_low_level_io_functions qm_mmio_fns;
> +
> +struct process_local_port_data
> +dlb2_port[DLB2_MAX_NUM_PORTS][DLB2_NUM_PORT_TYPES];
> +
> +/* override defaults with value(s) provided on command line */
> +static void
> +dlb2_init_queue_depth_thresholds(struct dlb2_eventdev *dlb2,
> +  int *qid_depth_thresholds)
> +{
> + int q;
> +
> + for (q = 0; q < DLB2_MAX_NUM_QUEUES; q++) {
> + if (qid_depth_thresholds[q] != 0)
> + dlb2->ev_queues[q].depth_threshold =
> + qid_depth_thresholds[q];
> + }
> +}
> +
> +static int
> +dlb2_hw_query_resources(struct dlb2_eventdev *dlb2)
> +{
> + struct dlb2_hw_dev *handle = &dlb2->qm_instance;
> + struct dlb2_hw_resource_info *dlb2_info = &handle->info;
> + int ret;
> +
> + /* Query driver resources provisioned for this VF */

"VF" -> "device"?

> +
> + ret = dlb2_iface_get_num_resources(handle,
> +&dlb2->hw_rsrc_query_results);
> + if (ret) {
> + DLB2_LOG_ERR("ioctl get dlb2 num resources, err=%d\n",
> +  ret);

Nit: this could be a one-liner

> + return ret;
> + }
> +
> + /* Complete filling in device resource info returned to evdev app,
> +  * overriding any default values.
> +  * The capabilities (CAPs) were set at compile time.
> +  */
> +
> + evdev_dlb2_default_info.max_event_queues =
> + dlb2->hw_rsrc_query_results.num_ldb_queues;
> +
> + evdev_dlb2_default_info.max_event_ports =
> + dlb2->hw_rsrc_query_results.num_ldb_ports;
> +
> + evdev_dlb2_default_info.max_num_events =
> + dlb2->hw_rsrc_query_results.num_ldb_credits;
> +
> + /* Save off values used when creating the scheduling domain. */
> +
> + handle->info.num_sched_domains =
> + dlb2->hw_rsrc_query_results.num_sched_domains;
> +
> + handle->info.hw_rsrc_max.nb_events_limit =
> + dlb2->hw_rsrc_query_results.num_ldb_credits;
> +
> + handle->info.hw_rsrc_max.num_queues =
> + dlb2->hw_rsrc_query_results.num_ldb_queues +
> + dlb2->hw_rsrc_query_results.num_dir_ports;
> +
> + handle->info.hw_rsrc_max.num_ldb_queues =
> + dlb2->hw

Re: [dpdk-dev] [PATCH 03/22] event/dlb2: add private data structures and constants

[Eads, Gage]

> +/*!
> + * Configure the token pop mode for a DLB2 port. By default, all ports use
> + * AUTO_POP. This function must be called before calling
> rte_event_port_setup()
> + * for the port, but after calling rte_event_dev_configure().
> + *
> + * @param dev_id
> + *The identifier of the event device.
> + * @param port_id
> + *The identifier of the event port.
> + * @param mode
> + *The token pop mode.
> + *
> + * @return
> + * - 0: Success
> + * - EINVAL: Invalid dev_id, port_id, or mode
> + * - EINVAL: The DLB2 is not configured, is already running, or the port is
> + *   already setup
> + */
> +
> +int
> +rte_pmd_dlb2_set_token_pop_mode(uint8_t dev_id,
> + uint8_t port_id,
> + enum dlb2_token_pop_mode mode);

When I build the full series (plus dependencies), I get this error:

"
rte_pmd_dlb2_set_token_pop_mode is not flagged as experimental
but is listed in version map
Please add __rte_experimental to the definition of 
rte_pmd_dlb2_set_token_pop_mode
"
(https://doc.dpdk.org/guides/contributing/abi_policy.html#experimental-apis)

Thanks,
Gage

Re: [dpdk-dev] [PATCH 05/22] event/dlb2: add inline functions

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 05/22] event/dlb2: add inline functions
> 
> Add miscellaneous inline functions that may be called
> from multiple files.  These functions include inline
> assembly of new x86 instructions, such as movdir64b,
> since they are not available as builtin functions in
> the minimum supported GCC version.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2_inline_fns.h | 85
> 
>  1 file changed, 85 insertions(+)
>  create mode 100644 drivers/event/dlb2/dlb2_inline_fns.h
> 
> diff --git a/drivers/event/dlb2/dlb2_inline_fns.h
> b/drivers/event/dlb2/dlb2_inline_fns.h
> new file mode 100644
> index 000..f2f0935
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2_inline_fns.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#ifndef _DLB2_INLINE_FNS_H_
> +#define _DLB2_INLINE_FNS_H_
> +
> +/* Inline functions required in more than one source file.
> + */

Nit: this comment can be a one-liner.

> +
> +static inline struct dlb2_eventdev *
> +dlb2_pmd_priv(const struct rte_eventdev *eventdev)
> +{
> + return eventdev->data->dev_private;
> +}
> +
> +static inline void
> +dlb2_umonitor(volatile void *addr)
> +{
> + /* TODO: Change to proper assembly when compiler support available */

I would drop/reword the 'TODO', here and elsewhere in the file, so it doesn't 
give the
wrong impression that this code is incomplete.

> + asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf7\t\n"
> + :
> + : "D" (addr));
> +}

Thanks,
Gage

Re: [dpdk-dev] [PATCH 04/22] event/dlb2: add definitions shared with LKM or shared code

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 04/22] event/dlb2: add definitions shared with LKM or shared
> code
> 
> Add headers containing structs and constants shared between
> the PMD and the shared code.  The term shared code refers to
> the code that implements the hardware interface. The shared code
> is introduced in the probe patch, and then is extended as
> additional eventdev PMD entry points are added to the patchset.
> In the case of the bifurcated PMD (to be introduced in the
> future), the shared code is contained in the Linux kernel
> module itself.
> 
> Signed-off-by: Timothy McDaniel 

The checkpatch warnings are codespell false positives; looks good to me.

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 03/22] event/dlb2: add private data structures and constants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 03/22] event/dlb2: add private data structures and constants
> 
> The header file dlb2_priv.h is used internally by the PMD.
> It include constants, macros for device resources,
> structure definitions for hardware interfaces and
> software state, and various forward-declarations.
> The header file rte_pmd_dlb2.h will be exported in a
> subsequent patch, but is included here due to a data
> structure dependency.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  drivers/event/dlb2/dlb2_priv.h| 614
> ++
>  drivers/event/dlb2/rte_pmd_dlb2.h |  59 
>  2 files changed, 673 insertions(+)
>  create mode 100644 drivers/event/dlb2/dlb2_priv.h
>  create mode 100644 drivers/event/dlb2/rte_pmd_dlb2.h
> 
> diff --git a/drivers/event/dlb2/dlb2_priv.h b/drivers/event/dlb2/dlb2_priv.h
> new file mode 100644
> index 000..7bec835
> --- /dev/null
> +++ b/drivers/event/dlb2/dlb2_priv.h
> @@ -0,0 +1,614 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2016-2020 Intel Corporation
> + */
> +
> +#ifndef _DLB2_PRIV_H_
> +#define _DLB2_PRIV_H_
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "rte_config.h"

This should be included with angle brackets and immediately follow
rte_eventdev.h.

[...]

> +/* Do not change - must match hardware! */
> +enum dlb2_hw_sched_type {
> + DLB2_SCHED_ATOMIC = 0,
> + DLB2_SCHED_UNORDERED,
> + DLB2_SCHED_ORDERED,
> + DLB2_SCHED_DIRECTED,
> + /* DLB2_NUM_HW_SCHED_TYPES must be last */
> + DLB2_NUM_HW_SCHED_TYPES
> +};
> +
> +/* TODO - these structs  have been converted from qm - some fields may not
> be
> + * required
> + */

Leftover TODO -- still valid?

> +
> +struct dlb2_hw_rsrcs {
> + int32_t nb_events_limit;
> + uint32_t num_queues;/**> Total queues (lb + dir) */

I don't believe "**>" is correct doxygen format (angle bracket's facing the 
wrong
direction), but since this is an internal/private header file it won't be 
included in
the doxygen output anyway. I'd recommend either correcting the inconsistency
in angle brackets or just dropping the doxygen style here altogether.

> + uint32_t num_ldb_queues;/**> Number of available ldb queues */
> + uint32_t num_ldb_ports; /**< Number of load balanced ports */
> + uint32_t num_dir_ports; /**< Number of directed ports */
> + uint32_t num_ldb_credits;   /**< Number of load balanced credits */
> + uint32_t num_dir_credits;   /**< Number of directed credits */
> + uint32_t reorder_window_size;   /**< Size of reorder window */
> +};
> +
> +struct dlb2_hw_resource_info {
> + /**> Max resources that can be provided */
> + struct dlb2_hw_rsrcs hw_rsrc_max;
> + int num_sched_domains;
> + uint32_t socket_id;
> + /**> EAL flags passed to this QM instance, allowing the application to
> +  * identify the pmd backend indicating hardware or software.
> +  */
> + const char *eal_flags;

Doesn't look like eal_flags is used in any of the later patches.

[...]

> +
> +enum DLB2_PORT_STATE {
> + PORT_CLOSED,
> + PORT_STARTED,
> + PORT_STOPPED
> +};

Nit: other enum names in this file are lower-case.

> +
> +enum dlb2_configuration_state {
> + /* The resource has not been configured */
> + DLB2_NOT_CONFIGURED,
> + /* The resource was configured, but the device was stopped */
> + DLB2_PREV_CONFIGURED,
> + /* The resource is currently configured */
> + DLB2_CONFIGURED
> +};
> +
> +struct dlb2_port {
> + uint32_t id;
> + bool is_directed;
> + bool gen_bit;
> + uint16_t dir_credits;
> + uint32_t dequeue_depth;
> + enum dlb2_token_pop_mode token_pop_mode;
> + union dlb2_port_config cfg;
> + uint32_t *credit_pool[DLB2_NUM_QUEUE_TYPES]; /* use __atomic
> builtins */
> + uint16_t cached_ldb_credits;
> + uint16_t ldb_credits;
> + uint16_t cached_dir_credits;
> + bool int_armed;
> + uint16_t owed_tokens;
> + int16_t issued_releases;
> + int16_t token_pop_thresh;
> + int cq_depth;
> + uint16_t cq_idx;
> + uint16_t cq_idx_unmasked;
> + uint16_t cq_depth_mask;
> + uint16_t gen_bit_shift;
> + enum DLB2_PORT_STATE state;
> + enum dlb2_configuration_state config_state;
> + int num_mapped_qids;
> +  

Re: [dpdk-dev] [PATCH 02/22] event/dlb2: add dynamic logging

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 02/22] event/dlb2: add dynamic logging
> 
> This commit adds base support for dynamic logging.
> The default log level is NOTICE. Dynamic logging
> is used exclusively throughout this patchset.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 01/22] event/dlb2: add meson build infrastructure

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 3:26 PM
> To: Richardson, Bruce ; Ray Kinsella
> ; Neil Horman 
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH 01/22] event/dlb2: add meson build infrastructure
> 
> Adds the meson build infrastructure, which includes
> compile-time constants in rte_config.h. DLB2 is
> only supported on Linux X86 platforms at this time.
> 
> Signed-off-by: Timothy McDaniel 
> ---
>  config/rte_config.h   | 7 +++
>  drivers/event/dlb2/meson.build| 7 +++
>  drivers/event/dlb2/rte_pmd_dlb2_event_version.map | 3 +++
>  drivers/event/meson.build | 4 
>  4 files changed, 21 insertions(+)
>  create mode 100644 drivers/event/dlb2/meson.build
>  create mode 100644 drivers/event/dlb2/rte_pmd_dlb2_event_version.map
> 
> diff --git a/config/rte_config.h b/config/rte_config.h
> index 0bae630..fd1b3c3 100644
> --- a/config/rte_config.h
> +++ b/config/rte_config.h
> @@ -131,4 +131,11 @@
>  /* QEDE PMD defines */
>  #define RTE_LIBRTE_QEDE_FW ""
> 
> +/* DLB2 defines */
> +#define RTE_LIBRTE_PMD_DLB2_POLL_INTERVAL 1000
> +#define RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE  0
> +#undef RTE_LIBRTE_PMD_DLB2_QUELL_STATS
> +#define RTE_LIBRTE_PMD_DLB2_SW_CREDIT_QUANTA 32
> +#define RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH 256
> +
>  #endif /* _RTE_CONFIG_H_ */
> diff --git a/drivers/event/dlb2/meson.build b/drivers/event/dlb2/meson.build
> new file mode 100644
> index 000..d4fd39f
> --- /dev/null
> +++ b/drivers/event/dlb2/meson.build
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2019-2020 Intel Corporation
> +
> +sources = files(
> +)
> +
> +deps += ['mbuf', 'mempool', 'ring', 'bus_vdev', 'pci', 'bus_pci']

I don't believe the code in this series depends on bus_vdev, is that right?

With that resolved (if need be):
Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 1/5] lib/stack: fix inconsistent weak / strong cas

[Eads, Gage]

> -Original Message-
> From: David Marchand 
> Sent: Monday, September 28, 2020 5:23 AM
> To: Steven Lariau ; Eads, Gage 
> Cc: Olivier Matz ; dev ; nd
> 
> Subject: Re: [dpdk-dev] [PATCH v2 1/5] lib/stack: fix inconsistent weak / 
> strong
> cas
> 
> On Fri, Sep 25, 2020 at 7:44 PM Steven Lariau  wrote:
> >
> > Fix cmpexchange usage of weak / strong.
> > The generated code is the same on x86 and ARM (there is no weak
> > cmpexchange), but the old usage was inconsistent.
> > For push and pop update size, weak is used because cmpexchange is inside
> > a loop.
> > For pop update root, strong is used even though cmpexchange is inside a
> > loop, because there may be a lot of operations to do in a loop iteration
> > (locate the new head).
> 
> Is this patch backport material?

It's not a bugfix. It could help performance on a system with weak
cmpexchange -- e.g. the pop-update change would ensure no spurious
failures (which the code can handle, but would require another relatively
expensive pass through the pop loop.)

Thanks,
Gage

Re: [dpdk-dev] [PATCH v2 5/5] lib/stack: remove pop cas release ordering

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 25, 2020 12:44 PM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; Steven Lariau 
> Subject: [PATCH v2 5/5] lib/stack: remove pop cas release ordering
> 
> Replace the store-release by relaxed for the CAS success at the end of
> pop. Release isn't needed, because there is not write to data that need
> to be synchronized.
> The only preceding write is when the length is decreased, but the length
> CAS loop already ensures the right synchronization.
> The situation to avoid is when a thread sees the old length but the new
> list, that doesn't have enough items for pop to success.
> But the CAS success on length before the pop loop ensures any core reads
> and updates the latest length, preventing this situation.
> 
> The store-release is also used to make sure that the items are read
> before the head is updated, in order to prevent a core in pop to read an
> incorrect value because another core rewrites it with push.
> But this isn't needed, because items are read only when removed from the
> used list. Right after this, they are pushed to the free list, and the
> store-release in push makes sure the items are read before they are
> visible in the free list.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v3] eventdev: support telemetry with xstats info

[Eads, Gage]

> -Original Message-
> From: Chen, Mike Ximing 
> Sent: Friday, September 18, 2020 12:39 PM
> To: Jerin Jacob 
> Cc: dev@dpdk.org; Eads, Gage ; Vedantham, Sundar
> ; Power, Ciara ;
> Richardson, Bruce 
> Subject: [PATCH v3] eventdev: support telemetry with xstats info
> 
> The telemetry library is connected with eventdev xstats and
> port link info. The following new telemetry commands are added:
> 
> /eventdev/dev_list
> /eventdev/port_list,DevID
> /eventdev/queue_list,DevID
> /eventdev/dev_xstats,DevID
> /eventdev/port_xstats,DevID,PortID
> /eventdev/queue_xstats,DevID,PortID
> /eventdev/queue_links,DevID,PortID
> 
> queue_links command displays a list of queues linked with a specified
> eventdev port and a service priority associated with each link.
> 
> Signed-off-by: Mike Ximing Chen 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 3/5] lib/stack: remove redundant orderings for list->len

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; dharmik.thak...@arm.com; Steven Lariau
> 
> Subject: [PATCH 3/5] lib/stack: remove redundant orderings for list->len
> 
> The load-acquire of list->len on pop function is redundant.
> Only the CAS success needs to be load-acquire.
> It synchronizes with the store release in push, to ensure that the
> updated head is visible when the new length is visible.
> Without this, one thread in pop could see the increased length but the
> old list, which doesn't have enough items yet for pop to succeed.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 5/5] lib/stack: remove pop cas release ordering

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; dharmik.thak...@arm.com; Steven Lariau
> 
> Subject: [PATCH 5/5] lib/stack: remove pop cas release ordering
> 
> Replace the store-release by relaxed for the CAS success at the end of
> pop. Release isn't needed, because there is not write to data that need
> to be synchronized.
> The only preceding write is when the length is decreased, but the length
> CAS loop already ensures the right synchronization.
> The situation to avoid is when a thread sees the old length but the new
> list, that doesn't have enough items for pop to success.
> But the CAS success on length before the pop loop ensures any core reads
> and updates the latest length, preventing this situation.
> 
> The store-release is also used to make sure that the items are read
> before the head is updated, in order to prevent a core in pop to read an
> incorrect value because another core rewrites it with push.
> But this isn't needed, because items are read only when removed from the
> used list. Right after this, they are pushed to the free list, and the
> store-release in push makes sure the items are read before they are
> visible in the free list.

Please add a comment to this effect above the compare-exchange call. Depending
on this caller behavior for correctness is a little risky, but since this header
is private to the library I think it's ok as long as it's well-documented.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 2/5] lib/stack: remove push acquire fence

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; dharmik.thak...@arm.com; Steven Lariau
> 
> Subject: [PATCH 2/5] lib/stack: remove push acquire fence
> 
> An acquire fence is used to make sure loads after the fence can observe
> all store operations before a specific store-release.
> But push doesn't read any data, except for the head which is part of a
> CAS operation (the items on the list are not read).
> So there is no need for the acquire barrier.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 4/5] lib/stack: reload head when pop fails

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; dharmik.thak...@arm.com; Steven Lariau
> 
> Subject: [PATCH 4/5] lib/stack: reload head when pop fails
> 
> List head must be loaded right before continue (when failed to
> find the new head).
> Without this, one thread might keep trying and failing to pop items
> without ever loading the new correct head.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Ruifeng Wang 

Good catch. Please add the following so the fix is considered for dpdk-stable:
Fixes: 7e6e609939a8 ("stack: add C11 atomic implementation")
Cc: sta...@dpdk.org

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 1/5] lib/stack: fix inconsistent weak / strong cas

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage ; Olivier Matz 
> Cc: dev@dpdk.org; n...@arm.com; dharmik.thak...@arm.com; Steven Lariau
> 
> Subject: [PATCH 1/5] lib/stack: fix inconsistent weak / strong cas
> 
> Fix cmpexchange usage of weak / strong.
> The generated code is the same on x86 and ARM (there is no weak
> cmpexchange), but the old usage was inconsistent.
> For push and pop update size, weak is used because cmpexchange is inside
> a loop.
> For pop update root, strong is used even though cmpexchange is inside a
> loop, because there may be a lot of operations to do in a loop iteration
> (locate the new head).
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v3] doc: add stack mempool guide

[Eads, Gage]

Hi Olivier,



> > +Stack Mempool Driver
> > +
> > +
> > +**rte_mempool_stack** is a pure software mempool driver based on the
> > +``rte_stack`` DPDK library. A stack-based mempool is often better suited to
> > +packet-processing workloads than a ring-based mempool, since its LIFO
> behavior
> > +results in better temporal locality and a minimal memory footprint even if 
> > the
> > +mempool is over-provisioned.
> 
> Would it make sense to give an example of a use-case where the stack
> driver should be used in place of the standard ring-based one?
> 
> In most run-to-completion applications, the mbufs stay in per-core
> caches, so changing the mempool driver won't have a big impact. However,
> I suspect that for applications using a pipeline model (ex: rx on core0,
> tx on core1), the stack model would be more efficient. Is it something
> that you measured? If yes, it would be useful to explain this in the
> documentation.
> 

Good point, I was overlooking the impact of the per-core caches. I've seen data 
showing
better overall packet throughput with the stack mempool, and indeed that was a 
pipelined
application. How about this re-write?

"
**rte_mempool_stack** is a pure software mempool driver based on the
``rte_stack`` DPDK library. For run-to-completion workloads with sufficiently
large per-lcore caches, the mbufs will likely stay in the per-lcore caches and 
the
mempool type (ring, stack, etc.) will have a negligible impact on performance. 
However
a stack-based mempool is often better suited to pipelined packet-processing 
workloads
(which allocate and free mbufs on different lcores) than a ring-based mempool, 
since its
LIFO behavior results in better temporal locality and a minimal memory 
footprint even
if the mempool is over-provisioned. Users are encouraged to benchmark with 
multiple
mempool types to determine which works best for their specific application.
"

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 04/22] event/dlb: add definitions shared with LKM or shared code

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 04/22] event/dlb: add definitions shared with LKM or
> shared code
> 
> Add headers containing structs and constants shared between
> the PMD and the shared code.  The term shared code refers to
> the code that implements the hardware interface. The shared code
> is introduced in the probe patch, and then is extended as
> additional eventdev PMD entry points are added to the patchset.
> In the case of the bifurcated PMD (to be introduced in the
> future), the shared code is contained in the Linux kernel
> module itself.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Re: [dpdk-dev] [PATCH v4 03/22] event/dlb: add private data structures and constants

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 03/22] event/dlb: add private data structures and
> constants
> 
> Add headers used internally by the PMD. These headers are used
> internally by the PMD. They include constants, macros for device

Safe to drop either sentence #1 or #2 😊

[...]

Looked like some of these structs and fields are unused, so I did a few
test builds (of the whole series) to confirm. More details below.

> +
> +union dlb_port_config {
> + struct dlb_create_ldb_port_args ldb;
> + struct dlb_create_dir_port_args dir;
> +};
> +
> +enum DLB_PORT_STATE {

Nit: inconsistent capitalization (lower-case used in enum names elsewhere)

> + PORT_CLOSED,
> + PORT_STARTED,
> + PORT_STOPPED
> +};
> +
> +enum dlb_configuration_state {
> + /* The resource has not been configured */
> + DLB_NOT_CONFIGURED,
> + /* The resource was configured, but the device was stopped */
> + DLB_PREV_CONFIGURED,
> + /* The resource is currently configured */
> + DLB_CONFIGURED
> +};
> +
> +struct dlb_port {
> + uint32_t id;
> + bool is_directed;
> + bool gen_bit;
> + uint16_t dir_credits;
> + uint32_t dequeue_depth;
> + enum dlb_token_pop_mode token_pop_mode;
> + union dlb_port_config cfg;

This field is set but not used. Looks like it and the union can be dropped.

> + int pp_mmio_base;
> + uint16_t cached_ldb_credits;
> + uint16_t ldb_pushcount_at_credit_expiry;
> + uint16_t ldb_credits;
> + uint16_t cached_dir_credits;
> + uint16_t dir_pushcount_at_credit_expiry;
> + bool int_armed;
> + bool use_rsvd_token_scheme;
> + uint8_t cq_rsvd_token_deficit;
> + uint16_t owed_tokens;
> + int16_t issued_releases;
> + int16_t token_pop_thresh;
> + int cq_depth;
> + uint16_t cq_idx;
> + uint16_t cq_idx_unmasked;
> + uint16_t cq_depth_mask;
> + uint16_t gen_bit_shift;
> + enum DLB_PORT_STATE state;
> + enum dlb_configuration_state config_state;
> + int num_mapped_qids;
> + uint8_t *qid_mappings;
> + struct dlb_enqueue_qe *qe4; /* Cache line's worth of QEs (4) */
> + struct dlb_cq_pop_qe *consume_qe;
> + struct dlb_eventdev *dlb; /* back ptr */
> + struct dlb_eventdev_port *ev_port; /* back ptr */
> +};
> +
> +/* Per-process per-port mmio and memory pointers */
> +struct process_local_port_data {
> + uint64_t *pp_addr;
> + uint16_t *ldb_popcount;
> + uint16_t *dir_popcount;
> + struct dlb_dequeue_qe *cq_base;
> + bool mmaped;
> +};
> +
> +struct dlb_config {
> + int configured;
> + int reserved;
> + uint32_t ldb_credit_pool_id;
> + uint32_t dir_credit_pool_id;
> + uint32_t num_ldb_credits;
> + uint32_t num_dir_credits;
> + struct dlb_create_sched_domain_args resources;
> +};
> +
> +struct dlb_hw_dev {
> + char device_name[DLB_NAME_SIZE];
> + char device_path[DLB_MAX_DEVICE_PATH];

device_name and device_path are used in strings but never set

> + int device_path_id;

Set but never used

> + char domain_device_path[DLB_MAX_DEVICE_PATH];

Used in an xstats print but never set

> + struct dlb_config cfg;
> + struct dlb_hw_resource_info info;
> + void *pf_dev; /* opaque pointer to PF PMD dev (struct dlb_dev) */
> + int device_id;
> + uint32_t domain_id;
> + int domain_id_valid;
> + rte_spinlock_t resource_lock; /* for MP support */
> +}; __rte_cache_aligned
> +
> +/* End HW related defines and structs */
> +
> +/* Begin DLB PMD Eventdev related defines and structs */
> +
> +#define DLB_MAX_NUM_QUEUES \
> + (DLB_MAX_NUM_DIR_QUEUES + DLB_MAX_NUM_LDB_QUEUES)
> +
> +#define DLB_MAX_NUM_PORTS (DLB_MAX_NUM_DIR_PORTS +
> DLB_MAX_NUM_LDB_PORTS)
> +#define DLB_MAX_INPUT_QUEUE_DEPTH 256
> +
> +/* Used for parsing dir ports/queues. */

What does this comment refer to?

> +
> +/** Structure to hold the queue to port link establishment attributes */
> +
> +struct dlb_event_queue_link {
> + uint8_t queue_id;
> + uint8_t priority;
> + bool mapped;
> + bool valid;
> +};
> +
> +struct dlb_traffic_stats {
> + uint64_t rx_ok;
> + uint64_t rx_drop;
> + uint64_t rx_interrupt_wait;
> + uint64_t rx_umonitor_umwait;
> + uint64_t tx_ok;
> + uint64_t total_polls;
> + uint64_t zero_polls;
> + uint64_t tx_nospc_ldb_hw_credits;
> + uint64_t tx_nospc_dir_hw_credits;
> + ui

Re: [dpdk-dev] [PATCH v4 02/22] event/dlb: add dynamic logging

[Eads, Gage]

> -Original Message-
> From: McDaniel, Timothy 
> Sent: Friday, September 11, 2020 2:18 PM
> Cc: dev@dpdk.org; Carrillo, Erik G ; Eads, Gage
> ; Van Haaren, Harry ;
> jer...@marvell.com
> Subject: [PATCH v4 02/22] event/dlb: add dynamic logging
> 
> This commit adds base support for dynamic logging.
> The default log level is NOTICE. Dynamic logging
> is used exclusively throughout this patchset.
> 
> Signed-off-by: Timothy McDaniel 

Reviewed-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH v4 01/22] event/dlb: add documentation and meson infrastructure

[Eads, Gage]

> diff --git a/doc/guides/eventdevs/dlb.rst b/doc/guides/eventdevs/dlb.rst
> new file mode 100644
> index 000..e5759c0
> --- /dev/null
> +++ b/doc/guides/eventdevs/dlb.rst

Please add a dlb entry to doc/guides/eventdevs/index.rst

> @@ -0,0 +1,340 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +Copyright(c) 2020 Intel Corporation.
> +
> +Driver for the Intel® Dynamic Load Balancer (DLB)
> +==
> +
> +The DPDK dlb poll mode driver supports the Intel® Dynamic Load Balancer.
> +
> +Prerequisites
> +-
> +
> +- Follow the DPDK :ref:`Getting Started Guide for Linux ` to
> setup
> +  the basic DPDK environment.

It would be better to remove the bullet point ('-'), I think, since there's only
One sentence in the section.

> +
> +Configuration
> +-
> +
> +* The DLB PF PMD is a user-space PMD that uses VFIO to gain direct
> +  device access. To use this operation mode, the PCIe PF device must be
> bound
> +  to a DPDK-compatible VFIO driver, such as vfio-pci.

Same here ('*')

[...]

> +Atomic Inflights Allocation
> +~~~
> +
> +In the last stage prior to scheduling an atomic event to a CQ, DLB holds the
> +inflight event in a temporary buffer that is divided among load-balanced
> +queues. If a queue's atomic buffer storage fills up, this can result in
> +head-of-line-blocking. For example:

You need a space between these two lines for doxygen to render the bullet
points properly.

Thanks,
Gage

Re: [dpdk-dev] [PATCH] eventdev: add PCI probe named convenience function

[Eads, Gage]

>  /**
>   * @internal
> + * Wrapper for use by pci drivers as a .probe function to attach to a event
> + * interface.
> + */
> +static inline int
> +rte_event_pmd_pci_probe(struct rte_pci_driver *pci_drv,
> + struct rte_pci_device *pci_dev,
> + size_t private_data_size,
> + eventdev_pmd_pci_callback_t devinit)
> +{
> + char eventdev_name[RTE_EVENTDEV_NAME_MAX_LEN];
> +
> +

Two blank lines -- DPDK coding style doesn't forbid this as far as I know
(LINE_SPACING is ignored in checkpatch), but just an FYI in case this was
unintentional.

> + rte_pci_device_name(&pci_dev->addr, eventdev_name,
> + sizeof(eventdev_name));
> +
> + return rte_event_pmd_pci_probe_named(pci_drv,
> +  pci_dev,
> +  private_data_size,
> +  devinit,
> + (const char *)eventdev_name);

Nit: the cast is unnecessary, the conversion will happen implicitly without it.

With that,
Reviewed-by: Gage Eads 

Re: [dpdk-dev] [PATCH v2 1/4] test/stack: avoid trivial memory allocations

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Wednesday, August 12, 2020 2:19 PM
> To: Eads, Gage ; Olivier Matz
> 
> Cc: dev@dpdk.org; n...@arm.com; Steven Lariau 
> Subject: [PATCH v2 1/4] test/stack: avoid trivial memory allocations
> 
> Replace the arguments array by one argument.
> All objects in the args array have the same values, so there is no need
> to use an array, only one struct is enough.
> The args object is a lot smaller, and the allocation can be replaced
> with a global variable.
> 
> The allocation of obj_table isn't needed either, because MAX_BULK is
> small. The allocation can instead be replaced with a static array.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Phil Yang 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 1/4] test/stack: avoid trivial memory allocations

[Eads, Gage]

> -Original Message-
> From: Honnappa Nagarahalli 
> Sent: Tuesday, August 11, 2020 3:50 PM
> To: Stephen Hemminger ; Eads, Gage
> 
> Cc: Steven Lariau ; Olivier Matz
> ; dev@dpdk.org; Dharmik Thakkar
> ; nd ; Honnappa Nagarahalli
> ; nd 
> Subject: RE: [dpdk-dev] [PATCH 1/4] test/stack: avoid trivial memory
> allocations
> 
> 
> 
> > > >
> > > > Replace the arguments array by one argument.
> > > > All objects in the args array have the same values, so there is no
> > > > need to use an array, only one struct is enough.
> > > > The args object is a lot smaller, and the allocation can be replaced
> > > > with a stack variable.
> > > >
> > > > The allocation of obj_table isn't needed either, because MAX_BULK is
> > > > small. The allocation can instead be replaced with a static array.
> > > >
> > > > Signed-off-by: Steven Lariau 
> > > > Reviewed-by: Dharmik Thakkar 
> > > > Reviewed-by: Phil Yang 
> > > > Reviewed-by: Ruifeng Wang 
> > > > ---
> > > >  app/test/test_stack.c | 39 ++-
> > > >  1 file changed, 6 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/app/test/test_stack.c b/app/test/test_stack.c index
> > > > c8dac1f55..5a7273a7d 100644
> > > > --- a/app/test/test_stack.c
> > > > +++ b/app/test/test_stack.c
> > > > @@ -280,16 +280,9 @@ static int
> > > >  stack_thread_push_pop(void *args)
> > > >  {
> > > > struct test_args *t = args;
> > > > -   void **obj_table;
> > > > +   void *obj_table[MAX_BULK];
> > > > int i;
> > > >
> > > > -   obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0);
> > > > -   if (obj_table == NULL) {
> > > > -   printf("[%s():%u] failed to calloc %zu bytes\n",
> > > > -  __func__, __LINE__, STACK_SIZE * sizeof(void *));
> > > > -   return -1;
> > > > -   }
> > > > -
> > > > for (i = 0; i < NUM_ITERS_PER_THREAD; i++) {
> > > > unsigned int success, num;
> > > >
> > > > @@ -310,28 +303,25 @@ stack_thread_push_pop(void *args)
> > > > if (rte_stack_push(t->s, obj_table, num) != num) {
> > > > printf("[%s():%u] Failed to push %u pointers\n",
> > > >__func__, __LINE__, num);
> > > > -   rte_free(obj_table);
> > > > return -1;
> > > > }
> > > >
> > > > if (rte_stack_pop(t->s, obj_table, num) != num) {
> > > > printf("[%s():%u] Failed to pop %u pointers\n",
> > > >__func__, __LINE__, num);
> > > > -   rte_free(obj_table);
> > > > return -1;
> > > > }
> > > >
> > > > rte_atomic64_sub(t->sz, num);
> > > > }
> > > >
> > > > -   rte_free(obj_table);
> > > > return 0;
> > > >  }
> > >
> > > Agreed, the dynamic allocation is unnecessary.
> > >
> > > >
> > > >  static int
> > > >  test_stack_multithreaded(uint32_t flags)  {
> > > > -   struct test_args *args;
> > > > +   struct test_args args;
> > > > unsigned int lcore_id;
> > > > struct rte_stack *s;
> > > > rte_atomic64_t size;
> > > > @@ -344,45 +334,28 @@ test_stack_multithreaded(uint32_t flags)
> > > > printf("[%s():%u] Running with %u lcores\n",
> > > >__func__, __LINE__, rte_lcore_count());
> > > >
> > > > -   args = rte_malloc(NULL, sizeof(struct test_args) * 
> > > > RTE_MAX_LCORE,
> > > > 0);
> > > > -   if (args == NULL) {
> > > > -   printf("[%s():%u] failed to malloc %zu bytes\n",
> > > > -  __func__, __LINE__,
> > > > -  sizeof(struct test_args) * RTE_MAX_LCORE);
> > > > -   return -1;
> > > > -   }
> > > > -
> > > 

Re: [dpdk-dev] [PATCH 4/4] test/stack: remove atomics operations

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Wednesday, August 5, 2020 10:57 AM
> To: Eads, Gage ; Olivier Matz
> 
> Cc: dev@dpdk.org; honnappa.nagaraha...@arm.com;
> dharmik.thak...@arm.com; n...@arm.com; Steven Lariau
> 
> Subject: [PATCH 4/4] test/stack: remove atomics operations
> 
> Remove the part that checks if there is enough room in the stack, it's
> always true as long as size of stack >= MAX_BULK*rte_lcore_count().
> This check used an atomic cmpset, and read / write to a shared size
> variable. These operations result in some form of synchronization
> that might get in the way of the actual stack testing.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Phil Yang 
> Reviewed-by: Ruifeng Wang 

Nice simplification, and thanks for the good contributions to these tests.

Acked-by: Gage Eads 

Re: [dpdk-dev] [PATCH 3/4] test/stack: propagate errors to main core

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Wednesday, August 5, 2020 10:57 AM
> To: Eads, Gage ; Olivier Matz
> 
> Cc: dev@dpdk.org; honnappa.nagaraha...@arm.com;
> dharmik.thak...@arm.com; n...@arm.com; Steven Lariau
> 
> Subject: [PATCH 3/4] test/stack: propagate errors to main core
> 
> Use rte_eal_wait_lcore to wait and get the return value for all cores.
> This is used to propagate any error to the main core.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Phil Yang 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 2/4] test/stack: launch tests with mp remote launch API

[Eads, Gage]

> -Original Message-
> From: Steven Lariau 
> Sent: Wednesday, August 5, 2020 10:57 AM
> To: Eads, Gage ; Olivier Matz
> 
> Cc: dev@dpdk.org; honnappa.nagaraha...@arm.com;
> dharmik.thak...@arm.com; n...@arm.com; Steven Lariau
> 
> Subject: [PATCH 2/4] test/stack: launch tests with mp remote launch API
> 
> All the cores use the same argument object, so there is no need to use
> a loop to launch the test on every core one by one.
> Replace loop with one call to rte_eal_mp_remote_launch
> 
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Phil Yang 
> Reviewed-by: Ruifeng Wang 

Acked-by: Gage Eads 

Thanks,
Gage

Re: [dpdk-dev] [PATCH 1/4] test/stack: avoid trivial memory allocations

[Eads, Gage]

Hi Steven,

> -Original Message-
> From: Steven Lariau 
> Sent: Wednesday, August 5, 2020 10:57 AM
> To: Eads, Gage ; Olivier Matz
> 
> Cc: dev@dpdk.org; honnappa.nagaraha...@arm.com;
> dharmik.thak...@arm.com; n...@arm.com; Steven Lariau
> 
> Subject: [PATCH 1/4] test/stack: avoid trivial memory allocations
> 
> Replace the arguments array by one argument.
> All objects in the args array have the same values, so there is no need
> to use an array, only one struct is enough.
> The args object is a lot smaller, and the allocation can be replaced
> with a stack variable.
> 
> The allocation of obj_table isn't needed either, because MAX_BULK is
> small. The allocation can instead be replaced with a static array.
> 
> Signed-off-by: Steven Lariau 
> Reviewed-by: Dharmik Thakkar 
> Reviewed-by: Phil Yang 
> Reviewed-by: Ruifeng Wang 
> ---
>  app/test/test_stack.c | 39 ++-
>  1 file changed, 6 insertions(+), 33 deletions(-)
> 
> diff --git a/app/test/test_stack.c b/app/test/test_stack.c
> index c8dac1f55..5a7273a7d 100644
> --- a/app/test/test_stack.c
> +++ b/app/test/test_stack.c
> @@ -280,16 +280,9 @@ static int
>  stack_thread_push_pop(void *args)
>  {
>   struct test_args *t = args;
> - void **obj_table;
> + void *obj_table[MAX_BULK];
>   int i;
> 
> - obj_table = rte_calloc(NULL, STACK_SIZE, sizeof(void *), 0);
> - if (obj_table == NULL) {
> - printf("[%s():%u] failed to calloc %zu bytes\n",
> -__func__, __LINE__, STACK_SIZE * sizeof(void *));
> - return -1;
> - }
> -
>   for (i = 0; i < NUM_ITERS_PER_THREAD; i++) {
>   unsigned int success, num;
> 
> @@ -310,28 +303,25 @@ stack_thread_push_pop(void *args)
>   if (rte_stack_push(t->s, obj_table, num) != num) {
>   printf("[%s():%u] Failed to push %u pointers\n",
>  __func__, __LINE__, num);
> - rte_free(obj_table);
>   return -1;
>   }
> 
>   if (rte_stack_pop(t->s, obj_table, num) != num) {
>   printf("[%s():%u] Failed to pop %u pointers\n",
>  __func__, __LINE__, num);
> - rte_free(obj_table);
>   return -1;
>   }
> 
>   rte_atomic64_sub(t->sz, num);
>   }
> 
> - rte_free(obj_table);
>   return 0;
>  }

Agreed, the dynamic allocation is unnecessary.

> 
>  static int
>  test_stack_multithreaded(uint32_t flags)
>  {
> - struct test_args *args;
> + struct test_args args;
>   unsigned int lcore_id;
>   struct rte_stack *s;
>   rte_atomic64_t size;
> @@ -344,45 +334,28 @@ test_stack_multithreaded(uint32_t flags)
>   printf("[%s():%u] Running with %u lcores\n",
>  __func__, __LINE__, rte_lcore_count());
> 
> - args = rte_malloc(NULL, sizeof(struct test_args) * RTE_MAX_LCORE,
> 0);
> - if (args == NULL) {
> - printf("[%s():%u] failed to malloc %zu bytes\n",
> -__func__, __LINE__,
> -sizeof(struct test_args) * RTE_MAX_LCORE);
> - return -1;
> - }
> -
>   s = rte_stack_create("test", STACK_SIZE, rte_socket_id(), flags);
>   if (s == NULL) {
>   printf("[%s():%u] Failed to create a stack\n",
>  __func__, __LINE__);
> - rte_free(args);
>   return -1;
>   }
> 
>   rte_atomic64_init(&size);
> + args.s = s;
> + args.sz = &size;
> 
>   RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> - args[lcore_id].s = s;
> - args[lcore_id].sz = &size;
> -
>   if (rte_eal_remote_launch(stack_thread_push_pop,
> -   &args[lcore_id], lcore_id))
> +   &args, lcore_id))
>   rte_panic("Failed to launch lcore %d\n", lcore_id);
>   }


In general we shouldn't pass a stack variable to other threads. Though your
code here looks fine, I'd rather err on the safe side in case this is ever used
as a template/basis for some other code...particularly since there's no
performance/correctness/etc. penalty to using dynamically allocated memory.

To support patch 2/4, you can instead convert the rte_malloc to allocate a
single shared test_args structure. Or perhaps move patch 4 earlier in the 
series,
and simply pass the stack pointer instead.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 14/27] event/dlb: add PMD self-tests

[Eads, Gage]

Hi Tim,

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Minor nit: I did a test build, looks like rte_ethdev.h, rte_per_lcore.h, and 
rte_pause.h are not required, but rte_mbuf.h and rte_mempool.h are.

> +int
> +test_dlb_eventdev(void)
> +{
> + const char *dlb_eventdev_name = "event_dlb";

"event_dlb" -> "dlb_event"

This self-test also needs to be added to app/test/test_eventdev.c.

Thanks,
Gage

Re: [dpdk-dev] [PATCH 15/27] event/dlb: add probe

[Eads, Gage]

Hi Tim,



> +/* declared extern in header, for access from other .c files */ int
> +eventdev_dlb_log_level;
> +
> +RTE_INIT(evdev_dlb_init_log)
> +{
> + eventdev_dlb_log_level = rte_log_register("pmd.event.dlb");
> + if (eventdev_dlb_log_level >= 0)
> + rte_log_set_level(eventdev_dlb_log_level, RTE_LOG_NOTICE);
> }

There's a new macro RTE_LOG_REGISTER() that's designed to replace this 
boilerplate code. Examples here: 
http://mails.dpdk.org/archives/dev/2020-July/172273.html

Thanks,
Gage

Re: [dpdk-dev] [PATCH 05/27] event/dlb: add DLB documentation

[Eads, Gage]

Hi Tim,

>  doc/guides/eventdevs/dlb.rst |  497
> ++
>  1 file changed, 497 insertions(+)
>  create mode 100644 doc/guides/eventdevs/dlb.rst
> 
> diff --git a/doc/guides/eventdevs/dlb.rst b/doc/guides/eventdevs/dlb.rst new
> file mode 100644 index 000..21e48fe
> --- /dev/null
> +++ b/doc/guides/eventdevs/dlb.rst
> @@ -0,0 +1,497 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +Copyright(c) 2020 Intel Corporation.
> +
> +Driver for the Intel® Dynamic Load Balancer (DLB)
> +==
> +
> +The DPDK dlb poll mode driver supports the Intel® Dynamic Load Balancer.
> +
> +.. note::
> +
> +This PMD is disabled by default in the build configuration files, owing 
> to
> +an external dependency on the `Netlink Protocol Library Suite
> +`_ (libnl-3 and libnl-genl-3) which
> +must be installed on the board.  Once the Netlink libraries are 
> installed,
> +the PMD can be enabled by setting CONFIG_RTE_LIBRTE_PMD_DLB_QM=y
> and
> +recompiling the DPDK.
> +

This description appears to be out-of-date.

> +Prerequisites
> +-
> +
> +- Follow the DPDK :ref:`Getting Started Guide for Linux ` to
> +setup
> +  the basic DPDK environment.
> +
> +- Learn about the DLB device and its capabilities at `Intel Support
> +  `_. FIXME: Add real link when
> +documentation
> +  becomes available.

Leftover FIXME

> +
> +- The DLB kernel module. If it is not included in the machine's OS
> +  distribution, download it from  +available> and
> +  follow the build instructions.
> +

Leftover FIXME



> +The hybrid timeout data structures are currently located in
> +drivers/event/dlb/dlb_timeout.h:
> +
> +.. code-block:: c
> +
> +struct rte_hybrid_timeout_ticks_64 {
> +RTE_STD_C11
> +union {
> +uint64_t val64;
> +struct {
> +uint64_t poll_ticks:62;
> +uint64_t umonitor_wait:1;
> +uint64_t interrupt_wait:1;
> +};
> +};
> +};
> +struct rte_hybrid_timeout_ns_32 {
> +RTE_STD_C11
> +union {
> +uint32_t val32;
> +struct {
> +uint32_t poll_ns:30;
> +uint32_t umonitor_wait:1;
> +uint32_t interrupt_wait:1;
> +};
> +};
> +};

Is this description correct? dlb_timeout.h isn't introduced in this patchset.

> +
> +VAS Configuration
> +~
> +
> +A VAS is a scheduling domain, of which there are 32 in the DLB.
> +(Producer ports in one VAS cannot enqueue events to a different VAS,
> +except through the `Data Mover`_.) When a VAS is configured, it

I believe this cross-VAS comment is out-of-date.

> +allocates load-balanced and directed queues, ports, credits, and other
> +hardware resources. Some VAS resource allocations are user-controlled
> +-- the number of queues, for example
> +-- and others, like credit pools (one directed and one load-balanced
> +pool per VAS), are not.
> +
> +The dlb PMD creates a single VAS per DLB device. Supporting multiple
> +VASes per DLB device is a planned feature, where each VAS will be
> +represented as a separate event device.

Is this comment up-to-date? Patch 16 ("event/dlb: add infos_get and configure") 
indicates that multiple event devices are supported.



> +Hardware Credits
> +
> +
> +DLB uses a hardware credit scheme to prevent software from overflowing
> +hardware event storage, with each unit of storage represented by a
> +credit. A port spends a credit to enqueue an event, and hardware
> +refills the ports with credits as the events are scheduled to ports.
> +Refills come from credit pools, and each port is a member of a
> +load-balanced credit pool and a directed credit pool. The load-balanced
> +credits are used to enqueue to load-balanced queues, and directed credits
> are used for directed queues.
> +
> +An dlb eventdev contains one load-balanced and one directed credit

"An dlb" -> "A dlb"

> +pool. These pools' sizes are controlled by the nb_events_limit field in
> +struct rte_event_dev_config. The load-balanced pool is sized to contain
> +nb_events_limit credits, and the directed pool is sized to contain
> +nb_events_limit/4 credits. The directed pool size can be overriden with
> +the num_dir_credits vdev argument, like so:
> +
> +.. code-block:: console
> +
> +   --vdev=dlb1_event,num_dir_credits=
> +
> +This can be used if the default allocation is too low or too high for
> +the specific application needs. The PMD also supports a vdev arg that
> +limits the max_num_events reported by rte_event_dev_info_get():
> +
> +