Re: [dpdk-dev] [PATCH v4 03/22] event/dlb: 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 2:18 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 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;
> +	uint64_t tx_nospc_inflight_max;
> +	uint64_t tx_nospc_new_event_limit;
> +	uint64_t tx_nospc_inflight_credits;
> +};
> +
> +struct dlb_port_stats {
> +	struct dlb_traffic_stats traffic;
> +	uint64_t tx_op_cnt[4]; /* indexed by rte_event.op */
> +	uint64_t tx_implicit_rel;
> +	uint64_t tx_sched_cnt[DLB_NUM_HW_SCHED_TYPES];
> +	uint64_t tx_invalid;
> +	uint64_t rx_sched_cnt[DLB_NUM_HW_SCHED_TYPES];
> +	uint64_t rx_sched_invalid;
> +	uint64_t enq_ok[DLB_MAX_NUM_QUEUES]; /* per-queue enq_ok
> */
> +};
> +
> +struct dlb_eventdev_port {
> +	struct dlb_port qm_port; /* hw specific data structure */
> +	struct rte_event_port_conf conf; /* user-supplied configuration */
> +	uint16_t inflight_credits; /* num credits this port has right now */
> +	uint16_t credit_update_quanta;
> +	struct dlb_eventdev *dlb; /* backlink optimization */
> +	struct dlb_port_stats stats __rte_cache_aligned;
> +	struct dlb_event_queue_link
> link[DLB_MAX_NUM_QIDS_PER_LDB_CQ];
> +	int num_links;
> +	uint32_t id;
> +	/* num releases yet to be completed on this port.
> +	 * Only applies to load-balanced ports.
> +	 */
> +	uint16_t outstanding_releases;
> +	uint16_t inflight_max; /* app requested max inflights for this port */
> +	/* setup_done is set when the event port is setup */
> +	bool setup_done;
> +	/* enq_configured is set when the qm port is created */
> +	bool enq_configured;
> +	uint8_t implicit_release; /* release events before dequeueing */
> +} __rte_cache_aligned;
> +
> +struct dlb_queue {
> +	uint32_t num_qid_inflights; /* User config */
> +	uint32_t num_atm_inflights; /* User config */
> +	enum dlb_configuration_state config_state;
> +	int sched_type; /* LB queue only */
> +	uint32_t id;
> +	bool is_directed;
> +};
> +
> +struct dlb_eventdev_queue {
> +	struct dlb_queue qm_queue;
> +	struct rte_event_queue_conf conf; /* User config */
> +	uint64_t enq_ok;
> +	uint32_t id;
> +	bool setup_done;
> +	uint8_t num_links;
> +};
> +
> +enum dlb_run_state {
> +	DLB_RUN_STATE_STOPPED = 0,
> +	DLB_RUN_STATE_STOPPING,
> +	DLB_RUN_STATE_STARTING,
> +	DLB_RUN_STATE_STARTED
> +};
> +
> +#define DLB_IS_VDEV true
> +#define DLB_NOT_VDEV false

Both these macros are unused

> +
> +struct dlb_eventdev {
> +	struct dlb_eventdev_port ev_ports[DLB_MAX_NUM_PORTS];
> +	struct dlb_eventdev_queue
> ev_queues[DLB_MAX_NUM_QUEUES];
> +	uint8_t qm_ldb_to_ev_queue_id[DLB_MAX_NUM_QUEUES];
> +	uint8_t qm_dir_to_ev_queue_id[DLB_MAX_NUM_QUEUES];
> +
> +	/* store num stats and offset of the stats for each queue */
> +	uint16_t xstats_count_per_qid[DLB_MAX_NUM_QUEUES];
> +	uint16_t xstats_offset_for_qid[DLB_MAX_NUM_QUEUES];
> +
> +	/* store num stats and offset of the stats for each port */
> +	uint16_t xstats_count_per_port[DLB_MAX_NUM_PORTS];
> +	uint16_t xstats_offset_for_port[DLB_MAX_NUM_PORTS];
> +	struct dlb_get_num_resources_args hw_rsrc_query_results;
> +	uint32_t xstats_count_mode_queue;
> +	struct dlb_hw_dev qm_instance; /* strictly hw related */
> +	uint64_t global_dequeue_wait_ticks;
> +	struct dlb_xstats_entry *xstats;
> +	struct rte_eventdev *event_dev; /* backlink to dev */
> +	uint32_t xstats_count_mode_port;
> +	uint32_t xstats_count_mode_dev;
> +	uint32_t xstats_count;
> +	uint32_t inflights; /* use __atomic builtins to access */
> +	uint32_t new_event_limit;
> +	int max_num_events_override;
> +	int num_dir_credits_override;
> +	volatile enum dlb_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 is_vdev;
> +	bool umwait_allowed;
> +	bool global_dequeue_wait; /* Not using per dequeue wait if true */
> +	bool defer_sched;
> +	unsigned int num_atm_inflights_per_queue;
> +	enum dlb_cq_poll_modes poll_mode;
> +	uint8_t revision;
> +	bool configured;
> +};
> +
> +/* End Eventdev related defines and structs */
> +
> +/* externs */
> +
> +extern struct dlb_dev *dlb_probe(struct rte_pci_device *pdev);

