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: Thu, 17 Sep 2015 08:55:11 +0300 Message-ID: <55FA55BF.6000005@samsung.com> References: <1441808537-28739-1-git-send-email-i.maximets@samsung.com> <55F931CB.4030903@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Cc: Dyasly Sergey To: "Wiles, Keith" , "dev@dpdk.org" Return-path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by dpdk.org (Postfix) with ESMTP id 46F542A1A for ; Thu, 17 Sep 2015 07:55:14 +0200 (CEST) Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NUT00DU44G0E070@mailout4.w1.samsung.com> for dev@dpdk.org; Thu, 17 Sep 2015 06:55:12 +0100 (BST) In-reply-to: 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" Ok. Thank you. I'll wait. On 16.09.2015 18:37, Wiles, Keith wrote: > 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. >=20 > On 9/16/15, 2:09 AM, "Ilya Maximets" wrote: >=20 >> 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 heade= rs. >>> >>> 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 ) >>> =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 onO= ff) >>> } >>> 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_ad= dr); >>> } else >>> info->seq_pkt[SINGLE_PKT].ip_dst_addr =3D ntohl(ip->addr.ipv4.s_ad= dr); >>> - 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 * ma= c) >>> { >>> 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_I= Pv4; >>> 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_i= dx] : >>> &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 >=20 > =8B=20 > Regards, > ++Keith > Intel Corporation >=20 >=20 >=20 >=20