From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilya Maximets Subject: Re: [Pktgen] [PATCH] pktgen_setup_packets: fix race for packet header Date: Wed, 16 Sep 2015 12:09:31 +0300 Message-ID: <55F931CB.4030903@samsung.com> References: <1441808537-28739-1-git-send-email-i.maximets@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: Dyasly Sergey To: dev@dpdk.org, Keith Wiles Return-path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 0FA99376D for ; Wed, 16 Sep 2015 11:09:46 +0200 (CEST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NUR00EQNIS8FS30@mailout2.w1.samsung.com> for dev@dpdk.org; Wed, 16 Sep 2015 10:09:44 +0100 (BST) In-reply-to: <1441808537-28739-1-git-send-email-i.maximets@samsung.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" Ping. On 09.09.2015 17:22, Ilya Maximets wrote: > While pktgen_setup_packets() all threads of one port uses same > info->seq_pkt. This leads to constructing packets in the same memory region > (&pkt->hdr). As a result, pktgen_setup_packets generates random headers. > > Fix that by making a local copy of info->seq_pkt and using it for > constructing of packets. > > Signed-off-by: Ilya Maximets > --- > app/pktgen-arp.c | 2 +- > app/pktgen-cmds.c | 40 ++++++++++++++++++++-------------------- > app/pktgen-ipv4.c | 2 +- > app/pktgen.c | 39 +++++++++++++++++++++++++++------------ > app/pktgen.h | 4 ++-- > app/t/pktgen.t.c | 6 +++--- > 6 files changed, 54 insertions(+), 39 deletions(-) > > diff --git a/app/pktgen-arp.c b/app/pktgen-arp.c > index c378880..b7040d7 100644 > --- a/app/pktgen-arp.c > +++ b/app/pktgen-arp.c > @@ -190,7 +190,7 @@ pktgen_process_arp( struct rte_mbuf * m, uint32_t pid, uint32_t vlan ) > > rte_memcpy(&pkt->eth_dst_addr, &arp->sha, 6); > for (i = 0; i < info->seqCnt; i++) > - pktgen_packet_ctor(info, i, -1); > + pktgen_packet_ctor(info, i, -1, NULL); > } > > // Swap the two MAC addresses > diff --git a/app/pktgen-cmds.c b/app/pktgen-cmds.c > index da040e5..a6abb41 100644 > --- a/app/pktgen-cmds.c > +++ b/app/pktgen-cmds.c > @@ -931,7 +931,7 @@ pktgen_set_proto(port_info_t * info, char type) > if ( type == 'i' ) > info->seq_pkt[SINGLE_PKT].ethType = ETHER_TYPE_IPv4; > > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1067,7 +1067,7 @@ pktgen_set_pkt_type(port_info_t * info, const char * type) > (type[3] == '6') ? ETHER_TYPE_IPv6 : > /* TODO print error: unknown type */ ETHER_TYPE_IPv4; > > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1092,7 +1092,7 @@ pktgen_set_vlan(port_info_t * info, uint32_t onOff) > } > else > pktgen_clr_port_flags(info, SEND_VLAN_ID); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1112,7 +1112,7 @@ pktgen_set_vlanid(port_info_t * info, uint16_t vlanid) > { > info->vlanid = vlanid; > info->seq_pkt[SINGLE_PKT].vlanid = info->vlanid; > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1137,7 +1137,7 @@ pktgen_set_mpls(port_info_t * info, uint32_t onOff) > } > else > pktgen_clr_port_flags(info, SEND_MPLS_LABEL); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1157,7 +1157,7 @@ pktgen_set_mpls_entry(port_info_t * info, uint32_t mpls_entry) > { > info->mpls_entry = mpls_entry; > info->seq_pkt[SINGLE_PKT].mpls_entry = info->mpls_entry; > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1182,7 +1182,7 @@ pktgen_set_qinq(port_info_t * info, uint32_t onOff) > } > else > pktgen_clr_port_flags(info, SEND_Q_IN_Q_IDS); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1204,7 +1204,7 @@ pktgen_set_qinqids(port_info_t * info, uint16_t outerid, uint16_t innerid) > info->seq_pkt[SINGLE_PKT].qinq_outerid = info->qinq_outerid; > info->qinq_innerid = innerid; > info->seq_pkt[SINGLE_PKT].qinq_innerid = info->qinq_innerid; > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1228,7 +1228,7 @@ pktgen_set_gre(port_info_t * info, uint32_t onOff) > } > else > pktgen_clr_port_flags(info, SEND_GRE_IPv4_HEADER); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1252,7 +1252,7 @@ pktgen_set_gre_eth(port_info_t * info, uint32_t onOff) > } > else > pktgen_clr_port_flags(info, SEND_GRE_ETHER_HEADER); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1272,7 +1272,7 @@ pktgen_set_gre_key(port_info_t * info, uint32_t gre_key) > { > info->gre_key = gre_key; > info->seq_pkt[SINGLE_PKT].gre_key = info->gre_key; > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1401,7 +1401,7 @@ void pktgen_port_defaults(uint32_t pid, uint8_t seq) > memset(&pkt->eth_dst_addr, 0, sizeof (pkt->eth_dst_addr)); > > > - pktgen_packet_ctor(info, seq, -1); > + pktgen_packet_ctor(info, seq, -1, NULL); > > pktgen.flags |= PRINT_LABELS_FLAG; > } > @@ -1423,7 +1423,7 @@ void pktgen_ping4(port_info_t * info) > memcpy(&info->seq_pkt[PING_PKT], > &info->seq_pkt[SINGLE_PKT], sizeof(pkt_seq_t)); > info->seq_pkt[PING_PKT].ipProto = PG_IPPROTO_ICMP; > - pktgen_packet_ctor(info, PING_PKT, ICMP4_ECHO); > + pktgen_packet_ctor(info, PING_PKT, ICMP4_ECHO, NULL); > pktgen_set_port_flags(info, SEND_PING4_REQUEST); > } > > @@ -1445,7 +1445,7 @@ void pktgen_ping6(port_info_t * info) > memcpy(&info->pkt[PING_PKT], > &info->pkt[SINGLE_PKT], sizeof(pkt_seq_t)); > info->pkt[PING_PKT].ipProto = PG_IPPROTO_ICMP; > - pktgen_packet_ctor(info, PING_PKT, ICMP6_ECHO); > + pktgen_packet_ctor(info, PING_PKT, ICMP6_ECHO, NULL); > pktgen_set_port_flags(info, SEND_PING6_REQUEST); > } > #endif > @@ -1645,7 +1645,7 @@ void pktgen_set_pkt_size(port_info_t * info, uint32_t size) > else if ( (size - FCS_SIZE) > MAX_PKT_SIZE) > size = MAX_PKT_SIZE + FCS_SIZE; > info->seq_pkt[SINGLE_PKT].pktSize = (size - FCS_SIZE); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > pktgen_packet_rate(info); > } > > @@ -1667,7 +1667,7 @@ void pktgen_set_port_value(port_info_t * info, char type, uint32_t portValue) > info->seq_pkt[SINGLE_PKT].dport = (uint16_t)portValue; > else > info->seq_pkt[SINGLE_PKT].sport = (uint16_t)portValue; > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1711,7 +1711,7 @@ void pktgen_set_ipaddr(port_info_t * info, char type, cmdline_ipaddr_t * ip) > info->seq_pkt[SINGLE_PKT].ip_src_addr = ntohl(ip->addr.ipv4.s_addr); > } else > info->seq_pkt[SINGLE_PKT].ip_dst_addr = ntohl(ip->addr.ipv4.s_addr); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -1729,7 +1729,7 @@ void pktgen_set_ipaddr(port_info_t * info, char type, cmdline_ipaddr_t * ip) > void pktgen_set_dst_mac(port_info_t * info, cmdline_etheraddr_t * mac) > { > memcpy(&info->seq_pkt[SINGLE_PKT].eth_dst_addr, mac->mac, 6); > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); > } > > /**************************************************************************//** > @@ -2111,7 +2111,7 @@ void pktgen_set_seq(port_info_t * info, uint32_t seqnum, > type = '4'; > pkt->ethType = (type == '6')? ETHER_TYPE_IPv6 : ETHER_TYPE_IPv4; > pkt->vlanid = vlanid; > - pktgen_packet_ctor(info, seqnum, -1); > + pktgen_packet_ctor(info, seqnum, -1, NULL); > } > > /**************************************************************************//** > @@ -2156,7 +2156,7 @@ void pktgen_compile_pkt(port_info_t * info, uint32_t seqnum, > (type == '6')? ETHER_TYPE_IPv6 : > (type == 'n')? ETHER_TYPE_VLAN : ETHER_TYPE_IPv4; > pkt->vlanid = vlanid; > - pktgen_packet_ctor(info, seqnum, -1); > + pktgen_packet_ctor(info, seqnum, -1, NULL); > } > > /**************************************************************************//** > diff --git a/app/pktgen-ipv4.c b/app/pktgen-ipv4.c > index 7813c36..a8b6be2 100644 > --- a/app/pktgen-ipv4.c > +++ b/app/pktgen-ipv4.c > @@ -138,7 +138,7 @@ pktgen_send_ping4( uint32_t pid, uint8_t seq_idx ) > return; > } > *ppkt = *spkt; // Copy the sequence setup to the ping setup. > - pktgen_packet_ctor(info, PING_PKT, ICMP4_ECHO); > + pktgen_packet_ctor(info, PING_PKT, ICMP4_ECHO, NULL); > rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t *)&ppkt->hdr, ppkt->pktSize); > > m->pkt_len = ppkt->pktSize; > diff --git a/app/pktgen.c b/app/pktgen.c > index 2dcdbfd..ecd4560 100644 > --- a/app/pktgen.c > +++ b/app/pktgen.c > @@ -401,8 +401,8 @@ static __inline__ int pktgen_has_work(void) { > */ > > void > -pktgen_packet_ctor(port_info_t * info, int32_t seq_idx, int32_t type) { > - pkt_seq_t * pkt = &info->seq_pkt[seq_idx]; > +pktgen_packet_ctor(port_info_t * info, int32_t seq_idx, int32_t type, pkt_seq_t * seq_pkt) { > + pkt_seq_t * pkt = (seq_pkt == NULL) ? &info->seq_pkt[seq_idx] : &seq_pkt[seq_idx]; > struct ether_hdr * eth = (struct ether_hdr *)&pkt->hdr.eth; > uint16_t tlen; > > @@ -812,22 +812,35 @@ static __inline__ void > pktgen_setup_packets(port_info_t * info, struct rte_mempool * mp, uint16_t qid) > { > struct rte_mbuf * m, * mm; > - pkt_seq_t * pkt; > + pkt_seq_t * pkt, * seq_pkt = NULL; > + uint16_t seqIdx; > + char buff[RTE_MEMZONE_NAMESIZE]; > > pktgen_clr_q_flags(info, qid, CLEAR_FAST_ALLOC_FLAG); > > if ( mp == info->q[qid].pcap_mp ) > return; > > + snprintf(buff, sizeof(buff), "tmp_seq_pkt_%d_%d", info->pid, qid); > + seq_pkt = (pkt_seq_t *)rte_zmalloc(buff, > + (sizeof(pkt_seq_t) * NUM_TOTAL_PKTS), RTE_CACHE_LINE_SIZE); > + if (seq_pkt == NULL) > + pktgen_log_panic("Unable to allocate %d pkt_seq_t headers", > + NUM_TOTAL_PKTS); > + /* Copy global configuration to construct new packets locally */ > + rte_memcpy((uint8_t *)seq_pkt, (uint8_t *)info->seq_pkt, > + sizeof(pkt_seq_t) * NUM_TOTAL_PKTS); > + seqIdx = info->seqIdx; > + > mm = NULL; > pkt = NULL; > > if ( mp == info->q[qid].tx_mp ) > - pkt = &info->seq_pkt[SINGLE_PKT]; > + pkt = &seq_pkt[SINGLE_PKT]; > else if ( mp == info->q[qid].range_mp ) > - pkt = &info->seq_pkt[RANGE_PKT]; > + pkt = &seq_pkt[RANGE_PKT]; > else if ( mp == info->q[qid].seq_mp ) > - pkt = &info->seq_pkt[info->seqIdx]; > + pkt = &seq_pkt[seqIdx]; > > // allocate each mbuf and put them on a list to be freed. > for(;;) { > @@ -840,7 +853,7 @@ pktgen_setup_packets(port_info_t * info, struct rte_mempool * mp, uint16_t qid) > mm = m; > > if ( mp == info->q[qid].tx_mp ) { > - pktgen_packet_ctor(info, SINGLE_PKT, -1); > + pktgen_packet_ctor(info, SINGLE_PKT, -1, seq_pkt); > > rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t *)&pkt->hdr, MAX_PKT_SIZE); > > @@ -848,16 +861,16 @@ pktgen_setup_packets(port_info_t * info, struct rte_mempool * mp, uint16_t qid) > m->data_len = pkt->pktSize; > } else if ( mp == info->q[qid].range_mp ) { > pktgen_range_ctor(&info->range, pkt); > - pktgen_packet_ctor(info, RANGE_PKT, -1); > + pktgen_packet_ctor(info, RANGE_PKT, -1, seq_pkt); > > rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t *)&pkt->hdr, MAX_PKT_SIZE); > > m->pkt_len = pkt->pktSize; > m->data_len = pkt->pktSize; > } else if ( mp == info->q[qid].seq_mp ) { > - pktgen_packet_ctor(info, info->seqIdx++, -1); > - if ( unlikely(info->seqIdx >= info->seqCnt) ) > - info->seqIdx = 0; > + pktgen_packet_ctor(info, seqIdx++, -1, seq_pkt); > + if ( unlikely(seqIdx >= info->seqCnt) ) > + seqIdx = 0; > > rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t *)&pkt->hdr, MAX_PKT_SIZE); > > @@ -865,12 +878,14 @@ pktgen_setup_packets(port_info_t * info, struct rte_mempool * mp, uint16_t qid) > m->data_len = pkt->pktSize; > > // move to the next packet in the sequence. > - pkt = &info->seq_pkt[info->seqIdx]; > + pkt = &seq_pkt[seqIdx]; > } > } > > if ( mm != NULL ) > rte_pktmbuf_free(mm); > + > + rte_free(seq_pkt); > } > > /**************************************************************************//** > diff --git a/app/pktgen.h b/app/pktgen.h > index 2c5c98c..f3f76f2 100644 > --- a/app/pktgen.h > +++ b/app/pktgen.h > @@ -151,7 +151,7 @@ > #include "pktgen-port-cfg.h" > #include "pktgen-capture.h" > #include "pktgen-log.h" > - > +#include "pktgen-seq.h" > > #define PKTGEN_VERSION "2.9.2" > #define PKTGEN_APP_NAME "Pktgen" > @@ -386,7 +386,7 @@ extern pktgen_t pktgen; > > extern void pktgen_page_display(__attribute__((unused)) struct rte_timer *tim, __attribute__((unused)) void *arg); > > -extern void pktgen_packet_ctor(port_info_t * info, int32_t seq_idx, int32_t type); > +extern void pktgen_packet_ctor(port_info_t * info, int32_t seq_idx, int32_t type, pkt_seq_t * seq_pkt); > extern void pktgen_packet_rate(port_info_t * info); > > extern void pktgen_send_mbuf(struct rte_mbuf *m, uint8_t pid, uint16_t qid); > diff --git a/app/t/pktgen.t.c b/app/t/pktgen.t.c > index 0eca4c4..77bf300 100644 > --- a/app/t/pktgen.t.c > +++ b/app/t/pktgen.t.c > @@ -77,7 +77,7 @@ void test_pktgen_packet_ctor_IPv4_UDP(void) > }; > > > - lives_ok( { pktgen_packet_ctor(&info, 0, 0); }, "pktgen_packet_ctor must generate IPv4/UDP"); > + lives_ok( { pktgen_packet_ctor(&info, 0, 0, NULL); }, "pktgen_packet_ctor must generate IPv4/UDP"); > > note("... with Ethernet header"); > cmp_mem_lit_incr(data, (0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd), " ... with correct dest MAC"); > @@ -133,9 +133,9 @@ void test_pktgen_packet_ctor_IPv4_GRE_Ether(void) > .pktSize = 102, // Subtract 4 for FCS > }; > > - pktgen_packet_ctor(&info, 0, 0); > + pktgen_packet_ctor(&info, 0, 0, NULL); > > - lives_ok( { pktgen_packet_ctor(&info, 0, 0); }, "pktgen_packet_ctor must generate IPv4 GRE Ethernet frame"); > + lives_ok( { pktgen_packet_ctor(&info, 0, 0, NULL); }, "pktgen_packet_ctor must generate IPv4 GRE Ethernet frame"); > > note("... with outer Ethernet header"); > cmp_mem_lit_incr(data, (0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd), " ... with correct dest MAC"); >