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

["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 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 <timothy.mcdaniel@intel.com>
> ---
>  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
upper bound check can be dropped, since it's already done in
dlb_eventdev_port_setup().

> +
> +	rte_spinlock_lock(&handle->resource_lock);
> +
> +	cfg.response = (uintptr_t)&response;
> +
> +	/* We round up to the next power of 2 if necessary */
> +	cfg.cq_depth = rte_align32pow2(cq_depth);
> +	cfg.cq_depth_threshold = rsvd_tokens;
> +
> +	cfg.cq_history_list_size =
> DLB_NUM_HIST_LIST_ENTRIES_PER_LDB_PORT;
> +
> +	/* User controls the LDB high watermark via enqueue depth. The DIR
> high
> +	 * watermark is equal, unless the directed credit pool is too small.
> +	 */
> +	cfg.ldb_credit_high_watermark = enqueue_depth;
> +
> +	/* If there are no directed ports, the kernel driver will ignore this
> +	 * port's directed credit settings. Don't use enqueue_depth if it would
> +	 * require more directed credits than are available.
> +	 */
> +	cfg.dir_credit_high_watermark =
> +		RTE_MIN(enqueue_depth,
> +			handle->cfg.num_dir_credits / dlb->num_ports);
> +
> +	cfg.ldb_credit_quantum = cfg.ldb_credit_high_watermark / 2;
> +	cfg.ldb_credit_low_watermark = RTE_MIN(16, cfg.ldb_credit_quantum);
> +
> +	cfg.dir_credit_quantum = cfg.dir_credit_high_watermark / 2;
> +	cfg.dir_credit_low_watermark = RTE_MIN(16, cfg.dir_credit_quantum);
> +
> +	/* Per QM values */
> +
> +	cfg.ldb_credit_pool_id = handle->cfg.ldb_credit_pool_id;
> +	cfg.dir_credit_pool_id = handle->cfg.dir_credit_pool_id;
> +
> +	ret = dlb_iface_ldb_port_create(handle, &cfg, dlb->poll_mode);
> +	if (ret < 0) {
> +		DLB_LOG_ERR("dlb: dlb_ldb_port_create error, ret=%d (driver
> status: %s)\n",
> +			    ret, dlb_error_strings[response.status]);
> +		goto error_exit;
> +	}
> +
> +	qm_port_id = response.id;
> +
> +	DLB_LOG_DBG("dlb: ev_port %d uses qm LB port %d <<<<<\n",
> +		    ev_port->id, qm_port_id);
> +
> +	qm_port = &ev_port->qm_port;
> +	qm_port->ev_port = ev_port; /* back ptr */
> +	qm_port->dlb = dlb; /* back ptr */
> +
> +	/*
> +	 * Allocate and init local qe struct(s).
> +	 * Note: MOVDIR64 requires the enqueue QE (qe4) to be aligned.
> +	 */
> +
> +	snprintf(mz_name, sizeof(mz_name), "%s_ldb_port%d",
> +		 handle->device_name,
> +		 ev_port->id);

I don't believe device_name is initialized.

> +
> +	ret = dlb_init_qe_mem(qm_port, mz_name);
> +	if (ret < 0) {
> +		DLB_LOG_ERR("dlb: init_qe_mem failed, ret=%d\n", ret);
> +		goto error_exit;
> +	}
> +
> +	qm_port->pp_mmio_base = DLB_LDB_PP_BASE + PAGE_SIZE *
> qm_port_id;
> +	qm_port->id = qm_port_id;
> +
> +	/* The credit window is one high water mark of QEs */
> +	qm_port->ldb_pushcount_at_credit_expiry = 0;
> +	qm_port->cached_ldb_credits = cfg.ldb_credit_high_watermark;
> +	/* The credit window is one high water mark of QEs */
> +	qm_port->dir_pushcount_at_credit_expiry = 0;
> +	qm_port->cached_dir_credits = cfg.dir_credit_high_watermark;
> +	qm_port->cq_depth = cfg.cq_depth;
> +	/* CQs with depth < 8 use an 8-entry queue, but withhold credits so
> +	 * the effective depth is smaller.
> +	 */
> +	qm_port->cq_depth = cfg.cq_depth <= 8 ? 8 : cfg.cq_depth;
> +	qm_port->cq_idx = 0;
> +	qm_port->cq_idx_unmasked = 0;
> +	if (dlb->poll_mode == DLB_CQ_POLL_MODE_SPARSE)
> +		qm_port->cq_depth_mask = (qm_port->cq_depth * 4) - 1;
> +	else
> +		qm_port->cq_depth_mask = qm_port->cq_depth - 1;
> +
> +	qm_port->gen_bit_shift = __builtin_popcount(qm_port-
> >cq_depth_mask);
> +	/* starting value of gen bit - it toggles at wrap time */
> +	qm_port->gen_bit = 1;
> +
> +	qm_port->use_rsvd_token_scheme = use_rsvd_token_scheme;
> +	qm_port->cq_rsvd_token_deficit = rsvd_tokens;
> +	qm_port->int_armed = false;
> +
> +	/* Save off for later use in info and lookup APIs. */
> +	qm_port->qid_mappings = &dlb->qm_ldb_to_ev_queue_id[0];
> +
> +	/* When using the reserved token scheme, token_pop_thresh is
> +	 * initially 2 * dequeue_depth. Once the tokens are reserved,
> +	 * the enqueue code re-assigns it to dequeue_depth.
> +	 */
> +	qm_port->dequeue_depth = dequeue_depth;
> +	qm_port->token_pop_thresh = cq_depth;
> +
> +	/* When the deferred scheduling vdev arg is selected, use deferred pop
> +	 * for all single-entry CQs.
> +	 */
> +	if (cfg.cq_depth == 1 || (cfg.cq_depth == 2 &&
> use_rsvd_token_scheme)) {
> +		if (dlb->defer_sched)
> +			qm_port->token_pop_mode = DEFERRED_POP;
> +	}
> +
> +	qm_port->owed_tokens = 0;
> +	qm_port->issued_releases = 0;
> +
> +	/* Save config message too. */
> +	rte_memcpy(&qm_port->cfg.ldb, &cfg, sizeof(cfg));

