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 9D11FA00E6 for ; Tue, 19 Mar 2019 17:14:27 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C7A6F1B19; Tue, 19 Mar 2019 17:14:25 +0100 (CET) Received: from mail-pf1-f196.google.com (mail-pf1-f196.google.com [209.85.210.196]) by dpdk.org (Postfix) with ESMTP id EE63311A4 for ; Tue, 19 Mar 2019 17:14:23 +0100 (CET) Received: by mail-pf1-f196.google.com with SMTP id v21so14022342pfm.12 for ; Tue, 19 Mar 2019 09:14:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=qzgQXw6dtQ5k+loW4nJu4oxEMb30VMDbPF0fY1eum/M=; b=OqhM+/Dmg1Yw7DB3Ymi7FRQ0paUxPlOiLvR3RDVfTXbYrwggelzChzCSlnRoPdIdCR rqaJoBtZ0EmUp9aZxNG25eeYMShYMi/XNsZNABjbJowlBcMA0JNLjYQV36QwYjpqeo3p 4/PGyxZNRwr7Xo77epj35BybvpxNF6c2I314ck8H16u84x7UjhevrOQDOZOZ9oxeyR3O wdTCkqaiWt4hxJ4/wazUX2Ldvz11mO+FvwmOUBVOU59rDt2llloTCWJAv/VzSOhPvQ9M EbMvmNQF+4/Ru/d7mUS1pfs63HALURTijHhiy/6fnkrV0cby/ycocUAMDIy6+S6V1Rsz ZbFg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=qzgQXw6dtQ5k+loW4nJu4oxEMb30VMDbPF0fY1eum/M=; b=cyiXBrxoNkuDRH21cVz/78t+ES/YV2u3ozyh0LlJ9nMGX+ddo5nidskjPTMw+vvTfu xivDYKrUFnadw7sa32FgpUc/9ICAgYpywSyK78jZQGEMlECztrVbKjklex7U300vKMPd hjxsx6Nwa4GQqa5AqTv/wovxpl5yDmHdF8hcq3EHnA37HIK5niPD76ytZs0Q+LRx/twP If8dTVsivA/OtnS9H0+svIrCMziqSkinUX2ypLsUZZ1TS1fyoM93tf689/Hvpuv1gpHV 3r7RzHHo3AT3uPp0PRyLBzGROJqPIvt6A4Q90VLUczC5ymWuPPIxF0kRLQEx8EZYLKOL hiGA== X-Gm-Message-State: APjAAAUcXwgXdp+FpTv5/SY+RKzjHHHsjTuqnvDpN+0NdgOQkmC5nyvO hm7pFAXl+d/uWc4hYhdLHlPnag== X-Google-Smtp-Source: APXvYqzczlE/H0WoiQMDZR+XyL80KIfC6Ah1CCcSprbq5wOu+eag/hwsB5V8lxnXHwVj22x3jJLvUg== X-Received: by 2002:a17:902:6942:: with SMTP id k2mr2869045plt.136.1553012062877; Tue, 19 Mar 2019 09:14:22 -0700 (PDT) Received: from shemminger-XPS-13-9360 (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id v9sm22498559pfg.130.2019.03.19.09.14.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 19 Mar 2019 09:14:22 -0700 (PDT) Date: Tue, 19 Mar 2019 09:14:17 -0700 From: Stephen Hemminger To: Xiaolong Ye Cc: dev@dpdk.org, Qi Zhang , Karlsson Magnus , Topel Bjorn Message-ID: <20190319091417.61554ec2@shemminger-XPS-13-9360> In-Reply-To: <20190319071256.26302-2-xiaolong.ye@intel.com> References: <20190301080947.91086-1-xiaolong.ye@intel.com> <20190319071256.26302-1-xiaolong.ye@intel.com> <20190319071256.26302-2-xiaolong.ye@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit 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: <20190319161417.y19ZK4xWZlXiTrOkIrfaja2RucFHBFmxQ6xea6wv3rs@z> Lots of little review comments. This is what I saw in 30 minutes. Expect more. 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() ? > + mbuf = rte_pktmbuf_alloc(rxq->mb_pool); > + if (mbuf) { > + memcpy(rte_pktmbuf_mtod(mbuf, void*), pkt, len); Space necessary in "void*" Use rte_memcpy. > +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. > + > +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. > + 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. > + > +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. > + 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. > +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? > +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 > + 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? > + 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 > +/** 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) { > + > +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) { > +{ > + 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. > + int ret; > + int i; > + > + internals = rte_zmalloc_socket(name, sizeof(*internals), 0, numa_node);