From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2] mlx5: Support for rte_eth_rx_queue_count Date: Fri, 26 Oct 2018 21:47:40 +0000 Message-ID: <20181026214729.GA13615@mtidpdk.mti.labs.mlnx> References: <1540565309-135175-1-git-send-email-barbette@kth.se> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , Shahaf Shuler To: Tom Barbette Return-path: Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00054.outbound.protection.outlook.com [40.107.0.54]) by dpdk.org (Postfix) with ESMTP id 9B48E2C52 for ; Fri, 26 Oct 2018 23:47:41 +0200 (CEST) In-Reply-To: <1540565309-135175-1-git-send-email-barbette@kth.se> Content-Language: en-US Content-ID: <410CD17CEEF4D94CA5E2B8D73996E1AF@eurprd05.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Oct 26, 2018 at 04:48:29PM +0200, Tom Barbette wrote: > This patch adds support for the rx_queue_count API in mlx5 driver >=20 > Changes in v2: > * Fixed styling issues > * Fix missing return >=20 > Signed-off-by: Tom Barbette > --- Thank you for your contribution! It is good but I have a few small comments. Mostly cosmetic ones. > drivers/net/mlx5/mlx5.c | 1 + > drivers/net/mlx5/mlx5_rxtx.c | 60 +++++++++++++++++++++++++++++++++++++-= ------ > drivers/net/mlx5/mlx5_rxtx.h | 1 + > 3 files changed, 53 insertions(+), 9 deletions(-) >=20 > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c > index ec63bc6..6fccadd 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -375,6 +375,7 @@ const struct eth_dev_ops mlx5_dev_ops =3D { > .filter_ctrl =3D mlx5_dev_filter_ctrl, > .rx_descriptor_status =3D mlx5_rx_descriptor_status, > .tx_descriptor_status =3D mlx5_tx_descriptor_status, > + .rx_queue_count =3D mlx5_rx_queue_count, > .rx_queue_intr_enable =3D mlx5_rx_intr_enable, > .rx_queue_intr_disable =3D mlx5_rx_intr_disable, > .is_removed =3D mlx5_is_removed, > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c > index 2d14f8a..cafb084 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.c > +++ b/drivers/net/mlx5/mlx5_rxtx.c > @@ -417,20 +417,17 @@ mlx5_tx_descriptor_status(void *tx_queue, uint16_t = offset) > } > =20 > /** > - * DPDK callback to check the status of a rx descriptor. > + * Internal function to compute the number of used descriptors in an RX = queue > * > - * @param rx_queue > - * The rx queue. I know you just copied it but I'd like to fix it by this opportunity. :-) Please change 'rx' to 'Rx'. Same for 'tx' > - * @param[in] offset > - * The index of the descriptor in the ring. > + * @param tx_queue > + * The tx queue. 'tx' -> 'Rx' > * > * @return > - * The status of the tx descriptor. > + * The number of used rx descriptor. > */ > -int > -mlx5_rx_descriptor_status(void *rx_queue, uint16_t offset) > +static uint32_t > +rx_queue_count(struct mlx5_rxq_data *rxq) > { > - struct mlx5_rxq_data *rxq =3D rx_queue; > struct rxq_zip *zip =3D &rxq->zip; > volatile struct mlx5_cqe *cqe; > const unsigned int cqe_n =3D (1 << rxq->cqe_n); > @@ -461,12 +458,57 @@ mlx5_rx_descriptor_status(void *rx_queue, uint16_t = offset) > cqe =3D &(*rxq->cqes)[cq_ci & cqe_cnt]; > } > used =3D RTE_MIN(used, (1U << rxq->elts_n) - 1); > + return used; > +} > + > +/** > + * DPDK callback to check the status of a rx descriptor. > + * > + * @param rx_queue > + * The rx queue. > + * @param[in] offset > + * The index of the descriptor in the ring. > + * > + * @return > + * The status of the tx descriptor. 'tx' -> 'Rx' > + */ > +int > +mlx5_rx_descriptor_status(void *rx_queue, uint16_t offset) > +{ > + struct mlx5_rxq_data *rxq =3D rx_queue; > + Please remove this blank line. We only allow a blank line between variable declarations and the body. > + uint32_t used =3D rx_queue_count(rxq); To be honest, there's a known issue that should've been fixed before. It is= only vaild for regular Rx burst. In mlx5, there are three types of rx_burst - mlx5_rx_burst(), mlx5_rx_burst_mprq() and mlx5_rx_burst_vec(). You can refe= r to mlx5_select_rx_function(). Among them, this works only with mlx5_rx_burst()= . So, if you don't mind can you please add two sanity checks here? { struct mlx5_rxq_ctrl *rxq_ctrl =3D container_of(rxq, struct mlx5_rxq_ctrl, rxq); struct rte_eth_dev *dev =3D ETH_DEV(rxq_ctrl->priv); if (dev->rx_pkt_burst !=3D mlx5_rx_burst) { rte_errno =3D ENOTSUP; return -rte_errno; } if (offset >=3D (1 << rxq->elts_n)) { rte_errno =3D EINVAL; return -rte_errno; } if (offset < rx_queue_count(rxq)) return RTE_ETH_RX_DESC_DONE; return RTE_ETH_RX_DESC_AVAIL; } > =20 > /** > + * DPDK callback to get the number of used descriptors in a RX queue > + * > + * @param tx_queue > + * The tx queue. Parameters are dev and rx_queue_id. > + * > + * @return > + * The number of used rx descriptor. > + * -EINVAL if the queue is invalid > + */ > +uint32_t > +mlx5_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id) > +{ > + struct priv *priv =3D dev->data->dev_private; > + struct mlx5_rxq_data *rxq; > + > + rxq =3D (*priv->rxqs)[rx_queue_id]; > + if (!rxq) { > + rte_errno =3D EINVAL; > + return -rte_errno; > + } And please do the same check here. if (dev->rx_pkt_burst !=3D mlx5_rx_burst) { rte_errno =3D ENOTSUP; return -rte_errno; } > + Please remove this blank line. Thanks, Yongseok > + return rx_queue_count(rxq); > +} > + > +/** > * DPDK callback for TX. > * > * @param dpdk_txq > diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h > index 48ed2b2..c82059b 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.h > +++ b/drivers/net/mlx5/mlx5_rxtx.h > @@ -345,6 +345,7 @@ uint16_t removed_rx_burst(void *dpdk_rxq, struct rte_= mbuf **pkts, > uint16_t pkts_n); > int mlx5_rx_descriptor_status(void *rx_queue, uint16_t offset); > int mlx5_tx_descriptor_status(void *tx_queue, uint16_t offset); > +uint32_t mlx5_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_= id); > =20 > /* Vectorized version of mlx5_rxtx.c */ > int mlx5_check_raw_vec_tx_support(struct rte_eth_dev *dev); > --=20 > 2.7.4 >=20