Safer to do sizeof() on the destination than the source, I think.

> +
> +	/* update state */
> +	qm_port->state = PORT_STARTED; /* enabled at create time */
> +	qm_port->config_state = DLB_CONFIGURED;
> +
> +	qm_port->dir_credits = cfg.dir_credit_high_watermark;
> +	qm_port->ldb_credits = cfg.ldb_credit_high_watermark;
> +
> +	DLB_LOG_DBG("dlb: created ldb port %d, depth = %d, ldb credits=%d, dir
> credits=%d\n",
> +		    qm_port_id,
> +		    cq_depth,
> +		    qm_port->ldb_credits,
> +		    qm_port->dir_credits);
> +
> +	rte_spinlock_unlock(&handle->resource_lock);
> +
> +	return 0;
> +
> +error_exit:
> +	if (qm_port) {
> +		dlb_free_qe_mem(qm_port);
> +		qm_port->pp_mmio_base = 0;
> +	}
> +
> +	rte_spinlock_unlock(&handle->resource_lock);
> +
> +	DLB_LOG_ERR("dlb: create ldb port failed!\n");
> +
> +	return ret;
> +}
> +
> +static int
> +dlb_hw_create_dir_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_dir_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 (dlb == NULL || handle == NULL)
> +		return -EINVAL;
> +
> +	if (cq_depth < DLB_MIN_DIR_CQ_DEPTH ||
> +	    cq_depth > DLB_MAX_INPUT_QUEUE_DEPTH) {
> +		DLB_LOG_ERR("dlb: invalid cq_depth, must be %d-%d\n",
> +			    DLB_MIN_DIR_CQ_DEPTH,
> DLB_MAX_INPUT_QUEUE_DEPTH);
> +		return -EINVAL;
> +	}

Enqueue depth check needed?

> +
> +	rte_spinlock_lock(&handle->resource_lock);
> +
> +	/* Directed queues are configured at link time. */
> +	cfg.queue_id = -1;
> +
> +	cfg.response = (uintptr_t)&response;
> +
> +	/* We round up to the next power of 2 if necessary */
> +	cfg.cq_depth = rte_align32pow2(cq_depth);
> +	cfg.cq_depth_threshold = rsvd_tokens;
> +
> +	/* User controls the LDB high watermark via enqueue depth. The DIR
> high
> +	 * watermark is equal, unless the directed credit pool is too small.
> +	 */
> +	cfg.ldb_credit_high_watermark = enqueue_depth;
> +
> +	/* Don't use enqueue_depth if it would require more directed credits
> +	 * than are available.
> +	 */
> +	cfg.dir_credit_high_watermark =
> +		RTE_MIN(enqueue_depth,
> +			handle->cfg.num_dir_credits / dlb->num_ports);
> +
> +	cfg.ldb_credit_quantum = cfg.ldb_credit_high_watermark / 2;
> +	cfg.ldb_credit_low_watermark = RTE_MIN(16, cfg.ldb_credit_quantum);
> +
> +	cfg.dir_credit_quantum = cfg.dir_credit_high_watermark / 2;
> +	cfg.dir_credit_low_watermark = RTE_MIN(16, cfg.dir_credit_quantum);
> +
> +	/* Per QM values */
> +
> +	cfg.ldb_credit_pool_id = handle->cfg.ldb_credit_pool_id;
> +	cfg.dir_credit_pool_id = handle->cfg.dir_credit_pool_id;
> +
> +	ret = dlb_iface_dir_port_create(handle, &cfg, dlb->poll_mode);
> +	if (ret < 0) {
> +		DLB_LOG_ERR("dlb: dlb_dir_port_create error, ret=%d (driver
> status: %s)\n",
> +			    ret, dlb_error_strings[response.status]);
> +		goto error_exit;
> +	}
> +
> +	qm_port_id = response.id;
> +
> +	DLB_LOG_DBG("dlb: ev_port %d uses qm DIR port %d <<<<<\n",
> +		    ev_port->id, qm_port_id);
> +
> +	qm_port = &ev_port->qm_port;
> +	qm_port->ev_port = ev_port; /* back ptr */
> +	qm_port->dlb = dlb;  /* back ptr */
> +
> +	/*
> +	 * Init local qe struct(s).
> +	 * Note: MOVDIR64 requires the enqueue QE to be aligned
> +	 */
> +
> +	snprintf(mz_name, sizeof(mz_name), "%s_dir_port%d",
> +		 handle->device_name,
> +		 ev_port->id);

