From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id A0F7B5A13 for ; Mon, 28 Dec 2015 02:35:39 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 27 Dec 2015 17:35:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,488,1444719600"; d="scan'208";a="870342808" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga001.fm.intel.com with ESMTP; 27 Dec 2015 17:35:17 -0800 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Dec 2015 17:35:16 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Dec 2015 17:35:16 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.57]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.220]) with mapi id 14.03.0248.002; Mon, 28 Dec 2015 09:35:15 +0800 From: "Wang, Zhihong" To: Stephen Hemminger Thread-Topic: [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd Thread-Index: AQHRPvhTw4yJKXUw+EecDuueGKSgNZ7e3MEAgADBQoA= Date: Mon, 28 Dec 2015 01:35:13 +0000 Message-ID: <8F6C2BD409508844A0EFC19955BE09418610B0@SHSMSX103.ccr.corp.intel.com> References: <1451011032-83106-1-git-send-email-zhihong.wang@intel.com> <1451011032-83106-3-git-send-email-zhihong.wang@intel.com> <20151227134904.3fd9ecce@xeon-e3> In-Reply-To: <20151227134904.3fd9ecce@xeon-e3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDgyMDI2ZjAtZTNlMy00MWFiLWI1MmEtMzE5Y2NlOGU1NGJlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjQuMTAuMTkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiNGZcL0I0NVwvcDhMZ3h0WFhGUWFZeXhrRkJwcHZ5WDVxalpsanY4QVNSRFZNPSJ9 x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and SIGTERM in l2fwd 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: Mon, 28 Dec 2015 01:35:40 -0000 Hi Stephen, Really appreciate the detailed review! Please see comments below. > > +static int force_quit =3D -1; > > +static int signo_quit =3D -1; >=20 > These need to be volatile otherwise you risk compiler optimizing away you= r > checks. Yes. Don't wanna take chances here. >=20 > Also, don't use -1/0 just use 0/1 for boolean or better yet the definitio= n in > of bool and true/false. > That way the code can read much nicer. -1 when forwarding not started yet. Can add a "static bool fwd_started;" to represent this to make it clearer. >=20 > > #define RTE_LOGTYPE_L2FWD RTE_LOGTYPE_USER1 > > > > #define NB_MBUF 8192 > > @@ -284,6 +289,8 @@ l2fwd_main_loop(void) > > } > > > > while (1) { > > + if (unlikely(force_quit !=3D 0)) > > + break; >=20 > Please maske this a proper while loop instead. Exactly. >=20 > while (!force_quit) { >=20 > > > > cur_tsc =3D rte_rdtsc(); > > > > @@ -534,6 +541,45 @@ check_all_ports_link_status(uint8_t port_num, > uint32_t port_mask) > > } > > } > > > > +static void > > +stop_ports(void) > > +{ > > + unsigned portid, nb_ports; > > + > > + nb_ports =3D rte_eth_dev_count(); > > + for (portid =3D 0; portid < nb_ports; portid++) { > > + if ((l2fwd_enabled_port_mask & (1 << portid)) =3D=3D 0) { > > + continue; > > + } >=20 > No need for {} here. >=20 > > + printf("Stopping port %d...", portid); > > + rte_eth_dev_stop(portid); > > + rte_eth_dev_close(portid); > > + printf(" Done\n"); > > + } > > +} > > + > > +static void > > +signal_handler(__rte_unused int signum) { > > + if (signum =3D=3D SIGINT || signum =3D=3D SIGTERM) { >=20 > signum is used, dont give __rte_unused attribute. >=20 > > > > /* launch per-lcore init on every lcore */ > > + force_quit =3D 0; >=20 > What is gained by having tri-value here. Just initialize it as false. As stated above. >=20 >=20 > > rte_eal_mp_remote_launch(l2fwd_launch_one_lcore, NULL, > CALL_MASTER); > > RTE_LCORE_FOREACH_SLAVE(lcore_id) { > > if (rte_eal_wait_lcore(lcore_id) < 0) > > return -1; > > } > > > > + printf("Stopping forwarding... Done\n"); > > + /* stop ports */ > > + stop_ports(); > > + printf("Bye...\n"); > > + /* inform if there's a caller */ > > + if (force_quit !=3D 0) { > > + signal(signo_quit, SIG_DFL); > > + kill(getpid(), signo_quit); >=20 > The kill should not be needed. The purpose is to make the program exit with the killed status. >=20 > It would be good if examples cleaned up allocations, that way they could = be used > with valgrind for validation of drivers, etc.