From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 849B9A04B2; Sat, 2 May 2020 20:43:36 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0C70B1D5FC; Sat, 2 May 2020 20:43:36 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id 825311D5F8 for ; Sat, 2 May 2020 20:43:34 +0200 (CEST) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 042IgJpH014399; Sat, 2 May 2020 11:43:33 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pfpt0818; bh=RXB+R4xNTtjMpX8KEGMNcmCzqcetDFS+3JdFDyTrseE=; b=Il6t0GxAm+iXmvucAbvh4e63vUBFBeKX5cHH/acAwkoEi0aBbW8TtsszPVaKI+xntvJE A7iOZkuOf2fo1oHbjEt9gRTHfaKn7DrE833uItTNewGr+FZ2YCpE2kRb6TiKnLaGoBBe /YJlQoxoVWoQi20ForLYYEM5wq6qIuu9zV/18ajwBs7ML0C/9PL5KDQRUQECVEdrY7P9 TrysX36pRiX6N8wYjkH/y49hx3HipmOpiiOK/Ejzrq6H0J5LwArUlBQsVa0DFGOZUi5h cgXmFUY9mcynCbSfMS9yIm05L1H3ZCc/j0BIak2eNIhWg/A8RG11A8LFGGsv0tcc83OU 6g== Received: from sc-exch01.marvell.com ([199.233.58.181]) by mx0a-0016f401.pphosted.com with ESMTP id 30s67q196n-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Sat, 02 May 2020 11:43:33 -0700 Received: from DC5-EXCH01.marvell.com (10.69.176.38) by SC-EXCH01.marvell.com (10.93.176.81) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Sat, 2 May 2020 11:43:31 -0700 Received: from SC-EXCH03.marvell.com (10.93.176.83) by DC5-EXCH01.marvell.com (10.69.176.38) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Sat, 2 May 2020 11:43:31 -0700 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (104.47.70.107) by SC-EXCH03.marvell.com (10.93.176.83) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Sat, 2 May 2020 11:43:30 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VMAcT4k8qwR6nXo+vew5qEW6tbEEPFQLXZlPvBvdhotupAz03uE/wP5l6gkRZHaS+5Q85jp21AUdyUWWjjncfxTXJ64gQthUQYS3Ht2RqgxLZTIhbs9nsuPZkw4bCK5b+ioeo0vBFeEHNHHNRDyIw9OJuLAOHruUeHmgni6b/8IA2j8vBOI8FZHip3xcP1bAD4G3/hTl/oR3n5K5nkk55Mj/+grhGPK8TsykRXlP9/THeIwagUbPXAlTEyeodrAkcTB7SPv16VAnHxouVGpGLLatfA7C4hdzRUoej42z2J1u8Jr09BvNWefVY+LiCGUwWZIcWxdEaTfHtMTFuwqCoA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RXB+R4xNTtjMpX8KEGMNcmCzqcetDFS+3JdFDyTrseE=; b=FWyHGXSOaKt6uaw/JD45eLadnDSz6z74RAKiH+TTz8q2zVYP3TNgSN2/bmyPpxWDIOPX1eJu82ly5m3UHzHWiPK1wemYSGHBFO6/X3+tBQf02JZcDu/nSPZE40LmubrHa+7KTZdMWXPeh/NddlVlk/V3BEdZJEDG+KB3Jsrk/V3LsDZAGUNpRJpLDZJTeibxY8U+2l9Ym+wfbB7/FBJzF05+LiuFHNx3DFBjwj/WH07HlRwUcdjkAQrdR49K7uzEcsCiqJxtIOjwGjRnJFLeOzGVTWnbyf5Z0V1NKskGF2hgFzHFNgVKvFpOCEzGY3jQWjWTvPwSqroEDeR/efWzPg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RXB+R4xNTtjMpX8KEGMNcmCzqcetDFS+3JdFDyTrseE=; b=AISQSRWMufg9BKevQJ6v7vCiI9RJMwj/OyIWMc+wM1Sa0VMzMqizv/t8RaFtaxvIjo8QHHWk4mwpfZEvNpEV3RgLwMTLG8sERAeIGiM4siFFPK6+k/6J8ApRiSwaZyYW4shG6DICxfvXIWNkV7ODSDuGO4kSj9M4VwkP3djei0o= Received: from MN2PR18MB2877.namprd18.prod.outlook.com (2603:10b6:208:3b::26) by MN2PR18MB3772.namprd18.prod.outlook.com (2603:10b6:208:211::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2937.22; Sat, 2 May 2020 18:43:28 +0000 Received: from MN2PR18MB2877.namprd18.prod.outlook.com ([fe80::15a6:7cce:c7ce:b74f]) by MN2PR18MB2877.namprd18.prod.outlook.com ([fe80::15a6:7cce:c7ce:b74f%7]) with mapi id 15.20.2958.027; Sat, 2 May 2020 18:43:28 +0000 From: Anoob Joseph To: "Ananyev, Konstantin" , Akhil Goyal , "Nicolau, Radu" CC: Narayana Prasad Raju Athreya , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: add per core packet stats Thread-Index: AQHWGim/k/TPRjHFIUi7O2VPB1TlgaiVC31g Date: Sat, 2 May 2020 18:43:28 +0000 Message-ID: References: <1587444633-17996-1-git-send-email-anoobj@marvell.com> <1587647245-10524-1-git-send-email-anoobj@marvell.com> In-Reply-To: Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=marvell.com; x-originating-ip: [111.125.205.80] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 4e26b501-ecfa-4901-2b08-08d7eec8b48b x-ms-traffictypediagnostic: MN2PR18MB3772: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4502; x-forefront-prvs: 039178EF4A x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MN2PR18MB2877.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(376002)(366004)(136003)(39850400004)(396003)(346002)(30864003)(5660300002)(2906002)(53546011)(66476007)(76116006)(66446008)(66946007)(66556008)(64756008)(86362001)(26005)(8936002)(6506007)(55236004)(186003)(478600001)(71200400001)(316002)(52536014)(7696005)(54906003)(110136005)(33656002)(55016002)(4326008)(9686003); DIR:OUT; SFP:1101; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: VLdTLKNXZwhWwy/Fh1E8SSWmrR4r77B7mijnMRlQkPUVQ57iYd+s1Hsl5vWEYHqJx7nJIZjj8bPCdwOM/I/um2OnBw+sHdYhCRDccmvvwB1FzasCjJWtbAJ+rgjz0FsubW18bw6Pw4sIBU6aRPGLEcjRA2tXTVJBVhrLMaF4EgbBqeeVTbtrZPTg3pK8NPvUGODli4HmSodt6/+T6oxMXN0h/+Ossu+fZiJiwY0CxwIBUy9T8I3acTpYQNf6rfrtKKbfBBJZRV2uP+RDnI3Nd0bGtPmlHgncKl8vyxoDg2G9eU4VqNU0K0eK3z/7u6CINToPPtUphV0iC+4KE0fPIWP4uRwaKj7u6pmkhnatf0b3yQxZvOr4WWMgqhgnxnOJ16Y2PVp5ZNloizB9m/+6dikVNbVRIRXfMRGfzW5gQuFd9KVSCbU31ULbHEAo0TYh x-ms-exchange-antispam-messagedata: bGhyvu8LzjzpetiL66TUlBJYBVneCevJEoE8gZpkXpGdtvE3XaXvAbkYSOWvIVRCLQULCV9T4hAhF+dU9l91iDV8L4H/uOkSgBjMRA05fudJA/mNJFvKOvZIhF9lgqlfBZIp/tiXqoGxDn4+2doWG8GUswGR2yaR9YexEsUn64Zi6t4lXaWvbogQUdOh1o2kqreiM6rD3eTmjhCBm1t6IvNX6W8h/BWqmhpz8e6pyuz5ExmIiHuopY7iymOzhwEobIGLPAZSUAo034K6oTz8ARmikiRJ2A3oV3LRHT6l/G27rXx1Oigl/oq4IDLolnRAxtROM9tQsuwEhTElBAH+SUhT0JdsU2GTiDQdaQGUM10trj/ta/QU945quXXUBC6/Ayr8HmKazfZY398MkZgy41298G4MD0Yvuzzts/pOSJ+14clhpmjnlJ2C6SpE9/f5ws6ltaXZ65aR//wrb6JChN+Tq1JgdBm6+Aw7F/zUtrxo7pHRMcTj6Vanyhdt6lBHZGpZ35eotDxnBoPw+3yhttxPGNPy/8vRAyFlveDnNZz4S1cMCNxBWSJI//AdBKTWKB1mXYmi7Q9jnJKH95zheVRlS75jdKZC97QC98Ve2JBZwiR36OnSOdld9amaWBoNfPjaj/fPWR+IyQw8GaWKg0H/xKiFY4CoPsT5nmGcJgH+rEvNevUymBIZFuG6ErVGa/6j2XSoqCIICby1sFgiVQAc7edT8aUAKHB5Iur7JugaSk4xZstGbflPWutJ4pYdrEWgt+BctqYNeSKP6FLW2QDyd6n4CjY1DSVnW2d2RGo= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 4e26b501-ecfa-4901-2b08-08d7eec8b48b X-MS-Exchange-CrossTenant-originalarrivaltime: 02 May 2020 18:43:28.4804 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ECMm8moLLshfnQZQ26dSCvRP2NcDX2txaH6Qf/cSB7UVfZoBKel68Ive1BEr3zelllAdNM0ZULrKiDWIwJECwQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR18MB3772 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.676 definitions=2020-05-02_10:2020-05-01, 2020-05-02 signatures=0 Subject: Re: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: add per core packet stats 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, Thanks for the review. Please see inline. @Akhil, do you have any comments? Thanks, Anoob > -----Original Message----- > From: Ananyev, Konstantin > Sent: Friday, April 24, 2020 4:44 PM > To: Anoob Joseph ; Akhil Goyal ; > Nicolau, Radu > Cc: Narayana Prasad Raju Athreya ; dev@dpdk.org > Subject: [EXT] RE: [dpdk-dev] [PATCH v2] examples/ipsec-secgw: add per co= re > packet stats >=20 > External Email >=20 > ---------------------------------------------------------------------- >=20 > > Adding per core packet handling stats to analyze traffic distribution > > when multiple cores are engaged. > > > > Since aggregating the packet stats across cores would affect > > performance, keeping the feature disabled using compile time flags. > > > > Signed-off-by: Anoob Joseph > > --- > > > > v2: > > * Added lookup failure cases to drop count > > > > examples/ipsec-secgw/ipsec-secgw.c | 118 > +++++++++++++++++++++++++++++++++-- > > examples/ipsec-secgw/ipsec-secgw.h | 2 + > > examples/ipsec-secgw/ipsec.c | 13 +++- > > examples/ipsec-secgw/ipsec.h | 22 +++++++ > > examples/ipsec-secgw/ipsec_process.c | 5 ++ > > 5 files changed, 154 insertions(+), 6 deletions(-) > > > > diff --git a/examples/ipsec-secgw/ipsec-secgw.c > > b/examples/ipsec-secgw/ipsec-secgw.c > > index 6d02341..db92ddc 100644 > > --- a/examples/ipsec-secgw/ipsec-secgw.c > > +++ b/examples/ipsec-secgw/ipsec-secgw.c > > @@ -288,6 +288,61 @@ adjust_ipv6_pktlen(struct rte_mbuf *m, const struc= t > rte_ipv6_hdr *iph, > > } > > } > > > > +#ifdef ENABLE_STATS > > +static uint64_t timer_period =3D 10; /* default period is 10 seconds *= / >=20 > I think it is better to add user ability to control stats period. > Either runtime-option, or just compile time: > replace ENABLE_STATS with STATS_PERIOD (0 would mean stats disabled). [Anoob] That's a good suggestion. Will make this change as part of v3. =20 >=20 > > + > > +/* Print out statistics on packet distribution */ static void > > +print_stats(void) > > +{ > > + uint64_t total_packets_dropped, total_packets_tx, total_packets_rx; > > + unsigned int coreid; > > + float burst_percent; > > + > > + total_packets_dropped =3D 0; > > + total_packets_tx =3D 0; > > + total_packets_rx =3D 0; > > + > > + const char clr[] =3D { 27, '[', '2', 'J', '\0' }; > > + const char topLeft[] =3D { 27, '[', '1', ';', '1', 'H', '\0' }; > > + > > + /* Clear screen and move to top left */ > > + printf("%s%s", clr, topLeft); >=20 > Is that really needed? [Anoob] I had just copied from l2fwd and worked from that. But it seems lik= e, 'topleft' is not needed. Will remove that. Without a clearscreen, the pr= ints would just crowd the screen. I would like to retain clearscreen. =20 >=20 > > + > > + printf("\nCore statistics =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D"); > > + > > + for (coreid =3D 0; coreid < RTE_MAX_LCORE; coreid++) { > > + /* skip disabled cores */ > > + if (rte_lcore_is_enabled(coreid) =3D=3D 0) > > + continue; > > + burst_percent =3D (float)(core_statistics[coreid].burst_rx * 100)/ > > + core_statistics[coreid].rx; >=20 > Would float be always enough here? Might better long double? [Anoob] This is just to get the percentage of traffic doing bursts. Since i= t's percentage float seemed enough. Do you think we need double instead? I = can change if you think so. =20 >=20 > > + printf("\nStatistics for core %u ------------------------------" > > + "\nPackets received: %20"PRIu64 > > + "\nPackets sent: %24"PRIu64 > > + "\nPackets dropped: %21"PRIu64 > > + "\nBurst percent: %23.2f", > > + coreid, > > + core_statistics[coreid].rx, > > + core_statistics[coreid].tx, > > + core_statistics[coreid].dropped, > > + burst_percent); > > + > > + total_packets_dropped +=3D core_statistics[coreid].dropped; > > + total_packets_tx +=3D core_statistics[coreid].tx; > > + total_packets_rx +=3D core_statistics[coreid].rx; > > + } > > + printf("\nAggregate statistics =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D" > > + "\nTotal packets received: %14"PRIu64 > > + "\nTotal packets sent: %18"PRIu64 > > + "\nTotal packets dropped: %15"PRIu64, > > + total_packets_rx, > > + total_packets_tx, > > + total_packets_dropped); > > + > printf("\n=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D > =3D\n"); > > +} > > +#endif /* ENABLE_STATS */ > > + > > static inline void > > prepare_one_packet(struct rte_mbuf *pkt, struct ipsec_traffic *t) { > > @@ -333,6 +388,7 @@ prepare_one_packet(struct rte_mbuf *pkt, struct > > ipsec_traffic *t) > > > > /* drop packet when IPv6 header exceeds first segment length > */ > > if (unlikely(l3len > pkt->data_len)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkt); > > return; > > } > > @@ -350,6 +406,7 @@ prepare_one_packet(struct rte_mbuf *pkt, struct > ipsec_traffic *t) > > /* Unknown/Unsupported type, drop the packet */ > > RTE_LOG(ERR, IPSEC, "Unsupported packet type 0x%x\n", > > rte_be_to_cpu_16(eth->ether_type)); > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkt); > > return; > > } > > @@ -471,6 +528,11 @@ send_burst(struct lcore_conf *qconf, uint16_t n, > uint16_t port) > > int32_t ret; > > uint16_t queueid; > > > > +#ifdef ENABLE_STATS > > + int lcore_id =3D rte_lcore_id(); > > + core_statistics[lcore_id].tx +=3D n; > > +#endif /* ENABLE_STATS */ >=20 > Instead of polluting genric code with ifdefs, why not to introduce 2 new > functions: core_stats_update_rx(), core_stats_update_tx(), as you did for > core_stats_drop()? [Anoob] The drop would happen from multiple places while Rx & Tx would happ= en only from one place each. I wasn't sure which would be the right approac= h. If you agree to introduce rx, tx & drop stats functions, I will update s= o. >=20 > > + > > queueid =3D qconf->tx_queue_id[port]; > > m_table =3D (struct rte_mbuf **)qconf->tx_mbufs[port].m_table; > > > > @@ -478,6 +540,9 @@ send_burst(struct lcore_conf *qconf, uint16_t n, > > uint16_t port) > > > > ret =3D rte_eth_tx_burst(port, queueid, m_table, n); > > if (unlikely(ret < n)) { > > +#ifdef ENABLE_STATS > > + core_statistics[lcore_id].dropped +=3D n-ret; #endif /* > ENABLE_STATS > > +*/ >=20 > You have core_stats_update_drop() for that - use it. [Anoob] lcore_id is already available as local variable. Will add the call = instead.=20 =20 >=20 > > do { > > rte_pktmbuf_free(m_table[ret]); > > } while (++ret < n); > > @@ -525,6 +590,7 @@ send_fragment_packet(struct lcore_conf *qconf, > struct rte_mbuf *m, > > "error code: %d\n", > > __func__, m->pkt_len, rte_errno); > > > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > return len; > > } > > @@ -549,8 +615,10 @@ send_single_packet(struct rte_mbuf *m, uint16_t > port, uint8_t proto) > > /* need to fragment the packet */ > > } else if (frag_tbl_sz > 0) > > len =3D send_fragment_packet(qconf, m, port, proto); > > - else > > + else { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); >=20 > It looks like a lot of such places... > Would it be worth to unite core_stats_update_drop() and rte_pktmbuf_free(= m) > Into some inline function: ipsec_secgw_packet_drop(struct rte_mbuf *m[], > uint32_t n) and use it all over such places. [Anoob] Ack =20 >=20 > > + } > > > > /* enough pkts to be sent */ > > if (unlikely(len =3D=3D MAX_PKT_BURST)) { @@ -584,18 +652,21 @@ > > inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type= *ip, > > continue; > > } > > if (res =3D=3D DISCARD) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > continue; > > } > > > > /* Only check SPI match for processed IPSec packets */ > > if (i < lim && ((m->ol_flags & PKT_RX_SEC_OFFLOAD) =3D=3D 0)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > continue; > > } > > > > sa_idx =3D res - 1; > > if (!inbound_sa_check(sa, m, sa_idx)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > continue; > > } > > @@ -630,8 +701,10 @@ split46_traffic(struct ipsec_traffic *trf, struct > rte_mbuf *mb[], uint32_t num) > > uint8_t *, > > offsetof(struct ip6_hdr, ip6_nxt)); > > n6++; > > - } else > > + } else { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > + } > > } > > > > trf->ip4.num =3D n4; > > @@ -682,11 +755,12 @@ outbound_sp(struct sp_ctx *sp, struct traffic_typ= e > *ip, > > for (i =3D 0; i < ip->num; i++) { > > m =3D ip->pkts[i]; > > sa_idx =3D ip->res[i] - 1; > > - if (ip->res[i] =3D=3D DISCARD) > > + if (ip->res[i] =3D=3D DISCARD) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(m); > > - else if (ip->res[i] =3D=3D BYPASS) > > + } else if (ip->res[i] =3D=3D BYPASS) { >=20 > Looks unnecessary. >=20 > > ip->pkts[j++] =3D m; > > - else { > > + } else { > > ipsec->res[ipsec->num] =3D sa_idx; > > ipsec->pkts[ipsec->num++] =3D m; > > } > > @@ -705,6 +779,8 @@ process_pkts_outbound(struct ipsec_ctx *ipsec_ctx, > > for (i =3D 0; i < traffic->ipsec.num; i++) > > rte_pktmbuf_free(traffic->ipsec.pkts[i]); > > > > + core_stats_update_drop(traffic->ipsec.num); > > + > > traffic->ipsec.num =3D 0; > > > > outbound_sp(ipsec_ctx->sp4_ctx, &traffic->ip4, &traffic->ipsec); @@ > > -745,12 +821,14 @@ process_pkts_inbound_nosp(struct ipsec_ctx *ipsec_ct= x, > > /* Drop any IPv4 traffic from unprotected ports */ > > for (i =3D 0; i < traffic->ip4.num; i++) > > rte_pktmbuf_free(traffic->ip4.pkts[i]); > > + core_stats_update_drop(traffic->ip4.num); > > > > traffic->ip4.num =3D 0; > > > > /* Drop any IPv6 traffic from unprotected ports */ > > for (i =3D 0; i < traffic->ip6.num; i++) > > rte_pktmbuf_free(traffic->ip6.pkts[i]); > > + core_stats_update_drop(traffic->ip6.num); > > > > traffic->ip6.num =3D 0; > > > > @@ -788,6 +866,7 @@ process_pkts_outbound_nosp(struct ipsec_ctx > *ipsec_ctx, > > /* Drop any IPsec traffic from protected ports */ > > for (i =3D 0; i < traffic->ipsec.num; i++) > > rte_pktmbuf_free(traffic->ipsec.pkts[i]); > > + core_stats_update_drop(traffic->ipsec.num); > > > > n =3D 0; > > > > @@ -901,6 +980,7 @@ route4_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf > *pkts[], uint8_t nb_pkts) > > } > > > > if ((pkt_hop & RTE_LPM_LOOKUP_SUCCESS) =3D=3D 0) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -953,6 +1033,7 @@ route6_pkts(struct rt_ctx *rt_ctx, struct rte_mbuf > *pkts[], uint8_t nb_pkts) > > } > > > > if (pkt_hop =3D=3D -1) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -1099,6 +1180,9 @@ ipsec_poll_mode_worker(void) > > const uint64_t drain_tsc =3D (rte_get_tsc_hz() + US_PER_S - 1) > > / US_PER_S * BURST_TX_DRAIN_US; > > struct lcore_rx_queue *rxql; > > +#ifdef ENABLE_STATS > > + uint64_t timer_tsc =3D 0; > > +#endif /* ENABLE_STATS */ >=20 > Probably better just RTE_SET_USED(timer_tsc); [Anoob] Isn't it better to keep all the code related an optional feature un= der on flag? Either way is fine. =20 >=20 > > > > prev_tsc =3D 0; > > lcore_id =3D rte_lcore_id(); > > @@ -1159,6 +1243,19 @@ ipsec_poll_mode_worker(void) > > drain_tx_buffers(qconf); > > drain_crypto_buffers(qconf); > > prev_tsc =3D cur_tsc; > > +#ifdef ENABLE_STATS > > + if (lcore_id =3D=3D rte_get_master_lcore()) { > > + /* advance the timer */ > > + timer_tsc +=3D diff_tsc; > > + > > + /* if timer has reached its timeout */ > > + if (unlikely(timer_tsc >=3D timer_period)) { > > + print_stats(); > > + /* reset the timer */ > > + timer_tsc =3D 0; > > + } > > + } > > +#endif /* ENABLE_STATS */ >=20 >=20 > Why to do stats collection/display inside data-path? > Why not use rte_timer/rte_alarm and make it happen in control thread? > Another option - make stats printing at some signal to the process. > In that case we don't need to bother with time period at all - it will be= user to > decide. > Again if we remove that print_stats() from data-path it might become chea= p > enough to always collect it, and we will not need ENABLE_STATS macro at a= ll. [Anoob] The stats collection will not be cheap because we are updating a co= mmon structure from multiple cores. And the idea is not to have a very effi= cient stats mechanism. When we have more cores engaged, IPsec performance i= s directly dependent on ability of RSS to split traffic to multiple cores. = Such stats would come handy in understanding per core distribution of incom= ing traffic.=20 =20 >=20 > > } > > > > for (i =3D 0; i < qconf->nb_rx_queue; ++i) { @@ -1169,6 +1266,12 > @@ > > ipsec_poll_mode_worker(void) > > nb_rx =3D rte_eth_rx_burst(portid, queueid, > > pkts, MAX_PKT_BURST); > > > > +#ifdef ENABLE_STATS > > + core_statistics[lcore_id].rx +=3D nb_rx; > > + if (nb_rx =3D=3D MAX_PKT_BURST) > > + core_statistics[lcore_id].burst_rx +=3D nb_rx; > #endif /* > > +ENABLE_STATS */ > > + >=20 > Same for above for TX: no need to pollute the code with ifdefs. > Better to introduce new function: stats_update_rx() or so. [Anoob] Ack >=20 >=20 > > if (nb_rx > 0) > > process_pkts(qconf, pkts, nb_rx, portid); > > > > @@ -2747,6 +2850,11 @@ main(int32_t argc, char **argv) > > signal(SIGINT, signal_handler); > > signal(SIGTERM, signal_handler); > > > > +#ifdef ENABLE_STATS > > + /* convert to number of cycles */ > > + timer_period *=3D rte_get_timer_hz(); > > +#endif /* ENABLE_STATS */ > > + > > /* initialize event helper configuration */ > > eh_conf =3D eh_conf_init(); > > if (eh_conf =3D=3D NULL) > > diff --git a/examples/ipsec-secgw/ipsec-secgw.h > > b/examples/ipsec-secgw/ipsec-secgw.h > > index 4b53cb5..d886a35 100644 > > --- a/examples/ipsec-secgw/ipsec-secgw.h > > +++ b/examples/ipsec-secgw/ipsec-secgw.h > > @@ -6,6 +6,8 @@ > > > > #include > > > > +//#define ENABLE_STATS > > + >=20 > Should be removed I think. >=20 > > #define NB_SOCKETS 4 > > > > #define MAX_PKT_BURST 32 > > diff --git a/examples/ipsec-secgw/ipsec.c > > b/examples/ipsec-secgw/ipsec.c index bf88d80..dcb9312 100644 > > --- a/examples/ipsec-secgw/ipsec.c > > +++ b/examples/ipsec-secgw/ipsec.c > > @@ -499,8 +499,10 @@ enqueue_cop_burst(struct cdev_qp *cqp) > > " enqueued %u crypto ops out of %u\n", > > cqp->id, cqp->qp, ret, len); > > /* drop packets that we fail to enqueue */ > > - for (i =3D ret; i < len; i++) > > + for (i =3D ret; i < len; i++) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(cqp->buf[i]->sym->m_src); > > + } > > } > > cqp->in_flight +=3D ret; > > cqp->len =3D 0; > > @@ -528,6 +530,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > > ipsec_ctx *ipsec_ctx, > > > > for (i =3D 0; i < nb_pkts; i++) { > > if (unlikely(sas[i] =3D=3D NULL)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -549,6 +552,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > > ipsec_ctx *ipsec_ctx, > > > > if ((unlikely(ips->security.ses =3D=3D NULL)) && > > create_lookaside_session(ipsec_ctx, sa, ips)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -563,6 +567,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > ipsec_ctx *ipsec_ctx, > > case RTE_SECURITY_ACTION_TYPE_CPU_CRYPTO: > > RTE_LOG(ERR, IPSEC, "CPU crypto is not supported by > the" > > " legacy mode."); > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > > > @@ -575,6 +580,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > > ipsec_ctx *ipsec_ctx, > > > > if ((unlikely(ips->crypto.ses =3D=3D NULL)) && > > create_lookaside_session(ipsec_ctx, sa, ips)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -584,6 +590,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > > ipsec_ctx *ipsec_ctx, > > > > ret =3D xform_func(pkts[i], sa, &priv->cop); > > if (unlikely(ret)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -608,6 +615,7 @@ ipsec_enqueue(ipsec_xform_fn xform_func, struct > > ipsec_ctx *ipsec_ctx, > > > > ret =3D xform_func(pkts[i], sa, &priv->cop); > > if (unlikely(ret)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkts[i]); > > continue; > > } > > @@ -643,6 +651,7 @@ ipsec_inline_dequeue(ipsec_xform_fn xform_func, > struct ipsec_ctx *ipsec_ctx, > > sa =3D priv->sa; > > ret =3D xform_func(pkt, sa, &priv->cop); > > if (unlikely(ret)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkt); > > continue; > > } > > @@ -690,12 +699,14 @@ ipsec_dequeue(ipsec_xform_fn xform_func, struct > ipsec_ctx *ipsec_ctx, > > RTE_SECURITY_ACTION_TYPE_NONE) { > > ret =3D xform_func(pkt, sa, cops[j]); > > if (unlikely(ret)) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkt); > > continue; > > } > > } else if (ipsec_get_action_type(sa) =3D=3D > > > RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL) { > > if (cops[j]->status) { > > + core_stats_update_drop(1); > > rte_pktmbuf_free(pkt); > > continue; > > } > > diff --git a/examples/ipsec-secgw/ipsec.h > > b/examples/ipsec-secgw/ipsec.h index 1e642d1..8519eab 100644 > > --- a/examples/ipsec-secgw/ipsec.h > > +++ b/examples/ipsec-secgw/ipsec.h > > @@ -46,6 +46,17 @@ > > > > #define IP6_VERSION (6) > > > > +#ifdef ENABLE_STATS > > +struct ipsec_core_statistics { > > + uint64_t tx; > > + uint64_t rx; > > + uint64_t dropped; > > + uint64_t burst_rx; >=20 > A bit strange to have burst_rx and no similar counterpart for tx. > BTW, do you need burst_rx? > Might be better: > nb_calls_rx, nb_calls_tx; > and then: rx/nb_calls_rx will give you average burst size. [Anoob] After the packets are received in the app, further IPsec processing= is done, which is more efficient when done in batches. For example, SA loo= kup is better when done in a batch, as SIMD instructions would be used. Abi= lity of traffic to be in burst or ability of ethdev to give burst will have= an effect on the IPsec performance per core. Tx burst percentage isn't tha= t important, since it is the app which submits for Tx. Rx/nb_calls_rx would also give a similar kind of stat, but the difference i= s whether 1 packet cases and 31 packet cases are treated equal. I would pre= fer to keep it the current way, but what you suggested also would work. =20 >=20 > > +} __rte_cache_aligned; > > + > > +struct ipsec_core_statistics core_statistics[RTE_MAX_ETHPORTS]; >=20 > Should be RTE_MAX_LCORES, I think. [Anoob] Ack >=20 > > +#endif /* ENABLE_STATS */ > > + > > struct rte_crypto_xform; > > struct ipsec_xform; > > struct rte_mbuf; > > @@ -416,4 +427,15 @@ check_flow_params(uint16_t fdir_portid, uint8_t > > fdir_qid); int create_ipsec_esp_flow(struct ipsec_sa *sa); > > > > +static inline void > > +core_stats_update_drop(int n) > > +{ > > +#ifdef ENABLE_STATS > > + int lcore_id =3D rte_lcore_id(); > > + core_statistics[lcore_id].dropped +=3D n; #else > > + RTE_SET_USED(n); > > +#endif /* ENABLE_STATS */ > > +} > > + > > #endif /* __IPSEC_H__ */ > > diff --git a/examples/ipsec-secgw/ipsec_process.c > > b/examples/ipsec-secgw/ipsec_process.c > > index bb2f2b8..05cb3ad 100644 > > --- a/examples/ipsec-secgw/ipsec_process.c > > +++ b/examples/ipsec-secgw/ipsec_process.c > > @@ -24,6 +24,11 @@ free_pkts(struct rte_mbuf *mb[], uint32_t n) { > > uint32_t i; > > > > +#ifdef ENABLE_STATS > > + int lcore_id =3D rte_lcore_id(); > > + core_statistics[lcore_id].dropped +=3D n; #endif /* ENABLE_STATS */ > > + >=20 > Same as above - why not use stats_update_drop() here? >=20 > > for (i =3D 0; i !=3D n; i++) > > rte_pktmbuf_free(mb[i]); > > } > > -- > > 2.7.4