From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Wang, Zhihong" <zhihong.wang@intel.com>
Subject: Re: [PATCH v2 2/3] examples/l2fwd: Handle SIGINT and
 SIGTERM in l2fwd
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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: Stephen Hemminger <stephen@networkplumber.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mga01.intel.com (mga01.intel.com [192.55.52.88])
 by dpdk.org (Postfix) with ESMTP id A0F7B5A13
 for <dev@dpdk.org>; Mon, 28 Dec 2015 02:35:39 +0100 (CET)
In-Reply-To: <20151227134904.3fd9ecce@xeon-e3>
Content-Language: en-US
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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
> <stdbool.h> 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.