(See device_name comment above)

> +
> +	ret = dlb_init_qe_mem(qm_port, mz_name);
> +
> +	if (ret < 0) {
> +		DLB_LOG_ERR("dlb: init_qe_mem failed, ret=%d\n", ret);
> +		goto error_exit;
> +	}
> +
> +	qm_port->pp_mmio_base = DLB_DIR_PP_BASE + PAGE_SIZE *
> qm_port_id;
> +	qm_port->id = qm_port_id;
> +
> +	/* The credit window is one high water mark of QEs */
> +	qm_port->ldb_pushcount_at_credit_expiry = 0;
> +	qm_port->cached_ldb_credits = cfg.ldb_credit_high_watermark;
> +	/* The credit window is one high water mark of QEs */
> +	qm_port->dir_pushcount_at_credit_expiry = 0;
> +	qm_port->cached_dir_credits = cfg.dir_credit_high_watermark;
> +	qm_port->cq_depth = cfg.cq_depth;
> +	qm_port->cq_idx = 0;
> +	qm_port->cq_idx_unmasked = 0;
> +	if (dlb->poll_mode == DLB_CQ_POLL_MODE_SPARSE)
> +		qm_port->cq_depth_mask = (cfg.cq_depth * 4) - 1;
> +	else
> +		qm_port->cq_depth_mask = cfg.cq_depth - 1;
> +
> +	qm_port->gen_bit_shift = __builtin_popcount(qm_port-
> >cq_depth_mask);
> +	/* starting value of gen bit - it toggles at wrap time */
> +	qm_port->gen_bit = 1;
> +
> +	qm_port->use_rsvd_token_scheme = use_rsvd_token_scheme;
> +	qm_port->cq_rsvd_token_deficit = rsvd_tokens;
> +	qm_port->int_armed = false;
> +
> +	/* Save off for later use in info and lookup APIs. */
> +	qm_port->qid_mappings = &dlb->qm_dir_to_ev_queue_id[0];
> +
> +	qm_port->dequeue_depth = dequeue_depth;
> +
> +	/* Directed ports are auto-pop, by default. */
> +	qm_port->token_pop_mode = AUTO_POP;
> +	qm_port->owed_tokens = 0;
> +	qm_port->issued_releases = 0;
> +
> +	/* Save config message too. */
> +	rte_memcpy(&qm_port->cfg.dir, &cfg, sizeof(cfg));

(See sizeof() comment above)

[...]

> diff --git a/drivers/event/dlb/pf/dlb_pf.c b/drivers/event/dlb/pf/dlb_pf.c
> index fffb88b..cd766d3 100644
> --- a/drivers/event/dlb/pf/dlb_pf.c
> +++ b/drivers/event/dlb/pf/dlb_pf.c
> @@ -221,6 +221,207 @@ dlb_pf_ldb_queue_create(struct dlb_hw_dev *handle,
>  }
> 
>  static int
> +dlb_pf_dir_queue_create(struct dlb_hw_dev *handle,
> +			struct dlb_create_dir_queue_args *cfg)
> +{
> +	struct dlb_dev *dlb_dev = (struct dlb_dev *)handle->pf_dev;
> +	struct dlb_cmd_response response = {0};
> +	int ret;
> +
> +	DLB_INFO(dev->dlb_device, "Entering %s()\n", __func__);
> +
> +	ret = dlb_hw_create_dir_queue(&dlb_dev->hw,
> +				      handle->domain_id,
> +				      cfg,
> +				      &response);
> +
> +	*(struct dlb_cmd_response *)cfg->response = response;
> +
> +	DLB_INFO(dev->dlb_device, "Exiting %s() with ret=%d\n", __func__,
> ret);
> +
> +	return ret;
> +}
> +
> +static void *
> +dlb_alloc_coherent_aligned(rte_iova_t *phys, size_t size, int align)
> +{
> +	const struct rte_memzone *mz;
> +	char mz_name[RTE_MEMZONE_NAMESIZE];
> +	uint32_t core_id = rte_lcore_id();
> +	unsigned int socket_id;
> +
> +	snprintf(mz_name, sizeof(mz_name) - 1, "%lx",
> +		 (unsigned long)rte_get_timer_cycles());

For debug purposes, it would be better if this name can trace the mz back to
this driver. How about something like event_dlb2_pf_name + ldb/dir_port + port ID?

I also don't see the port memzones getting freed anywhere, e.g. if the event device
is reset. Looks like a possible memory leak.

Thanks,
Gage