From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 08/12] pmd/mlx4: add dev_ptype_info_get implementation Date: Tue, 5 Jan 2016 17:18:36 +0100 Message-ID: <20160105161835.GF12095@6wind.com> References: <1451544799-70776-1-git-send-email-jianfeng.tan@intel.com> <1451544799-70776-9-git-send-email-jianfeng.tan@intel.com> <20160104111114.GS3806@6wind.com> <568B3394.7020207@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: "Tan, Jianfeng" Return-path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id B7D659382 for ; Tue, 5 Jan 2016 17:18:55 +0100 (CET) Received: by mail-wm0-f48.google.com with SMTP id f206so29154336wmf.0 for ; Tue, 05 Jan 2016 08:18:55 -0800 (PST) Content-Disposition: inline In-Reply-To: <568B3394.7020207@intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jan 05, 2016 at 11:08:04AM +0800, Tan, Jianfeng wrote: > > > On 1/4/2016 7:11 PM, Adrien Mazarguil wrote: > >Hi Jianfeng, > > > >I'm only commenting the mlx4/mlx5 bits in this message, see below. > > > >On Thu, Dec 31, 2015 at 02:53:15PM +0800, Jianfeng Tan wrote: > >>Signed-off-by: Jianfeng Tan > >>--- > >> drivers/net/mlx4/mlx4.c | 27 +++++++++++++++++++++++++++ > >> 1 file changed, 27 insertions(+) > >> > >>diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c > >>index 207bfe2..85afa32 100644 > >>--- a/drivers/net/mlx4/mlx4.c > >>+++ b/drivers/net/mlx4/mlx4.c > >>@@ -2836,6 +2836,8 @@ rxq_cleanup(struct rxq *rxq) > >> * @param flags > >> * RX completion flags returned by poll_length_flags(). > >> * > >>+ * @note: fix mlx4_dev_ptype_info_get() if any change here. > >>+ * > >> * @return > >> * Packet type for struct rte_mbuf. > >> */ > >>@@ -4268,6 +4270,30 @@ mlx4_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info) > >> priv_unlock(priv); > >> } > >>+static int > >>+mlx4_dev_ptype_info_get(struct rte_eth_dev *dev, uint32_t ptype_mask, > >>+ uint32_t ptypes[]) Note this line is not properly indented (uint32_t should be aligned like the rest of the file). > >>+{ > >>+ int num = 0; > >>+ > >>+ if ((dev->rx_pkt_burst == mlx4_rx_burst) > >>+ || (dev->rx_pkt_burst == mlx4_rx_burst_sp)) { I prefer operators/separators at the end of the previous line, indentation should be fixed as well. > >>+ /* refers to rxq_cq_to_pkt_type() */ > >>+ if ((ptype_mask & RTE_PTYPE_L3_MASK) == RTE_PTYPE_L3_MASK) { > >>+ ptypes[num++] = RTE_PTYPE_L3_IPV4; > >>+ ptypes[num++] = RTE_PTYPE_L3_IPV6; > >>+ } > >>+ > >>+ if ((ptype_mask & RTE_PTYPE_INNER_L3_MASK) == RTE_PTYPE_INNER_L3_MASK) { > >>+ ptypes[num++] = RTE_PTYPE_INNER_L3_IPV4; > >>+ ptypes[num++] = RTE_PTYPE_INNER_L3_IPV6; > >>+ } > >>+ } else > >>+ num = -ENOTSUP; > >>+ > >>+ return num; > >>+} > >I think checking for mlx4_rx_burst and mlx4_rx_burst_sp is unnecessary at > >the moment, all RX burst functions do update the packet_type field, no need > >for extra complexity. > > > >Same comment for mlx5. > > Hi Mazarguil, > > My original thought is that rx_pkt_burst could be also set as > removed_rx_burst, which does not make sense indeed > because it's only possible when the device is closed. Yes, indeed. > Another consideration is to keep same style with other devices. Each > kind of device could have several rx burst functions. > So current implementation can keep extensibility to add new rx burst > functions. How do you think of it? OK, that makes sense. Please check my above comments about coding style/indents (I know I'm annoying). > >>+ > >> /** > >> * DPDK callback to get device statistics. > >> * > >>@@ -4989,6 +5015,7 @@ static const struct eth_dev_ops mlx4_dev_ops = { > >> .stats_reset = mlx4_stats_reset, > >> .queue_stats_mapping_set = NULL, > >> .dev_infos_get = mlx4_dev_infos_get, > >>+ .dev_ptypes_info_get = mlx4_dev_ptype_info_get, > >> .vlan_filter_set = mlx4_vlan_filter_set, > >> .vlan_tpid_set = NULL, > >> .vlan_strip_queue_set = NULL, > >>-- > >>2.1.4 > >> > -- Adrien Mazarguil 6WIND