From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 62C3F379E for ; Wed, 16 Sep 2015 17:38:02 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 16 Sep 2015 08:38:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,540,1437462000"; d="scan'208";a="806493837" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by fmsmga002.fm.intel.com with ESMTP; 16 Sep 2015 08:37:56 -0700 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 16 Sep 2015 08:37:25 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by ORSMSX157.amr.corp.intel.com (10.22.240.23) with Microsoft SMTP Server (TLS) id 14.3.224.2; Wed, 16 Sep 2015 08:37:25 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.247]) by FMSMSX112.amr.corp.intel.com ([10.18.116.6]) with mapi id 14.03.0224.002; Wed, 16 Sep 2015 08:37:25 -0700 From: "Wiles, Keith" To: Ilya Maximets , "dev@dpdk.org" Thread-Topic: [Pktgen] [PATCH] pktgen_setup_packets: fix race for packet header Thread-Index: AQHQ6wr8YUPuXUhVNEy/CDq5JjuEuJ4/Xg2A///3BoA= Date: Wed, 16 Sep 2015 15:37:24 +0000 Message-ID: References: <1441808537-28739-1-git-send-email-i.maximets@samsung.com> <55F931CB.4030903@samsung.com> In-Reply-To: <55F931CB.4030903@samsung.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.34.228.36] Content-Type: text/plain; charset="Windows-1252" Content-ID: <2BC6BC76A5420840A50081F6044FAAEB@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: Dyasly Sergey Subject: Re: [dpdk-dev] [Pktgen] [PATCH] pktgen_setup_packets: fix race for packet header X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Sep 2015 15:38:03 -0000 Thanks the patch looks fine, but I have not had a lot of time to review it detail. I hope to get to it next week after I return back home. On 9/16/15, 2:09 AM, "Ilya Maximets" wrote: >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. >>=20 >> Fix that by making a local copy of info->seq_pkt and using it for >> constructing of packets. >>=20 >> 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(-) >>=20 >> 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 ) >> =20 >> rte_memcpy(&pkt->eth_dst_addr, &arp->sha, 6); >> for (i =3D 0; i < info->seqCnt; i++) >> - pktgen_packet_ctor(info, i, -1); >> + pktgen_packet_ctor(info, i, -1, NULL); >> } >> =20 >> // 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 =3D=3D 'i' ) >> info->seq_pkt[SINGLE_PKT].ethType =3D ETHER_TYPE_IPv4; >> =20 >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -1067,7 +1067,7 @@ pktgen_set_pkt_type(port_info_t * info, const >>char * type) >> (type[3] =3D=3D '6') ? ETHER_TYPE_IPv6 : >> /* TODO print error: unknown type */ ETHER_TYPE_IPv4; >> =20 >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -1112,7 +1112,7 @@ pktgen_set_vlanid(port_info_t * info, uint16_t >>vlanid) >> { >> info->vlanid =3D vlanid; >> info->seq_pkt[SINGLE_PKT].vlanid =3D info->vlanid; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -1157,7 +1157,7 @@ pktgen_set_mpls_entry(port_info_t * info, >>uint32_t mpls_entry) >> { >> info->mpls_entry =3D mpls_entry; >> info->seq_pkt[SINGLE_PKT].mpls_entry =3D info->mpls_entry; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -1204,7 +1204,7 @@ pktgen_set_qinqids(port_info_t * info, uint16_t >>outerid, uint16_t innerid) >> info->seq_pkt[SINGLE_PKT].qinq_outerid =3D info->qinq_outerid; >> info->qinq_innerid =3D innerid; >> info->seq_pkt[SINGLE_PKT].qinq_innerid =3D info->qinq_innerid; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -1272,7 +1272,7 @@ pktgen_set_gre_key(port_info_t * info, uint32_t >>gre_key) >> { >> info->gre_key =3D gre_key; >> info->seq_pkt[SINGLE_PKT].gre_key =3D info->gre_key; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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)); >> =20 >> =20 >> - pktgen_packet_ctor(info, seq, -1); >> + pktgen_packet_ctor(info, seq, -1, NULL); >> =20 >> pktgen.flags |=3D 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 =3D 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); >> } >> =20 >> @@ -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 =3D 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 =3D MAX_PKT_SIZE + FCS_SIZE; >> info->seq_pkt[SINGLE_PKT].pktSize =3D (size - FCS_SIZE); >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> pktgen_packet_rate(info); >> } >> =20 >> @@ -1667,7 +1667,7 @@ void pktgen_set_port_value(port_info_t * info, >>char type, uint32_t portValue) >> info->seq_pkt[SINGLE_PKT].dport =3D (uint16_t)portValue; >> else >> info->seq_pkt[SINGLE_PKT].sport =3D (uint16_t)portValue; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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 =3D ntohl(ip->addr.ipv4.s_addr)= ; >> } else >> info->seq_pkt[SINGLE_PKT].ip_dst_addr =3D ntohl(ip->addr.ipv4.s_addr)= ; >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -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); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -2111,7 +2111,7 @@ void pktgen_set_seq(port_info_t * info, uint32_t >>seqnum, >> type =3D '4'; >> pkt->ethType =3D (type =3D=3D '6')? ETHER_TYPE_IPv6 : ETHER_TYPE_IPv4= ; >> pkt->vlanid =3D vlanid; >> - pktgen_packet_ctor(info, seqnum, -1); >> + pktgen_packet_ctor(info, seqnum, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> @@ -2156,7 +2156,7 @@ void pktgen_compile_pkt(port_info_t * info, >>uint32_t seqnum, >> (type =3D=3D '6')? ETHER_TYPE_IPv6 : >> (type =3D=3D 'n')? ETHER_TYPE_VLAN : ETHER_TYPE_IPv4; >> pkt->vlanid =3D vlanid; >> - pktgen_packet_ctor(info, seqnum, -1); >> + pktgen_packet_ctor(info, seqnum, -1, NULL); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> 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 =3D *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); >> =20 >> m->pkt_len =3D 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) { >> */ >> =20 >> void >> -pktgen_packet_ctor(port_info_t * info, int32_t seq_idx, int32_t type) { >> - pkt_seq_t * pkt =3D &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 =3D (seq_pkt =3D=3D NULL) ? &info->seq_pkt[seq_idx]= : >>&seq_pkt[seq_idx]; >> struct ether_hdr * eth =3D (struct ether_hdr *)&pkt->hdr.eth; >> uint16_t tlen; >> =20 >> @@ -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 =3D NULL; >> + uint16_t seqIdx; >> + char buff[RTE_MEMZONE_NAMESIZE]; >> =20 >> pktgen_clr_q_flags(info, qid, CLEAR_FAST_ALLOC_FLAG); >> =20 >> if ( mp =3D=3D info->q[qid].pcap_mp ) >> return; >> =20 >> + snprintf(buff, sizeof(buff), "tmp_seq_pkt_%d_%d", info->pid, qid); >> + seq_pkt =3D (pkt_seq_t *)rte_zmalloc(buff, >> + (sizeof(pkt_seq_t) * NUM_TOTAL_PKTS), RTE_CACHE_LINE_SIZE); >> + if (seq_pkt =3D=3D 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 =3D info->seqIdx; >> + >> mm =3D NULL; >> pkt =3D NULL; >> =20 >> if ( mp =3D=3D info->q[qid].tx_mp ) >> - pkt =3D &info->seq_pkt[SINGLE_PKT]; >> + pkt =3D &seq_pkt[SINGLE_PKT]; >> else if ( mp =3D=3D info->q[qid].range_mp ) >> - pkt =3D &info->seq_pkt[RANGE_PKT]; >> + pkt =3D &seq_pkt[RANGE_PKT]; >> else if ( mp =3D=3D info->q[qid].seq_mp ) >> - pkt =3D &info->seq_pkt[info->seqIdx]; >> + pkt =3D &seq_pkt[seqIdx]; >> =20 >> // 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 =3D m; >> =20 >> if ( mp =3D=3D info->q[qid].tx_mp ) { >> - pktgen_packet_ctor(info, SINGLE_PKT, -1); >> + pktgen_packet_ctor(info, SINGLE_PKT, -1, seq_pkt); >> =20 >> rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t >>*)&pkt->hdr, MAX_PKT_SIZE); >> =20 >> @@ -848,16 +861,16 @@ pktgen_setup_packets(port_info_t * info, struct >>rte_mempool * mp, uint16_t qid) >> m->data_len =3D pkt->pktSize; >> } else if ( mp =3D=3D 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); >> =20 >> rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t >>*)&pkt->hdr, MAX_PKT_SIZE); >> =20 >> m->pkt_len =3D pkt->pktSize; >> m->data_len =3D pkt->pktSize; >> } else if ( mp =3D=3D info->q[qid].seq_mp ) { >> - pktgen_packet_ctor(info, info->seqIdx++, -1); >> - if ( unlikely(info->seqIdx >=3D info->seqCnt) ) >> - info->seqIdx =3D 0; >> + pktgen_packet_ctor(info, seqIdx++, -1, seq_pkt); >> + if ( unlikely(seqIdx >=3D info->seqCnt) ) >> + seqIdx =3D 0; >> =20 >> rte_memcpy((uint8_t *)m->buf_addr + m->data_off, (uint8_t >>*)&pkt->hdr, MAX_PKT_SIZE); >> =20 >> @@ -865,12 +878,14 @@ pktgen_setup_packets(port_info_t * info, struct >>rte_mempool * mp, uint16_t qid) >> m->data_len =3D pkt->pktSize; >> =20 >> // move to the next packet in the sequence. >> - pkt =3D &info->seq_pkt[info->seqIdx]; >> + pkt =3D &seq_pkt[seqIdx]; >> } >> } >> =20 >> if ( mm !=3D NULL ) >> rte_pktmbuf_free(mm); >> + >> + rte_free(seq_pkt); >> } >> =20 >> =20 >>/************************************************************************ >>**//** >> 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" >> =20 >> #define PKTGEN_VERSION "2.9.2" >> #define PKTGEN_APP_NAME "Pktgen" >> @@ -386,7 +386,7 @@ extern pktgen_t pktgen; >> =20 >> extern void pktgen_page_display(__attribute__((unused)) struct >>rte_timer *tim, __attribute__((unused)) void *arg); >> =20 >> -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); >> =20 >> 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) >> }; >> =20 >> =20 >> - 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"); >> =20 >> 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 =3D 102, // Subtract 4 for FCS >> }; >> =20 >> - pktgen_packet_ctor(&info, 0, 0); >> + pktgen_packet_ctor(&info, 0, 0, NULL); >> =20 >> - 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"); >> =20 >> note("... with outer Ethernet header"); >> cmp_mem_lit_incr(data, (0xdd, 0xdd, 0xdd, 0xdd, 0xdd, 0xdd), " ... >>with correct dest MAC"); >>=20 > =8B=20 Regards, ++Keith Intel Corporation