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

["Eads, Gage" <gage.eads@intel.com>]

> -----Original Message-----
> From: McDaniel, Timothy <timothy.mcdaniel@intel.com>
> Sent: Friday, September 11, 2020 3:26 PM
> Cc: dev@dpdk.org; Carrillo, Erik G <erik.g.carrillo@intel.com>; Eads, Gage
> <gage.eads@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com>;
> jerinj@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 <timothy.mcdaniel@intel.com>
> ---
>  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 0000000..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 <emmintrin.h>
> +#include <stdbool.h>
> +
> +#include <rte_eventdev.h>
> +
> +#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;
> +	uint8_t *qid_mappings;
> +	struct dlb2_enqueue_qe *qe4; /* Cache line's worth of QEs (4) */
> +	struct dlb2_enqueue_qe *int_arm_qe;
> +	struct dlb2_cq_pop_qe *consume_qe;
> +	struct dlb2_eventdev *dlb2; /* back ptr */
> +	struct dlb2_eventdev_port *ev_port; /* back ptr */
> +};
> +
> +/* Per-process per-port mmio and memory pointers */
> +struct process_local_port_data {
> +	uint64_t *pp_addr;
> +	struct dlb2_dequeue_qe *cq_base;
> +	bool mmaped;
> +};
> +
> +struct dlb2_eventdev;
> +
> +struct dlb2_port_low_level_io_functions {
> +	void (*pp_enqueue_four)(void *qe4,
> +				void *pp_addr);

In the DLB PMD v4 patchset, I noticed the pp_enqueue_four indirection is dropped
in favor of directly calling the MOVDIR64 function -- can that be done here as well?

> +};
> +
> +/* TODO - Optimize memory use and layout */

Leftover TODO

> +struct dlb2_config {
> +	int configured;
> +	int reserved;
> +	uint32_t num_ldb_credits;
> +	uint32_t num_dir_credits;
> +	struct dlb2_create_sched_domain_args resources;
> +};
> +
> +enum dlb2_cos {
> +	DLB2_COS_DEFAULT = -1,
> +	DLB2_COS_0 = 0,
> +	DLB2_COS_1,
> +	DLB2_COS_2,
> +	DLB2_COS_3
> +};
> +
> +struct dlb2_hw_dev {
> +	char device_name[DLB2_NAME_SIZE];
> +	char device_path[DLB2_MAX_DEVICE_PATH];
> +	int device_path_id;
> +	struct dlb2_config cfg;
> +	struct dlb2_hw_resource_info info;
> +	void *pf_dev; /* opaque pointer to PF PMD dev (struct dlb2_dev) */
> +	int device_id;
> +	uint32_t domain_id;
> +	int domain_id_valid;
> +	enum dlb2_cos cos_id;
> +	rte_spinlock_t resource_lock; /* for MP support */
> +}; __rte_cache_aligned

Attribute needs to come before the semi-colon.

> +
> +/* End HW related defines and structs */
> +
> +/* Begin DLB2 PMD Eventdev related defines and structs */
> +
> +#define DLB2_MAX_NUM_QUEUES \
> +	(DLB2_MAX_NUM_DIR_QUEUES + DLB2_MAX_NUM_LDB_QUEUES)
> +
> +#define DLB2_MAX_NUM_PORTS (DLB2_MAX_NUM_DIR_PORTS +
> DLB2_MAX_NUM_LDB_PORTS)
> +#define DLB2_MAX_INPUT_QUEUE_DEPTH 256
> +
> +/* Used for parsing dir ports/queues. */

Does this comment refer to the following code? Seems out of place.

> +
> +/* Note: eventdev currently is limited to 64 ports, but DLB hardware has 128
> + * directed ports/queues.
> + */

Does this refer to RTE_EVENT_MAX_QUEUES_PER_DEV? If so, looks like the change
in the DLB PMD to increase that to 255 makes this comment unnecessary.

> +struct dlb2_dir_resource_list {
> +	int entries;
> +	uint32_t id[DLB2_MAX_NUM_DIR_PORTS];
> +};

Doesn't look this structure is used elsewhere in this series.

[...]

> +struct dlb2_eventdev_queue {
> +	struct dlb2_queue qm_queue;
> +	struct rte_event_queue_conf conf; /* User config */
> +	int depth_threshold; /* use default if 0 */
> +	uint32_t id;
> +	bool setup_done;
> +	uint8_t num_links;
> +};
> +
> +struct dlb2_device_stats {
> +	/* Device specific */
> +};

Any need for this, since it's empty?

