[v5] app/pdump: add pudmp exits with primary support.
Checks
Commit Message
When primary app exits, the residual running pdump will stop the
primary app to restart. Add pdump exits with primary support.
Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
app/pdump/main.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
Comments
On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
> When primary app exits, the residual running pdump will stop the
> primary app to restart. Add pdump exits with primary support.
>
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> ---
<snip>
> static void
> +disable_primary_monitor(void)
> +{
> + int ret;
> +
> + /* Don't worry about it is primary exit case. The alarm cancel
> + * function will take care about that. */
> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> + if (ret < 0)
> + printf("Fail to disable monitor fail:%d\n", ret);
Double fail :)
> +}
> +
> +static void
> signal_handler(int sig_num)
> {
> if (sig_num == SIGINT) {
> @@ -910,6 +936,19 @@ struct parse_val {
> ;
> }
>
> +static void
> +enable_primary_monitor(void)
> +{
> + int ret;
> +
> + /* Once primary exits, so will pdump. */
> + ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> + if (ret < 0) {
> + cleanup_pdump_resources();
> + rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
> + }
Why is this function void, when you could've called rte_exit() in the
caller on failure? And why is it such a fatal error to set up the timer?
IMO just a warning would've been enough.
> +}
> +
> int
> main(int argc, char **argv)
> {
> @@ -950,11 +989,13 @@ struct parse_val {
> rte_exit(EXIT_FAILURE, "Invalid argument\n");
> }
>
> - /* create mempool, ring and vdevs info */
> + /* create mempool, ring, vdevs info and primary monitor */
> create_mp_ring_vdev();
> enable_pdump();
> + enable_primary_monitor();
> dump_packets();
>
> + disable_primary_monitor();
> cleanup_pdump_resources();
> /* dump debug stats */
> print_pdump_stats();
>
On 2019/4/30 17:42, Burakov, Anatoly wrote:
> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>> When primary app exits, the residual running pdump will stop the
>> primary app to restart. Add pdump exits with primary support.
>>
>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>> ---
>
> <snip>
>
>> static void
>> +disable_primary_monitor(void)
>> +{
>> + int ret;
>> +
>> + /* Don't worry about it is primary exit case. The alarm cancel
>> + * function will take care about that. */
>> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>> + if (ret < 0)
>> + printf("Fail to disable monitor fail:%d\n", ret);
>
> Double fail :)
Ah, yes, sorry for that the code gets worse. :(
>
>> +}
>> +
>> +static void
>> signal_handler(int sig_num)
>> {
>> if (sig_num == SIGINT) {
>> @@ -910,6 +936,19 @@ struct parse_val {
>> ;
>> }
>> +static void
>> +enable_primary_monitor(void)
>> +{
>> + int ret;
>> +
>> + /* Once primary exits, so will pdump. */
>> + ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>> + if (ret < 0) {
>> + cleanup_pdump_resources();
>> + rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>> + }
>
> Why is this function void, when you could've called rte_exit() in the
> caller on failure? And why is it such a fatal error to set up the
> timer? IMO just a warning would've been enough.
Here comes with two issues:
Q1. The return value of the function:
A1: I'm so sorry that it does not seem to make sense to check the
function's return value. Does it mean if we change the timer set up from
error to warning, then we can use the return value to judge if need to
disable the primary_monitor?
Q2. The choice when rte_eal_alarm_set fail:
A2: OK, agree with that.
>
>> +}
>> +
>> int
>> main(int argc, char **argv)
>> {
>> @@ -950,11 +989,13 @@ struct parse_val {
>> rte_exit(EXIT_FAILURE, "Invalid argument\n");
>> }
>> - /* create mempool, ring and vdevs info */
>> + /* create mempool, ring, vdevs info and primary monitor */
>> create_mp_ring_vdev();
>> enable_pdump();
>> + enable_primary_monitor();
>> dump_packets();
>> + disable_primary_monitor();
>> cleanup_pdump_resources();
>> /* dump debug stats */
>> print_pdump_stats();
>>
>
>
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
> Sent: Tuesday, April 30, 2019 12:35 PM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary
> support.
>
> When primary app exits, the residual running pdump will stop the primary app to
> restart. Add pdump exits with primary support.
>
> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
You need to run below scripts, and fix the issues reported before sending next version.
1)
Ex:
export DPDK_CHECKPATCH_PATH=/usr/src/kernels/4.18.9-200.fc28.x86_64/scripts/checkpatch.pl
[DPDK]# ./devtools/checkpatches.sh
### app/pdump: add pudmp exits with primary support.
WARNING:BLOCK_COMMENT_STYLE: Block comments use a trailing */ on a separate line
#62: FILE: app/pdump/main.c:559:
+ * function will take care about that. */
total: 0 errors, 1 warnings, 83 lines checked
0/1 valid patch
2)
[DPDK]# ./devtools/check-git-log.sh
Wrong headline format:
app/pdump: add pudmp exits with primary support.
Need to remove full stop at the end.
Wrong tag:
Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
Might need to remove ,
> @@ -26,6 +26,7 @@
> #include <rte_ring.h>
> #include <rte_string_fns.h>
> #include <rte_pdump.h>
> +#include <rte_alarm.h>
>
Nitpick:
As per the coding guidelines(https://doc.dpdk.org/guides/contributing/coding_style.html)
the order of the headers is below, so you can include rte_alarm.h after <rte_eal.h>
libc includes (system includes first)
DPDK EAL includes
DPDK misc libraries includes
application-specific includes
> +#define MONITOR_INTERVAL (500 * 1000)
Adding comment on how many ms/us would be helpful.
<snipped>
> + /* Don't worry about it is primary exit case. The alarm cancel
> + * function will take care about that. */
Multiline comments should be like below.
/*
* Multi-line comments look like this. Make them real sentences. Fill
* them so they look like real paragraphs.
*/
Source: https://doc.dpdk.org/guides/contributing/coding_style.html
Thanks,
Reshma
On 30-Apr-19 12:25 PM, Suanming.Mou wrote:
>
> On 2019/4/30 17:42, Burakov, Anatoly wrote:
>> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>>> When primary app exits, the residual running pdump will stop the
>>> primary app to restart. Add pdump exits with primary support.
>>>
>>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>> ---
>>
>> <snip>
>>
>>> static void
>>> +disable_primary_monitor(void)
>>> +{
>>> + int ret;
>>> +
>>> + /* Don't worry about it is primary exit case. The alarm cancel
>>> + * function will take care about that. */
>>> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>> + if (ret < 0)
>>> + printf("Fail to disable monitor fail:%d\n", ret);
>>
>> Double fail :)
> Ah, yes, sorry for that the code gets worse. :(
>>
>>> +}
>>> +
>>> +static void
>>> signal_handler(int sig_num)
>>> {
>>> if (sig_num == SIGINT) {
>>> @@ -910,6 +936,19 @@ struct parse_val {
>>> ;
>>> }
>>> +static void
>>> +enable_primary_monitor(void)
>>> +{
>>> + int ret;
>>> +
>>> + /* Once primary exits, so will pdump. */
>>> + ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>>> + if (ret < 0) {
>>> + cleanup_pdump_resources();
>>> + rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>>> + }
>>
>> Why is this function void, when you could've called rte_exit() in the
>> caller on failure? And why is it such a fatal error to set up the
>> timer? IMO just a warning would've been enough.
>
> Here comes with two issues:
>
> Q1. The return value of the function:
>
> A1: I'm so sorry that it does not seem to make sense to check the
> function's return value. Does it mean if we change the timer set up from
> error to warning, then we can use the return value to judge if need to
> disable the primary_monitor?
>
> Q2. The choice when rte_eal_alarm_set fail:
>
> A2: OK, agree with that.
If this is non-fatal, no need to change anything - just print out a
warning instead of rte_exit, and no more changes needed here.
>
>>
>>> +}
>>> +
>>> int
>>> main(int argc, char **argv)
>>> {
>>> @@ -950,11 +989,13 @@ struct parse_val {
>>> rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>> }
>>> - /* create mempool, ring and vdevs info */
>>> + /* create mempool, ring, vdevs info and primary monitor */
>>> create_mp_ring_vdev();
>>> enable_pdump();
>>> + enable_primary_monitor();
>>> dump_packets();
>>> + disable_primary_monitor();
>>> cleanup_pdump_resources();
>>> /* dump debug stats */
>>> print_pdump_stats();
>>>
>>
>>
>
>
On 2019/5/1 0:39, Burakov, Anatoly wrote:
> On 30-Apr-19 12:25 PM, Suanming.Mou wrote:
>>
>> On 2019/4/30 17:42, Burakov, Anatoly wrote:
>>> On 30-Apr-19 12:35 PM, Suanming.Mou wrote:
>>>> When primary app exits, the residual running pdump will stop the
>>>> primary app to restart. Add pdump exits with primary support.
>>>>
>>>> Suggested-by: Varghese, Vipin <vipin.varghese@intel.com>
>>>> Suggested-by: Burakov, Anatoly <anatoly.burakov@intel.com>
>>>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
>>>> ---
>>>
>>> <snip>
>>>
>>>> static void
>>>> +disable_primary_monitor(void)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + /* Don't worry about it is primary exit case. The alarm cancel
>>>> + * function will take care about that. */
>>>> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>>> + if (ret < 0)
>>>> + printf("Fail to disable monitor fail:%d\n", ret);
>>>
>>> Double fail :)
>> Ah, yes, sorry for that the code gets worse. :(
>>>
>>>> +}
>>>> +
>>>> +static void
>>>> signal_handler(int sig_num)
>>>> {
>>>> if (sig_num == SIGINT) {
>>>> @@ -910,6 +936,19 @@ struct parse_val {
>>>> ;
>>>> }
>>>> +static void
>>>> +enable_primary_monitor(void)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + /* Once primary exits, so will pdump. */
>>>> + ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>>>> + if (ret < 0) {
>>>> + cleanup_pdump_resources();
>>>> + rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
>>>> + }
>>>
>>> Why is this function void, when you could've called rte_exit() in
>>> the caller on failure? And why is it such a fatal error to set up
>>> the timer? IMO just a warning would've been enough.
>>
>> Here comes with two issues:
>>
>> Q1. The return value of the function:
>>
>> A1: I'm so sorry that it does not seem to make sense to check the
>> function's return value. Does it mean if we change the timer set up
>> from error to warning, then we can use the return value to judge if
>> need to disable the primary_monitor?
>>
>> Q2. The choice when rte_eal_alarm_set fail:
>>
>> A2: OK, agree with that.
>
> If this is non-fatal, no need to change anything - just print out a
> warning instead of rte_exit, and no more changes needed here.
Happy may day holiday! Thanks for your confirmation.
>
>>
>>>
>>>> +}
>>>> +
>>>> int
>>>> main(int argc, char **argv)
>>>> {
>>>> @@ -950,11 +989,13 @@ struct parse_val {
>>>> rte_exit(EXIT_FAILURE, "Invalid argument\n");
>>>> }
>>>> - /* create mempool, ring and vdevs info */
>>>> + /* create mempool, ring, vdevs info and primary monitor */
>>>> create_mp_ring_vdev();
>>>> enable_pdump();
>>>> + enable_primary_monitor();
>>>> dump_packets();
>>>> + disable_primary_monitor();
>>>> cleanup_pdump_resources();
>>>> /* dump debug stats */
>>>> print_pdump_stats();
>>>>
>>>
>>>
>>
>>
>
>
@@ -26,6 +26,7 @@
#include <rte_ring.h>
#include <rte_string_fns.h>
#include <rte_pdump.h>
+#include <rte_alarm.h>
#define CMD_LINE_OPT_PDUMP "pdump"
#define CMD_LINE_OPT_PDUMP_NUM 256
@@ -65,6 +66,7 @@
#define SIZE 256
#define BURST_SIZE 32
#define NUM_VDEVS 2
+#define MONITOR_INTERVAL (500 * 1000)
/* true if x is a power of 2 */
#define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +415,18 @@ struct parse_val {
}
static void
+monitor_primary(void *arg __rte_unused)
+{
+ if (quit_signal)
+ return;
+
+ if (rte_eal_primary_proc_alive(NULL))
+ rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+ else
+ quit_signal = 1;
+}
+
+static void
print_pdump_stats(void)
{
int i;
@@ -537,6 +551,18 @@ struct parse_val {
}
static void
+disable_primary_monitor(void)
+{
+ int ret;
+
+ /* Don't worry about it is primary exit case. The alarm cancel
+ * function will take care about that. */
+ ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+ if (ret < 0)
+ printf("Fail to disable monitor fail:%d\n", ret);
+}
+
+static void
signal_handler(int sig_num)
{
if (sig_num == SIGINT) {
@@ -910,6 +936,19 @@ struct parse_val {
;
}
+static void
+enable_primary_monitor(void)
+{
+ int ret;
+
+ /* Once primary exits, so will pdump. */
+ ret = rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+ if (ret < 0) {
+ cleanup_pdump_resources();
+ rte_exit(EXIT_FAILURE, "Fail to monitor primary:%d\n", ret);
+ }
+}
+
int
main(int argc, char **argv)
{
@@ -950,11 +989,13 @@ struct parse_val {
rte_exit(EXIT_FAILURE, "Invalid argument\n");
}
- /* create mempool, ring and vdevs info */
+ /* create mempool, ring, vdevs info and primary monitor */
create_mp_ring_vdev();
enable_pdump();
+ enable_primary_monitor();
dump_packets();
+ disable_primary_monitor();
cleanup_pdump_resources();
/* dump debug stats */
print_pdump_stats();