From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 8A99F11A4 for ; Wed, 20 Mar 2019 03:36:51 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Mar 2019 19:36:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,246,1549958400"; d="scan'208";a="308665696" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.110.206]) by orsmga005.jf.intel.com with ESMTP; 19 Mar 2019 19:36:48 -0700 Date: Wed, 20 Mar 2019 10:32:52 +0800 From: Ye Xiaolong To: Stephen Hemminger Cc: dev@dpdk.org, Qi Zhang , Karlsson Magnus , Topel Bjorn Message-ID: <20190320023252.GF16135@intel.com> References: <20190301080947.91086-1-xiaolong.ye@intel.com> <20190319071256.26302-1-xiaolong.ye@intel.com> <20190319071256.26302-2-xiaolong.ye@intel.com> <20190319091417.61554ec2@shemminger-XPS-13-9360> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190319091417.61554ec2@shemminger-XPS-13-9360> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v2 1/6] net/af_xdp: introduce AF XDP PMD driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Mar 2019 02:36:52 -0000 Hi, Stephen On 03/19, Stephen Hemminger wrote: >Lots of little review comments. This is what I saw in 30 minutes. >Expect more. Thanks for taking time to review my patch. They are all valuable inputs. > > >On Tue, 19 Mar 2019 15:12:51 +0800 >Xiaolong Ye wrote: > >> + nb_pkts = nb_pkts < ETH_AF_XDP_RX_BATCH_SIZE ? >> + nb_pkts : ETH_AF_XDP_RX_BATCH_SIZE; > >Maybe use RTE_MIN() ? Will do. > >> + mbuf = rte_pktmbuf_alloc(rxq->mb_pool); >> + if (mbuf) { >> + memcpy(rte_pktmbuf_mtod(mbuf, void*), pkt, len); > >Space necessary in "void*" >Use rte_memcpy. Will do. > >> +static void pull_umem_cq(struct xsk_umem_info *umem, int size) >> +{ >> + struct xsk_ring_cons *cq = &umem->cq; >> + int i, n; >> + uint32_t idx_cq; >> + uint64_t addr; >> + >> + n = xsk_ring_cons__peek(cq, size, &idx_cq); >> + if (n > 0) { >> + for (i = 0; i < n; i++) { > >You don't need the if (n > 0) since if n <= 0 the for loop >would happen 0 times. Will remove the redundant "if (n > 0)" > >> + >> +static void kick_tx(struct pkt_tx_queue *txq) >> +{ >> + struct xsk_umem_info *umem = txq->pair->umem; >> + int ret; >> + >> + while (1) { >> + ret = sendto(xsk_socket__fd(txq->pair->xsk), NULL, 0, >> + MSG_DONTWAIT, NULL, 0); >> + >> + /* everything is ok */ >> + if (ret >= 0) >> + break; > >I would prefer: > while ((send(xsk_socket__fd(fd, NULL, 0, MSG_DONTWAIT) < 0) { > >Because: > - use while() to make looping clearer rather than while(1) > - use send() rather than sendto() because you aren't sending with addr > - you don't care about return value (ie.ret) > >> + >> + /* some thing unexpected */ >> + if (errno != EBUSY && errno != EAGAIN) >> + break; > >What about EINTR > > > >> +static void >> +eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + >> + dev_info->if_index = internals->if_index; >> + dev_info->max_mac_addrs = 1; >> + dev_info->max_rx_pktlen = (uint32_t)ETH_FRAME_LEN; > >Cast here is unnecessary. Got it. > >> + dev_info->max_rx_queues = 1; >> + dev_info->max_tx_queues = 1; >> + dev_info->min_rx_bufsize = 0; > >dev_info is already zero, don't need to fill other values. Got it. > >> + >> +static void >> +eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + >> + dev_info->if_index = internals->if_index; >> + dev_info->max_mac_addrs = 1; >> + dev_info->max_rx_pktlen = (uint32_t)ETH_FRAME_LEN; >> + dev_info->max_rx_queues = 1; >> + dev_info->max_tx_queues = 1; >> + dev_info->min_rx_bufsize = 0; >> + >> + dev_info->default_rxportconf.nb_queues = 1; >> + dev_info->default_txportconf.nb_queues = 1; >> + dev_info->default_rxportconf.ring_size = ETH_AF_XDP_DFLT_NUM_DESCS; >> + dev_info->default_txportconf.ring_size = ETH_AF_XDP_DFLT_NUM_DESCS; >> +} >> + >> +static int >> +eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + struct xdp_statistics xdp_stats; >> + struct pkt_rx_queue *rxq; >> + socklen_t optlen; >> + int i; >> + >> + optlen = sizeof(struct xdp_statistics); > >In theory each call to getsockopt() could change or reduce the value of >optlen. Best to initialize in loop before each call. Got it. > >> + for (i = 0; i < dev->data->nb_rx_queues; i++) { >> + rxq = &internals->rx_queues[i]; >> + stats->q_ipackets[i] = internals->rx_queues[i].rx_pkts; >> + stats->q_ibytes[i] = internals->rx_queues[i].rx_bytes; >> + >> + stats->q_opackets[i] = internals->tx_queues[i].tx_pkts; >> + stats->q_obytes[i] = internals->tx_queues[i].tx_bytes; >> + >> + stats->ipackets += stats->q_ipackets[i]; >> + stats->ibytes += stats->q_ibytes[i]; >> + stats->imissed += internals->rx_queues[i].rx_dropped; >> + getsockopt(xsk_socket__fd(rxq->xsk), SOL_XDP, XDP_STATISTICS, >> + &xdp_stats, &optlen); > >You need to check return value of getsockopt() otherwise coverity will complain. Got it. > >> +static void >> +eth_stats_reset(struct rte_eth_dev *dev) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + int i; >> + >> + for (i = 0; i < ETH_AF_XDP_MAX_QUEUE_PAIRS; i++) { >> + internals->rx_queues[i].rx_pkts = 0; >> + internals->rx_queues[i].rx_bytes = 0; >> + internals->rx_queues[i].rx_dropped = 0; >> + >> + internals->tx_queues[i].tx_pkts = 0; >> + internals->tx_queues[i].err_pkts = 0; >> + internals->tx_queues[i].tx_bytes = 0; > >Put all the statistics together and use memset? Sounds good, will do. > >> +static struct xsk_umem_info *xdp_umem_configure(void) >> +{ >> + struct xsk_umem_info *umem; >> + struct xsk_umem_config usr_config = { >> + .fill_size = ETH_AF_XDP_DFLT_NUM_DESCS, >> + .comp_size = ETH_AF_XDP_DFLT_NUM_DESCS, >> + .frame_size = ETH_AF_XDP_FRAME_SIZE, >> + .frame_headroom = ETH_AF_XDP_DATA_HEADROOM }; >> + void *bufs = NULL; >> + char ring_name[0x100]; > >0x100 is unconvential here. Instead use RTE_RING_NAMESIZE >but variable is unnecessary, see below Got it. > >> + int ret; >> + uint64_t i; >> + >> + umem = calloc(1, sizeof(*umem)); > >Why not use rte_zmalloc_node to: > 1. work with primary/secondary > 2. guarantee memory on correct numa node? Will do. > >> + if (!umem) { >> + RTE_LOG(ERR, AF_XDP, "Failed to allocate umem info"); >> + return NULL; >> + } >> + >> + snprintf(ring_name, 0x100, "af_xdp_ring"); > >If ring is always the same, why copy it. Just use string literal Got it. > >> +/** parse name argument */ >> +static int >> +parse_name_arg(const char *key __rte_unused, >> + const char *value, void *extra_args) >> +{ >> + char *name = extra_args; >> + >> + if (strlen(value) > IFNAMSIZ) { > >Why not: > if (strnlen(value, IFNAMSIZ) >= IFNAMSIZ) { Will do. > >> + >> +static int >> +init_internals(struct rte_vdev_device *dev, >> + const char *if_name, >> + int queue_idx, >> + struct rte_eth_dev **eth_dev) > >If you changed the function to return the new eth_dev (and return NULL on error) >then you wouldn't have to pass eth_dev by reference. > >static struct rte_eth_dev * >allocate_ethdev(struct rte_vdev_device *dev, const char *if_name, uint16_t queue_idx) Sounds better, will do. >{ > >> +{ >> + const char *name = rte_vdev_device_name(dev); >> + const unsigned int numa_node = dev->device.numa_node; >> + struct pmd_internals *internals = NULL; > >Useless initialization, first thing you do is allocate this. Got it. Thanks, Xiaolong > > >> + int ret; >> + int i; >> + >> + internals = rte_zmalloc_socket(name, sizeof(*internals), 0, numa_node); > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id CE9EDA00E6 for ; Wed, 20 Mar 2019 03:36:54 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AD4311DBD; Wed, 20 Mar 2019 03:36:53 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 8A99F11A4 for ; Wed, 20 Mar 2019 03:36:51 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Mar 2019 19:36:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,246,1549958400"; d="scan'208";a="308665696" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.110.206]) by orsmga005.jf.intel.com with ESMTP; 19 Mar 2019 19:36:48 -0700 Date: Wed, 20 Mar 2019 10:32:52 +0800 From: Ye Xiaolong To: Stephen Hemminger Cc: dev@dpdk.org, Qi Zhang , Karlsson Magnus , Topel Bjorn Message-ID: <20190320023252.GF16135@intel.com> References: <20190301080947.91086-1-xiaolong.ye@intel.com> <20190319071256.26302-1-xiaolong.ye@intel.com> <20190319071256.26302-2-xiaolong.ye@intel.com> <20190319091417.61554ec2@shemminger-XPS-13-9360> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline In-Reply-To: <20190319091417.61554ec2@shemminger-XPS-13-9360> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v2 1/6] net/af_xdp: introduce AF XDP PMD driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190320023252.OBf4MriAivXDrf50XKXWlXCoVLJryJlVAGRU00cuwpI@z> Hi, Stephen On 03/19, Stephen Hemminger wrote: >Lots of little review comments. This is what I saw in 30 minutes. >Expect more. Thanks for taking time to review my patch. They are all valuable inputs. > > >On Tue, 19 Mar 2019 15:12:51 +0800 >Xiaolong Ye wrote: > >> + nb_pkts = nb_pkts < ETH_AF_XDP_RX_BATCH_SIZE ? >> + nb_pkts : ETH_AF_XDP_RX_BATCH_SIZE; > >Maybe use RTE_MIN() ? Will do. > >> + mbuf = rte_pktmbuf_alloc(rxq->mb_pool); >> + if (mbuf) { >> + memcpy(rte_pktmbuf_mtod(mbuf, void*), pkt, len); > >Space necessary in "void*" >Use rte_memcpy. Will do. > >> +static void pull_umem_cq(struct xsk_umem_info *umem, int size) >> +{ >> + struct xsk_ring_cons *cq = &umem->cq; >> + int i, n; >> + uint32_t idx_cq; >> + uint64_t addr; >> + >> + n = xsk_ring_cons__peek(cq, size, &idx_cq); >> + if (n > 0) { >> + for (i = 0; i < n; i++) { > >You don't need the if (n > 0) since if n <= 0 the for loop >would happen 0 times. Will remove the redundant "if (n > 0)" > >> + >> +static void kick_tx(struct pkt_tx_queue *txq) >> +{ >> + struct xsk_umem_info *umem = txq->pair->umem; >> + int ret; >> + >> + while (1) { >> + ret = sendto(xsk_socket__fd(txq->pair->xsk), NULL, 0, >> + MSG_DONTWAIT, NULL, 0); >> + >> + /* everything is ok */ >> + if (ret >= 0) >> + break; > >I would prefer: > while ((send(xsk_socket__fd(fd, NULL, 0, MSG_DONTWAIT) < 0) { > >Because: > - use while() to make looping clearer rather than while(1) > - use send() rather than sendto() because you aren't sending with addr > - you don't care about return value (ie.ret) > >> + >> + /* some thing unexpected */ >> + if (errno != EBUSY && errno != EAGAIN) >> + break; > >What about EINTR > > > >> +static void >> +eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + >> + dev_info->if_index = internals->if_index; >> + dev_info->max_mac_addrs = 1; >> + dev_info->max_rx_pktlen = (uint32_t)ETH_FRAME_LEN; > >Cast here is unnecessary. Got it. > >> + dev_info->max_rx_queues = 1; >> + dev_info->max_tx_queues = 1; >> + dev_info->min_rx_bufsize = 0; > >dev_info is already zero, don't need to fill other values. Got it. > >> + >> +static void >> +eth_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + >> + dev_info->if_index = internals->if_index; >> + dev_info->max_mac_addrs = 1; >> + dev_info->max_rx_pktlen = (uint32_t)ETH_FRAME_LEN; >> + dev_info->max_rx_queues = 1; >> + dev_info->max_tx_queues = 1; >> + dev_info->min_rx_bufsize = 0; >> + >> + dev_info->default_rxportconf.nb_queues = 1; >> + dev_info->default_txportconf.nb_queues = 1; >> + dev_info->default_rxportconf.ring_size = ETH_AF_XDP_DFLT_NUM_DESCS; >> + dev_info->default_txportconf.ring_size = ETH_AF_XDP_DFLT_NUM_DESCS; >> +} >> + >> +static int >> +eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + struct xdp_statistics xdp_stats; >> + struct pkt_rx_queue *rxq; >> + socklen_t optlen; >> + int i; >> + >> + optlen = sizeof(struct xdp_statistics); > >In theory each call to getsockopt() could change or reduce the value of >optlen. Best to initialize in loop before each call. Got it. > >> + for (i = 0; i < dev->data->nb_rx_queues; i++) { >> + rxq = &internals->rx_queues[i]; >> + stats->q_ipackets[i] = internals->rx_queues[i].rx_pkts; >> + stats->q_ibytes[i] = internals->rx_queues[i].rx_bytes; >> + >> + stats->q_opackets[i] = internals->tx_queues[i].tx_pkts; >> + stats->q_obytes[i] = internals->tx_queues[i].tx_bytes; >> + >> + stats->ipackets += stats->q_ipackets[i]; >> + stats->ibytes += stats->q_ibytes[i]; >> + stats->imissed += internals->rx_queues[i].rx_dropped; >> + getsockopt(xsk_socket__fd(rxq->xsk), SOL_XDP, XDP_STATISTICS, >> + &xdp_stats, &optlen); > >You need to check return value of getsockopt() otherwise coverity will complain. Got it. > >> +static void >> +eth_stats_reset(struct rte_eth_dev *dev) >> +{ >> + struct pmd_internals *internals = dev->data->dev_private; >> + int i; >> + >> + for (i = 0; i < ETH_AF_XDP_MAX_QUEUE_PAIRS; i++) { >> + internals->rx_queues[i].rx_pkts = 0; >> + internals->rx_queues[i].rx_bytes = 0; >> + internals->rx_queues[i].rx_dropped = 0; >> + >> + internals->tx_queues[i].tx_pkts = 0; >> + internals->tx_queues[i].err_pkts = 0; >> + internals->tx_queues[i].tx_bytes = 0; > >Put all the statistics together and use memset? Sounds good, will do. > >> +static struct xsk_umem_info *xdp_umem_configure(void) >> +{ >> + struct xsk_umem_info *umem; >> + struct xsk_umem_config usr_config = { >> + .fill_size = ETH_AF_XDP_DFLT_NUM_DESCS, >> + .comp_size = ETH_AF_XDP_DFLT_NUM_DESCS, >> + .frame_size = ETH_AF_XDP_FRAME_SIZE, >> + .frame_headroom = ETH_AF_XDP_DATA_HEADROOM }; >> + void *bufs = NULL; >> + char ring_name[0x100]; > >0x100 is unconvential here. Instead use RTE_RING_NAMESIZE >but variable is unnecessary, see below Got it. > >> + int ret; >> + uint64_t i; >> + >> + umem = calloc(1, sizeof(*umem)); > >Why not use rte_zmalloc_node to: > 1. work with primary/secondary > 2. guarantee memory on correct numa node? Will do. > >> + if (!umem) { >> + RTE_LOG(ERR, AF_XDP, "Failed to allocate umem info"); >> + return NULL; >> + } >> + >> + snprintf(ring_name, 0x100, "af_xdp_ring"); > >If ring is always the same, why copy it. Just use string literal Got it. > >> +/** parse name argument */ >> +static int >> +parse_name_arg(const char *key __rte_unused, >> + const char *value, void *extra_args) >> +{ >> + char *name = extra_args; >> + >> + if (strlen(value) > IFNAMSIZ) { > >Why not: > if (strnlen(value, IFNAMSIZ) >= IFNAMSIZ) { Will do. > >> + >> +static int >> +init_internals(struct rte_vdev_device *dev, >> + const char *if_name, >> + int queue_idx, >> + struct rte_eth_dev **eth_dev) > >If you changed the function to return the new eth_dev (and return NULL on error) >then you wouldn't have to pass eth_dev by reference. > >static struct rte_eth_dev * >allocate_ethdev(struct rte_vdev_device *dev, const char *if_name, uint16_t queue_idx) Sounds better, will do. >{ > >> +{ >> + const char *name = rte_vdev_device_name(dev); >> + const unsigned int numa_node = dev->device.numa_node; >> + struct pmd_internals *internals = NULL; > >Useless initialization, first thing you do is allocate this. Got it. Thanks, Xiaolong > > >> + int ret; >> + int i; >> + >> + internals = rte_zmalloc_socket(name, sizeof(*internals), 0, numa_node); >