From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 84F071E34 for ; Fri, 1 Mar 2019 08:38:45 +0100 (CET) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us4.ppe-hosted.com (Proofpoint Essentials ESMTP Server) with ESMTPS id 87A464C0064; Fri, 1 Mar 2019 07:38:43 +0000 (UTC) Received: from [192.168.38.17] (91.220.146.112) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Fri, 1 Mar 2019 07:38:37 +0000 To: Pavan Nikhilesh Bhagavatula , "Jerin Jacob Kollanukkaran" , "wenzhuo.lu@intel.com" , "bernard.iremonger@intel.com" CC: "dev@dpdk.org" References: <20190228194128.14236-1-pbhagavatula@marvell.com> From: Andrew Rybchenko Message-ID: <46756fe9-ecc7-91eb-4c91-fea42f67a40c@solarflare.com> Date: Fri, 1 Mar 2019 10:38:33 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <20190228194128.14236-1-pbhagavatula@marvell.com> Content-Language: en-GB X-Originating-IP: [91.220.146.112] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24462.003 X-TM-AS-Result: No-17.916100-8.000000-10 X-TMASE-MatchedRID: 9d2LtCNB3NI6yy6RAAEPcya1MaKuob8PC/ExpXrHizz4JyR+b5tvoIxb /pIP8cBQ+FVAbsNxrCtH8EpG2MYinQi9LIdJLRICkVUMyVUKRZ4RWG8HyxfLI7lmMfLNiukamS0 Gw4UBelhc0PCrkvwMo5OtFMK5KxfkZWePH6wPorLN+qWlu2ZxaP+UEb65dgmQvJGeuZ1ZZew6cg wTFeDdf3g/QTJzb+ugyYEQHeSJb40diedB1vHs3k1Wvi92YKnOc3ewuwbSaG4N/sfWy+i4PAaTa lM8C773BlKDcqdqcXYXifW96NU4apF8y72N+Bo0f01qcJQDhV5CVgEuvT6//ucPw4H9Nz2i807t +VEWpeYgZIcq73TEpZJTzjwz+Gzox7hajJv6RUXIOn6NK8S1ayXdp9l6EkRZ52VTYrkmb1uCAEC xRbm5ZM4ffyU1s6hMt4yMn+rBDs7fW6mMWTtZdOQYBHVKqgDUfS0Ip2eEHny+qryzYw2E8LLn+0 Vm71LcMOYynlQYGG/fd+P6wwCt81KV6oMVdTepHKvZxX6cukHmG69YrBcSKBUTWVZ+QBy96KRMc u/Ns9puyshvvIKgTym2dtrn4XCecdnVGSom89OugWm0QirL5NXL6b+lDE68 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--17.916100-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24462.003 X-MDID: 1551425924-YESAk-GWcijY Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] app/testpmd: use mempool bulk get for txonly mode 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: Fri, 01 Mar 2019 07:38:45 -0000 On 2/28/19 10:42 PM, Pavan Nikhilesh Bhagavatula wrote: > From: Pavan Nikhilesh > > Use mempool bulk get ops to alloc burst of packets and process them > instead of calling pktalloc for every packet. > > Signed-off-by: Pavan Nikhilesh > --- > app/test-pmd/txonly.c | 139 +++++++++++++++++++++--------------------- > 1 file changed, 71 insertions(+), 68 deletions(-) > > diff --git a/app/test-pmd/txonly.c b/app/test-pmd/txonly.c > index 1f08b6ed3..eef8b3a45 100644 > --- a/app/test-pmd/txonly.c > +++ b/app/test-pmd/txonly.c > @@ -147,6 +147,61 @@ setup_pkt_udp_ip_headers(struct ipv4_hdr *ip_hdr, > ip_hdr->hdr_checksum = (uint16_t) ip_cksum; > } > > +static inline bool > +pkt_burst_prepare(struct rte_mbuf *pkt, struct rte_mempool *mbp, > + struct ether_hdr *eth_hdr, const uint16_t vlan_tci, > + const uint16_t vlan_tci_outer, const uint64_t ol_flags) > +{ > + uint32_t nb_segs, pkt_len = 0; > + struct rte_mbuf *pkt_seg; > + uint8_t i; > + > + if (unlikely(tx_pkt_split == TX_PKT_SPLIT_RND)) > + nb_segs = random() % tx_pkt_nb_segs + 1; > + else > + nb_segs = tx_pkt_nb_segs; > + > + rte_pktmbuf_reset_headroom(pkt); > + pkt->data_len = tx_pkt_seg_lengths[0]; > + pkt->ol_flags = ol_flags; > + pkt->vlan_tci = vlan_tci; > + pkt->vlan_tci_outer = vlan_tci_outer; > + pkt->l2_len = sizeof(struct ether_hdr); > + pkt->l3_len = sizeof(struct ipv4_hdr); > + > + pkt_seg = pkt; > + for (i = 1; i < nb_segs; i++) { > + pkt_seg->next = rte_mbuf_raw_alloc(mbp); Why is bulk allocation not used here? > + if (pkt_seg->next == NULL) { > + pkt->nb_segs = i; > + rte_pktmbuf_free(pkt); > + return false; > + } > + pkt_seg = pkt_seg->next; > + pkt_seg->data_len = tx_pkt_seg_lengths[i]; > + pkt_len += pkt_seg->data_len; > + } > + pkt_seg->next = NULL; /* Last segment of packet. */ > + /* > + * Copy headers in first packet segment(s). > + */ > + copy_buf_to_pkt(eth_hdr, sizeof(eth_hdr), pkt, 0); > + copy_buf_to_pkt(&pkt_ip_hdr, sizeof(pkt_ip_hdr), pkt, > + sizeof(struct ether_hdr)); > + copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt, > + sizeof(struct ether_hdr) + > + sizeof(struct ipv4_hdr)); > + > + /* > + * Complete first mbuf of packet and append it to the > + * burst of packets to be transmitted. > + */ > + pkt->nb_segs = nb_segs; > + pkt->pkt_len += pkt_len; > + > + return true; > +} > + > /* > * Transmit a burst of multi-segments packets. > */ > @@ -155,8 +210,6 @@ pkt_burst_transmit(struct fwd_stream *fs) > { > struct rte_mbuf *pkts_burst[MAX_PKT_BURST]; > struct rte_port *txp; > - struct rte_mbuf *pkt; > - struct rte_mbuf *pkt_seg; > struct rte_mempool *mbp; > struct ether_hdr eth_hdr; > uint16_t nb_tx; > @@ -164,14 +217,12 @@ pkt_burst_transmit(struct fwd_stream *fs) > uint16_t vlan_tci, vlan_tci_outer; > uint32_t retry; > uint64_t ol_flags = 0; > - uint8_t i; > uint64_t tx_offloads; > #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES > uint64_t start_tsc; > uint64_t end_tsc; > uint64_t core_cycles; > #endif > - uint32_t nb_segs, pkt_len; > > #ifdef RTE_TEST_PMD_RECORD_CORE_CYCLES > start_tsc = rte_rdtsc(); > @@ -188,72 +239,24 @@ pkt_burst_transmit(struct fwd_stream *fs) > ol_flags |= PKT_TX_QINQ_PKT; > if (tx_offloads & DEV_TX_OFFLOAD_MACSEC_INSERT) > ol_flags |= PKT_TX_MACSEC; > - for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) { > - pkt = rte_mbuf_raw_alloc(mbp); > - if (pkt == NULL) { > - nomore_mbuf: > - if (nb_pkt == 0) > - return; > - break; > - } > > - /* > - * Using raw alloc is good to improve performance, > - * but some consumers may use the headroom and so > - * decrement data_off. We need to make sure it is > - * reset to default value. > - */ > - rte_pktmbuf_reset_headroom(pkt); > - pkt->data_len = tx_pkt_seg_lengths[0]; > - pkt_seg = pkt; > - if (tx_pkt_split == TX_PKT_SPLIT_RND) > - nb_segs = random() % tx_pkt_nb_segs + 1; > - else > - nb_segs = tx_pkt_nb_segs; > - pkt_len = pkt->data_len; > - for (i = 1; i < nb_segs; i++) { > - pkt_seg->next = rte_mbuf_raw_alloc(mbp); > - if (pkt_seg->next == NULL) { > - pkt->nb_segs = i; > - rte_pktmbuf_free(pkt); > - goto nomore_mbuf; > - } > - pkt_seg = pkt_seg->next; > - pkt_seg->data_len = tx_pkt_seg_lengths[i]; > - pkt_len += pkt_seg->data_len; > - } > - pkt_seg->next = NULL; /* Last segment of packet. */ > - > - /* > - * Initialize Ethernet header. > - */ > - ether_addr_copy(&peer_eth_addrs[fs->peer_addr],ð_hdr.d_addr); > - ether_addr_copy(&ports[fs->tx_port].eth_addr, ð_hdr.s_addr); > - eth_hdr.ether_type = rte_cpu_to_be_16(ETHER_TYPE_IPv4); > - > - /* > - * Copy headers in first packet segment(s). > - */ > - copy_buf_to_pkt(ð_hdr, sizeof(eth_hdr), pkt, 0); > - copy_buf_to_pkt(&pkt_ip_hdr, sizeof(pkt_ip_hdr), pkt, > - sizeof(struct ether_hdr)); > - copy_buf_to_pkt(&pkt_udp_hdr, sizeof(pkt_udp_hdr), pkt, > - sizeof(struct ether_hdr) + > - sizeof(struct ipv4_hdr)); > - > - /* > - * Complete first mbuf of packet and append it to the > - * burst of packets to be transmitted. > - */ > - pkt->nb_segs = nb_segs; > - pkt->pkt_len = pkt_len; > - pkt->ol_flags = ol_flags; > - pkt->vlan_tci = vlan_tci; > - pkt->vlan_tci_outer = vlan_tci_outer; > - pkt->l2_len = sizeof(struct ether_hdr); > - pkt->l3_len = sizeof(struct ipv4_hdr); > - pkts_burst[nb_pkt] = pkt; > + /* > + * Initialize Ethernet header. > + */ > + ether_addr_copy(&peer_eth_addrs[fs->peer_addr], ð_hdr.d_addr); > + ether_addr_copy(&ports[fs->tx_port].eth_addr, ð_hdr.s_addr); > + eth_hdr.ether_type = rte_cpu_to_be_16(ETHER_TYPE_IPv4); > + > + if (rte_mempool_get_bulk(mbp, (void **)pkts_burst, nb_pkt_per_burst)) > + return; Before the patch the code survived insufficient of mbufs condition and sent as much as it can allocate. Now it is not. I can't say for sure if the new behaviour is acceptable or not (I'd say no), but even if it is acceptable it should be highlighted in the changeset description. Taking segments allocation into account may I suggest to consider a bit sophisticated implementation which allocates packets in bulks with fallback to individual mbufs allocation and usage of the mechanism for all segments (i.e. allocate bulk, use it, allocate next, use it, etc). > + > + for (nb_pkt = 0; nb_pkt < nb_pkt_per_burst; nb_pkt++) { > + if (unlikely(!pkt_burst_prepare(pkts_burst[nb_pkt], mbp, > + ð_hdr, vlan_tci, vlan_tci_outer, ol_flags))) > + goto tx_pkts; If segment allocation fails, who frees remaining packets from the bulk? > } > +tx_pkts: > + > nb_tx = rte_eth_tx_burst(fs->tx_port, fs->tx_queue, pkts_burst, nb_pkt); > /* > * Retry if necessary