> +
> +enum dlb2_run_state {
> +	DLB2_RUN_STATE_STOPPED = 0,
> +	DLB2_RUN_STATE_STOPPING,
> +	DLB2_RUN_STATE_STARTING,
> +	DLB2_RUN_STATE_STARTED
> +};
> +
> +struct dlb2_eventdev {
> +	struct dlb2_eventdev_port ev_ports[DLB2_MAX_NUM_PORTS];
> +	struct dlb2_eventdev_queue ev_queues[DLB2_MAX_NUM_QUEUES];
> +	uint8_t qm_ldb_to_ev_queue_id[DLB2_MAX_NUM_QUEUES];
> +	uint8_t qm_dir_to_ev_queue_id[DLB2_MAX_NUM_QUEUES];
> +	/* store num stats and offset of the stats for each queue */
> +	uint16_t xstats_count_per_qid[DLB2_MAX_NUM_QUEUES];
> +	uint16_t xstats_offset_for_qid[DLB2_MAX_NUM_QUEUES];
> +	/* store num stats and offset of the stats for each port */
> +	uint16_t xstats_count_per_port[DLB2_MAX_NUM_PORTS];
> +	uint16_t xstats_offset_for_port[DLB2_MAX_NUM_PORTS];
> +	struct dlb2_get_num_resources_args hw_rsrc_query_results;
> +	uint32_t xstats_count_mode_queue;
> +	struct dlb2_hw_dev qm_instance; /* strictly hw related */
> +	uint64_t global_dequeue_wait_ticks;
> +	struct dlb2_xstats_entry *xstats;
> +	struct rte_eventdev *event_dev; /* backlink to dev */
> +	uint32_t xstats_count_mode_dev;
> +	uint32_t xstats_count_mode_port;
> +	uint32_t xstats_count;
> +	uint32_t inflights; /* use __atomic builtins */
> +	uint32_t new_event_limit;
> +	int max_num_events_override;
> +	int num_dir_credits_override;
> +	volatile enum dlb2_run_state run_state;
> +	uint16_t num_dir_queues; /* total num of evdev dir queues requested
> */
> +	uint16_t num_dir_credits;
> +	uint16_t num_ldb_credits;
> +	uint16_t num_queues; /* total queues */
> +	uint16_t num_ldb_queues; /* total num of evdev ldb queues requested
> */
> +	uint16_t num_ports; /* total num of evdev ports requested */
> +	uint16_t num_ldb_ports; /* total num of ldb ports requested */
> +	uint16_t num_dir_ports; /* total num of dir ports requested */
> +	bool umwait_allowed;
> +	bool global_dequeue_wait; /* Not using per dequeue wait if true */
> +	bool defer_sched;
> +	enum dlb2_cq_poll_modes poll_mode;
> +	uint8_t revision;
> +	bool configured;
> +	uint16_t max_ldb_credits;
> +	uint16_t max_dir_credits;
> +
> +	/* force hw credit pool counters into exclusive cache lines */
> +
> +	/* use __atomic builtins */ /* shared hw cred */
> +	uint32_t ldb_credit_pool __rte_cache_aligned;
> +	/* use __atomic builtins */ /* shared hw cred */
> +	uint32_t dir_credit_pool __rte_cache_aligned;
> +
> +	/* Device stats */
> +	struct dlb2_device_stats stats __rte_cache_aligned;
> +};
> +
> +/* used for collecting and passing around the dev args */
> +struct dlb2_qid_depth_thresholds {
> +	int val[DLB2_MAX_NUM_QUEUES];
> +};

Nit: whitespace between the struct definitions, to match the rest of the file.

> +struct dlb2_devargs {
> +	int socket_id;
> +	int max_num_events;
> +	int num_dir_credits_override;
> +	int dev_id;
> +	int defer_sched;
> +	struct dlb2_qid_depth_thresholds qid_depth_thresholds;
> +	enum dlb2_cos cos_id;
> +};
> +
> +/* End Eventdev related defines and structs */
> +
> +/* Forwards for non-inlined functions */
> +
> +int dlb2_uninit(const char *name);

This function is never defined

> +
> +void dlb2_eventdev_dump(struct rte_eventdev *dev, FILE *f);
> +
> +int dlb2_xstats_init(struct dlb2_eventdev *dlb2);
> +
> +void dlb2_xstats_uninit(struct dlb2_eventdev *dlb2);
> +
> +int dlb2_eventdev_xstats_get(const struct rte_eventdev *dev,
> +		enum rte_event_dev_xstats_mode mode, uint8_t
> queue_port_id,
> +		const unsigned int ids[], uint64_t values[], unsigned int n);
> +
> +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 *xstat_names,
> +		unsigned int *ids, unsigned int size);
> +
> +uint64_t dlb2_eventdev_xstats_get_by_name(const struct rte_eventdev *dev,
> +					  const char *name, unsigned int *id);
> +
> +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);
> +
> +int test_dlb2_eventdev(void);
> +
> +int dlb2_primary_eventdev_probe(struct rte_eventdev *dev,
> +				const char *name,
> +				struct dlb2_devargs *dlb2_args);
> +
> +int dlb2_secondary_eventdev_probe(struct rte_eventdev *dev,
> +				  const char *name);
> +
> +uint32_t dlb2_get_queue_depth(struct dlb2_eventdev *dlb2,
> +		     struct dlb2_eventdev_queue *queue);
> +
> +/* arg parsing helper functions */
> +int dlb2_parse_params(const char *params,
> +		      const char *name,
> +		      struct dlb2_devargs *dlb2_args);
> +
> +int dlb2_string_to_int(int *result, const char *str);

Looks like this is only used in dlb2.c, is the forward declaration needed?

Thanks,
Gage