Re: [dpdk-dev] [PATCH v2] net/mlx4: fix no Rx interrupts

["Nélio Laranjeiro" <nelio.laranjeiro@6wind.com>]

Hi Moti,

On Tue, Oct 24, 2017 at 04:11:14PM +0300, Moti Haimovsky wrote:
> This commit addresses the issue of rx interrupts support with
> the new Rx datapath introduced in DPDK version 17.11.
> In order to generate an Rx interrupt an event queue is armed with the
> consumer index of the Rx completion queue. Since version 17.11 this
> index is handled by the PMD so it is now the responsibility of the
> PMD to write this value when enabling Rx interrupts.
> 
> Fixes: 6681b845034c ("net/mlx4: add Rx bypassing Verbs")
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> V2:
> * Rebased on top of ff3397e9 ("net/mlx4: relax Rx queue configuration order")
> ---
>  drivers/net/mlx4/mlx4_intr.c | 38 +++++++++++++++++++++++++++++++++++++-
>  drivers/net/mlx4/mlx4_prm.h  |  9 +++++++++
>  drivers/net/mlx4/mlx4_rxq.c  |  6 +++++-
>  3 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4_intr.c b/drivers/net/mlx4/mlx4_intr.c
> index 3806322..217c591 100644
> --- a/drivers/net/mlx4/mlx4_intr.c
> +++ b/drivers/net/mlx4/mlx4_intr.c
> @@ -53,6 +53,7 @@
>  #include <rte_alarm.h>
>  #include <rte_errno.h>
>  #include <rte_ethdev.h>
> +#include <rte_io.h>
>  #include <rte_interrupts.h>
>  
>  #include "mlx4.h"
> @@ -239,6 +240,40 @@
>  }
>  
>  /**
> + * MLX4 CQ notification .
> + *
> + * @param rxq
> + *   Pointer to receive queue structure.
> + * @param solicited
> + *   Is request solicited or not.
> + *
> + * @return
> + *   Always 0.

Why not setting the return type to void in this case?

> + */
> +static int
> +mlx4_arm_cq(struct rxq *rxq, int solicited)
> +{
> +	struct mlx4_cq *cq = &rxq->mcq;
> +	uint64_t doorbell;
> +	uint32_t sn = cq->arm_sn & MLX4_CQ_DB_GEQ_N_MASK;
> +	uint32_t ci = cq->cons_index & MLX4_CQ_DB_CI_MASK;
> +	uint32_t cmd = solicited ? MLX4_CQ_DB_REQ_NOT_SOL : MLX4_CQ_DB_REQ_NOT;
> +	void *cq_db_reg = (char *)cq->cq_uar + MLX4_CQ_DOORBELL;

use uint8_t* instead of char*.

By the way, why not storing the address to the doorbell directly in the
structure instead of computing it each time it is necessary?

> +	*cq->arm_db = rte_cpu_to_be_32(sn << 28 | cmd | ci);
> +	/*
> +	 * Make sure that the doorbell record in host memory is
> +	 * written before ringing the doorbell via PCI MMIO.
> +	 */
> +	rte_wmb();
> +	doorbell = sn << 28 | cmd | cq->cqn;
> +	doorbell <<= 32;
> +	doorbell |= ci;
> +	rte_write64(rte_cpu_to_be_64(doorbell), cq_db_reg);
> +	return 0;
> +}
> +
> +/**
>   * Uninstall interrupt handler.
>   *
>   * @param priv
> @@ -333,6 +368,7 @@
>  		WARN("unable to disable interrupt on rx queue %d",
>  		     idx);
>  	} else {
> +		rxq->mcq.arm_sn++;
>  		ibv_ack_cq_events(rxq->cq, 1);
>  	}
>  	return -ret;
> @@ -358,7 +394,7 @@
>  	if (!rxq || !rxq->channel)
>  		ret = EINVAL;
>  	else
> -		ret = ibv_req_notify_cq(rxq->cq, 0);
> +		ret = mlx4_arm_cq(rxq, 0);
>  	if (ret) {
>  		rte_errno = ret;
>  		WARN("unable to arm interrupt on rx queue %d", idx);
> diff --git a/drivers/net/mlx4/mlx4_prm.h b/drivers/net/mlx4/mlx4_prm.h
> index 3a77502..92f2fec 100644
> --- a/drivers/net/mlx4/mlx4_prm.h
> +++ b/drivers/net/mlx4/mlx4_prm.h
> @@ -78,6 +78,11 @@ enum {
>  	MLX4_CQE_L2_TUNNEL_IPOK = (int)(1u << 31),
>  };
>  
> +/* Completion queue events, numbers and masks. */
> +#define MLX4_CQ_DB_GEQ_N_MASK 0x3
> +#define MLX4_CQ_DB_CI_MASK 0xffffff

You could have replaced all the places where this mask is hard coded by
this new define for consistency.

> +#define MLX4_CQ_DOORBELL 0x20
> +
>  /* Send queue information. */
>  struct mlx4_sq {
>  	uint8_t *buf; /**< SQ buffer. */
> @@ -100,6 +105,10 @@ struct mlx4_cq {
>  	uint32_t cqe_64:1; /**< CQ entry size is 64 bytes. */
>  	uint32_t cons_index; /**< Last queue entry that was handled. */
>  	uint32_t *set_ci_db; /**< Pointer to the completion queue doorbell. */
> +	uint32_t *arm_db; /**< Pointer to doorbell for arming Rx events. */
> +	int arm_sn; /**< Rx event counter. */
> +	void *cq_uar; /**< CQ user access region. */
> +	uint32_t cqn; /**< CQ number. */

Try to re-arrange the variable order to avoid holes in the structure.

>  };
>  
>  /**
> diff --git a/drivers/net/mlx4/mlx4_rxq.c b/drivers/net/mlx4/mlx4_rxq.c
> index 4c50077..24262cb 100644
> --- a/drivers/net/mlx4/mlx4_rxq.c
> +++ b/drivers/net/mlx4/mlx4_rxq.c
> @@ -510,7 +510,7 @@ void mlx4_rss_detach(struct mlx4_rss *rss)
>  	struct rte_mbuf *(*elts)[elts_n] = rxq->elts;
>  	struct mlx4dv_obj mlxdv;
>  	struct mlx4dv_rwq dv_rwq;
> -	struct mlx4dv_cq dv_cq;
> +	struct mlx4dv_cq dv_cq = { .comp_mask = MLX4DV_CQ_MASK_UAR, };
>  	const char *msg;
>  	struct ibv_cq *cq = NULL;
>  	struct ibv_wq *wq = NULL;
> @@ -604,6 +604,10 @@ void mlx4_rss_detach(struct mlx4_rss *rss)
>  	rxq->mcq.cqe_cnt = dv_cq.cqe_cnt;
>  	rxq->mcq.set_ci_db = dv_cq.set_ci_db;
>  	rxq->mcq.cqe_64 = (dv_cq.cqe_size & 64) ? 1 : 0;
> +	rxq->mcq.arm_db = dv_cq.arm_db;
> +	rxq->mcq.arm_sn = dv_cq.arm_sn;
> +	rxq->mcq.cqn = dv_cq.cqn;
> +	rxq->mcq.cq_uar = dv_cq.cq_uar;
>  	/* Update doorbell counter. */
>  	rxq->rq_ci = elts_n / sges_n;
>  	rte_wmb();
> -- 
> 1.8.3.1

Thanks,

-- 
Nélio Laranjeiro
6WIND