Re: [dpdk-dev] [PATCH v3] net/mlx5: return RSS hash result in mbuf

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

Hi all,

Actually while this patch may silence clang, it still contains a bug. len
must remain in the outer scope as its value must be preserved across
iterations. I forgot this while reading the original patch and wrongly
suggested Nelio to move it to in the inner loop, sorry about that.

In short clang properly detected the issue, this was not a false
positive. We should use it more often.

On Wed, Sep 28, 2016 at 09:02:15AM +0200, Nelio Laranjeiro wrote:
> Signed-off-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
> ---
>  doc/guides/nics/mlx5.rst     |  1 +
>  drivers/net/mlx5/mlx5_rxq.c  |  1 +
>  drivers/net/mlx5/mlx5_rxtx.c | 21 ++++++++++++++++-----
>  drivers/net/mlx5/mlx5_rxtx.h |  2 ++
>  4 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/guides/nics/mlx5.rst b/doc/guides/nics/mlx5.rst
> index 8923173..0d1fabb 100644
> --- a/doc/guides/nics/mlx5.rst
> +++ b/doc/guides/nics/mlx5.rst
> @@ -88,6 +88,7 @@ Features
>    RTE_ETH_FDIR_REJECT).
>  - Secondary process TX is supported.
>  - KVM and VMware ESX SR-IOV modes are supported.
> +- RSS hash result is supported.
>  
>  Limitations
>  -----------
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index f6f4315..d32ad68 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -926,6 +926,7 @@ rxq_ctrl_setup(struct rte_eth_dev *dev, struct rxq_ctrl *rxq_ctrl,
>  		.rxq = {
>  			.elts_n = log2above(desc),
>  			.mp = mp,
> +			.rss_hash = priv->rxqs_n > 1,
>  		},
>  	};
>  	struct ibv_exp_wq_attr mod;
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index e0ff47f..4491ae8 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -1128,6 +1128,8 @@ rxq_cq_to_pkt_type(volatile struct mlx5_cqe64 *cqe)
>   *   Pointer to RX queue.
>   * @param cqe
>   *   CQE to process.
> + * @param[out] rss_hash
> + *   Packet RSS Hash result.
>   *
>   * @return
>   *   Packet size in bytes (0 if there is none), -1 in case of completion
> @@ -1135,7 +1137,7 @@ rxq_cq_to_pkt_type(volatile struct mlx5_cqe64 *cqe)
>   */
>  static inline int
>  mlx5_rx_poll_len(struct rxq *rxq, volatile struct mlx5_cqe64 *cqe,
> -		 uint16_t cqe_cnt)
> +		 uint16_t cqe_cnt, uint32_t *rss_hash)
>  {
>  	struct rxq_zip *zip = &rxq->zip;
>  	uint16_t cqe_n = cqe_cnt + 1;
> @@ -1148,6 +1150,7 @@ mlx5_rx_poll_len(struct rxq *rxq, volatile struct mlx5_cqe64 *cqe,
>  			(uintptr_t)(&(*rxq->cqes)[zip->ca & cqe_cnt].cqe64);
>  
>  		len = ntohl((*mc)[zip->ai & 7].byte_cnt);
> +		*rss_hash = ntohl((*mc)[zip->ai & 7].rx_hash_result);
>  		if ((++zip->ai & 7) == 0) {
>  			/*
>  			 * Increment consumer index to skip the number of
> @@ -1202,9 +1205,11 @@ mlx5_rx_poll_len(struct rxq *rxq, volatile struct mlx5_cqe64 *cqe,
>  			zip->cq_ci = rxq->cq_ci + zip->cqe_cnt;
>  			/* Get packet size to return. */
>  			len = ntohl((*mc)[0].byte_cnt);
> +			*rss_hash = ntohl((*mc)[0].rx_hash_result);
>  			zip->ai = 1;
>  		} else {
>  			len = ntohl(cqe->byte_cnt);
> +			*rss_hash = ntohl(cqe->rx_hash_res);
>  		}
>  		/* Error while receiving packet. */
>  		if (unlikely(MLX5_CQE_OPCODE(op_own) == MLX5_CQE_RESP_ERR))
> @@ -1286,12 +1291,13 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  		&(*rxq->cqes)[rxq->cq_ci & cqe_cnt].cqe64;
>  	unsigned int i = 0;
>  	unsigned int rq_ci = rxq->rq_ci << sges_n;
> -	int len;
>  
>  	while (pkts_n) {
>  		unsigned int idx = rq_ci & wqe_cnt;
>  		volatile struct mlx5_wqe_data_seg *wqe = &(*rxq->wqes)[idx];
>  		struct rte_mbuf *rep = (*rxq->elts)[idx];
> +		int len = 0;
> +		uint32_t rss_hash_res = 0;
>  
>  		if (pkt)
>  			NEXT(seg) = rep;
> @@ -1320,8 +1326,9 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  		}
>  		if (!pkt) {
>  			cqe = &(*rxq->cqes)[rxq->cq_ci & cqe_cnt].cqe64;
> -			len = mlx5_rx_poll_len(rxq, cqe, cqe_cnt);
> -			if (len == 0) {
> +			len = mlx5_rx_poll_len(rxq, cqe, cqe_cnt,
> +					       &rss_hash_res);
> +			if (!len) {
>  				rte_mbuf_refcnt_set(rep, 0);
>  				__rte_mbuf_raw_free(rep);
>  				break;
> @@ -1338,12 +1345,16 @@ mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n)
>  			/* Update packet information. */
>  			pkt->packet_type = 0;
>  			pkt->ol_flags = 0;
> +			if (rxq->rss_hash) {
> +				pkt->hash.rss = rss_hash_res;
> +				pkt->ol_flags = PKT_RX_RSS_HASH;
> +			}
>  			if (rxq->csum | rxq->csum_l2tun | rxq->vlan_strip |
>  			    rxq->crc_present) {
>  				if (rxq->csum) {
>  					pkt->packet_type =
>  						rxq_cq_to_pkt_type(cqe);
> -					pkt->ol_flags =
> +					pkt->ol_flags |=
>  						rxq_cq_to_ol_flags(rxq, cqe);
>  				}
>  				if (cqe->l4_hdr_type_etc &
> diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
> index 9828aef..e813f38 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.h
> +++ b/drivers/net/mlx5/mlx5_rxtx.h
> @@ -113,6 +113,8 @@ struct rxq {
>  	unsigned int cqe_n:4; /* Log 2 of CQ elements. */
>  	unsigned int elts_n:4; /* Log 2 of Mbufs. */
>  	unsigned int port_id:8;
> +	unsigned int rss_hash:1; /* RSS hash result is enabled. */
> +	unsigned int :9; /* Remaining bits. */
>  	volatile uint32_t *rq_db;
>  	volatile uint32_t *cq_db;
>  	uint16_t rq_ci;
> -- 
> 2.1.4
> 

-- 
Adrien Mazarguil
6WIND