This extern isn't needed, the function is later declared in pf/dlb_main.h

> +extern struct process_local_port_data
> dlb_port[][NUM_DLB_PORT_TYPES];
> +
> +/* Forwards for non-inlined functions */
> +
> +void dlb_free_qe_mem(struct dlb_port *port);
> +
> +int dlb_init_qe_mem(struct dlb_port *port, char *mz_name);
> +
> +int dlb_init_send_qe(struct dlb_port *port, char *mz_name);
> +
> +int dlb_init_partial_qe(struct dlb_port *port, char *mz_name);
> +
> +int dlb_init_fwd_qe(struct dlb_port *port, char *mz_name);
> +
> +int dlb_init_complete_qe(struct dlb_port *port, char *mz_name);
> +
> +int dlb_init_noop_qe(struct dlb_port *port, char *mz_name);

dlb_init_qe_mem -> dlb_init_noop_qe are only used in dlb.c, and
don't need forward declarations (dlb_free_qe_mem() should be
defined 'static' though).

> +
> +int dlb_uninit(const char *name);

This function is never defined

> +
> +void dlb_eventdev_dump(struct rte_eventdev *dev, FILE *f);
> +
> +int dlb_xstats_init(struct dlb_eventdev *dlb);
> +
> +void dlb_xstats_uninit(struct dlb_eventdev *dlb);
> +
> +int dlb_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 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
> *xstat_names,
> +				  unsigned int *ids, unsigned int size);
> +
> +uint64_t dlb_eventdev_xstats_get_by_name(const struct rte_eventdev
> *dev,
> +					 const char *name, unsigned int *id);
> +
> +int dlb_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_dlb_eventdev(void);
> +
> +int dlb_primary_eventdev_probe(struct rte_eventdev *dev,
> +			       const char *name,
> +			       struct dlb_devargs *dlb_args,
> +			       bool is_vdev);
> +
> +int dlb_secondary_eventdev_probe(struct rte_eventdev *dev,
> +				 const char *name,
> +				 bool is_vdev);
> +uint32_t dlb_get_queue_depth(struct dlb_eventdev *dlb,
> +			     struct dlb_eventdev_queue *queue);
> +
> +int set_numa_node(const char *key __rte_unused, const char *value,
> +		  void *opaque);
> +
> +int set_dir_ports(const char *key __rte_unused,
> +		  const char *value __rte_unused,
> +		  void *opaque __rte_unused);
> +
> +int set_dir_queues(const char *key __rte_unused,
> +		   const char *value __rte_unused,
> +		   void *opaque __rte_unused);

The set_dir_* functions are also undefined

> +
> +int set_max_num_events(const char *key __rte_unused, const char
> *value,
> +		       void *opaque);
> +
> +int set_num_dir_credits(const char *key __rte_unused, const char *value,
> +			void *opaque);
> +
> +void dlb_drain(struct rte_eventdev *dev);

Only used in dlb.c, no need for a forward declaration -- should be static
instead.

> +
> +void dlb_entry_points_init(struct rte_eventdev *dev);
> +
> +int dlb_parse_params(const char *params,
> +		     const char *name,
> +		     struct dlb_devargs *dlb_args);
> +
> +int dlb_string_to_int(int *result, const char *str);
> +
> +#endif	/* _DLB_PRIV_H_ */
> diff --git a/drivers/event/dlb/rte_pmd_dlb.h
> b/drivers/event/dlb/rte_pmd_dlb.h

This file should be introduced in the commit "event/dlb: add PMD's token pop 
public interface"

Thanks,
Gage