From: "Wiles, Keith" <keith.wiles@intel.com>
To: Ilya Maximets <i.maximets@samsung.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Dyasly Sergey <s.dyasly@samsung.com>
Subject: Re: [Pktgen] [PATCH] pktgen_setup_packets: fix race for packet header
Date: Wed, 16 Sep 2015 15:37:24 +0000 [thread overview]
Message-ID: <D21EDA83.2A795%keith.wiles@intel.com> (raw)
In-Reply-To: <55F931CB.4030903@samsung.com>
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" <i.maximets@samsung.com> 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.
>>
>> Fix that by making a local copy of info->seq_pkt and using it for
>> constructing of packets.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> 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");
>>
>
‹
Regards,
++Keith
Intel Corporation
next prev parent reply other threads:[~2015-09-16 15:38 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-09-09 14:22 [Pktgen] [PATCH] pktgen_setup_packets: fix race for packet header Ilya Maximets
2015-09-16 9:09 ` Ilya Maximets
2015-09-16 15:37 ` Wiles, Keith [this message]
2015-09-17 5:55 ` Ilya Maximets
2015-10-02 9:23 ` Ilya Maximets
2015-10-02 11:25 ` Wiles, Keith
2015-10-12 14:09 ` Ilya Maximets
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D21EDA83.2A795%keith.wiles@intel.com \
--to=keith.wiles@intel.com \
--cc=dev@dpdk.org \
--cc=i.maximets@samsung.com \
--cc=s.dyasly@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.