Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support

From: Thomas Monjalon <thomas@monjalon.net>
To: "Suanming. Mou" <mousuanming@huawei.com>
Cc: dev@dpdk.org, vipin.varghese@intel.com, anatoly.burakov@intel.com
Subject: Re: [dpdk-dev] [PATCH v8] app/pdump: add pudmp exits with primary support
Date: Wed, 08 May 2019 10:04:03 +0200	[thread overview]
Message-ID: <1677555.97jmHZBzRH@xps> (raw)
In-Reply-To: <1556862508-61677-1-git-send-email-mousuanming@huawei.com>

Hi,

I try to suggest some rewording below.

03/05/2019 07:48, Suanming. Mou:
> +/* Enough to set it to 500ms for exiting. */
> +#define MONITOR_INTERVAL (500 * 1000)

What is "enough"?
What is "it"?
What is the relation between MONITOR_INTERVAL and exiting?

[...]
> +	/*
> +	 * Don't worry about it is primary exit case. The alarm cancel
> +	 * function will take care about that. Ignore the ENOENT case.
> +	 */

I don't understand the comment.
Maybe you can explicit what is "it" and "that".
It would be probably simpler if you just describe why you cancel
this alarm.
How is it related to ENOENT?

> +	ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> +	if (ret < 0)
> +		printf("Fail to disable monitor:%d\n", ret);

[...]
> -	/* create mempool, ring and vdevs info */
> +	/* create mempool, ring, vdevs info and primary monitor */

I don't see any value to this comment.
You may just drop it.

>  	create_mp_ring_vdev();
>  	enable_pdump();
> +	enable_primary_monitor();