[v4] 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 | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
Comments
Thanks for the patch work with rte_eal_alaram. But I am not able to find
1. the documentation update.
2. cover letter.
3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
Can you share a new patch as v5 with these changes?
Thanks
Vipin Varghese
> -----Original Message-----
> From: Suanming.Mou <mousuanming@huawei.com>
> Sent: Tuesday, April 30, 2019 9:09 AM
> To: dev@dpdk.org
> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Subject: [PATCH v4] 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>
> ---
> app/pdump/main.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/app/pdump/main.c b/app/pdump/main.c index 3d20854..c3413da
> 100644
> --- a/app/pdump/main.c
> +++ b/app/pdump/main.c
> @@ -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) @@ -864,12 +866,26 @@ struct
> parse_val {
> return 0;
> }
>
> +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 inline void
> dump_packets(void)
> {
> int i;
> uint32_t lcore_id = 0;
>
> + /* Once primary exits, so will pdump. */
> + rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
> +
> if (!multiple_core_capture) {
> printf(" core (%u), capture for (%d) tuples\n",
> rte_lcore_id(), num_tuples);
> --
> 1.8.3.4
On 2019/4/30 10:33, Varghese, Vipin wrote:
> Thanks for the patch work with rte_eal_alaram. But I am not able to find
>
> 1. the documentation update.
I'd like to reconfirm that since Anatoly suggested to make the option
default, I deleted the option supporting code.
Is it aligned that we can make it default?
If it is aligned to make it default, then if I understand correctly, to
add a new note in the documentation to describe pdump will exit with
primary will be OK?
> 2. cover letter.
Sorry for that. Will add the cover letter in the V5 patch.
> 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
Ah, yes.
>
> Can you share a new patch as v5 with these changes?
Thanks for your reviewing. I will send the next patch after Q1 confirmed.
>
> Thanks
> Vipin Varghese
>
>> -----Original Message-----
>> From: Suanming.Mou <mousuanming@huawei.com>
>> Sent: Tuesday, April 30, 2019 9:09 AM
>> To: dev@dpdk.org
>> Cc: Varghese, Vipin <vipin.varghese@intel.com>; Burakov, Anatoly
>> <anatoly.burakov@intel.com>
>> Subject: [PATCH v4] 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>
>> ---
>> app/pdump/main.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/app/pdump/main.c b/app/pdump/main.c index 3d20854..c3413da
>> 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -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) @@ -864,12 +866,26 @@ struct
>> parse_val {
>> return 0;
>> }
>>
>> +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 inline void
>> dump_packets(void)
>> {
>> int i;
>> uint32_t lcore_id = 0;
>>
>> + /* Once primary exits, so will pdump. */
>> + rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
>> +
>> if (!multiple_core_capture) {
>> printf(" core (%u), capture for (%d) tuples\n",
>> rte_lcore_id(), num_tuples);
>> --
>> 1.8.3.4
>
> .
>
snipped
> >
> > 1. the documentation update.
>
> I'd like to reconfirm that since Anatoly suggested to make the option default, I
> deleted the option supporting code.
>
> Is it aligned that we can make it default?
>
> If it is aligned to make it default, then if I understand correctly, to add a new note
> in the documentation to describe pdump will exit with primary will be OK?
I am not aware of the conversation and will leave this to maintainer if the option should be `--exit-with-primary` or `--do-not-exit-primary`.
Please add the changes to `doc/guides/tools/proc_info.rst` of the changes and new option that gets added.
>
> > 2. cover letter.
> Sorry for that. Will add the cover letter in the V5 patch.
thanks
> > 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
> Ah, yes.
Thanks
> >
> > Can you share a new patch as v5 with these changes?
> Thanks for your reviewing. I will send the next patch after Q1 confirmed.
Ok
snipped
On 30-Apr-19 3:33 AM, Varghese, Vipin wrote:
> Thanks for the patch work with rte_eal_alaram. But I am not able to find
>
> 1. the documentation update.
> 2. cover letter.
Why would a single patch need a cover letter? I don't think it's needed
in this case. The commit message is enough.
> 3. for signal SIGINT, set for ` rte_eal_alarm_cancel`.
>
> Can you share a new patch as v5 with these changes?
>
> Thanks
> Vipin Varghese
>
snipped
> > Thanks for the patch work with rte_eal_alaram. But I am not able to
> > find
> >
> > 1. the documentation update.
> > 2. cover letter.
>
> Why would a single patch need a cover letter? I don't think it's needed in this
> case. The commit message is enough.
In my opinion, the cover letter is to be added as it is new feature and explains the reasoning behind the new change. Please let me know if there change in the same?
snipped
When primary exits, pdump as the sencondary keeps running will make
primary fail to restart. Add the primary monitor in pdump to make it
eixts with primary.
Note:
One more thing still not confirmed is that if it can be set default.
In the V5 patch, it is still set as default. Since it seems there is
no disadvantages to make it exit with primary as default as Anatoly
suggested. But the exit_with_primary configuration is needed, there
will be a v6 patch and the doc update.
Change in v5:
- add rte_eal_alarm_cancel
- make the primary monitor enable/disable more indepence.
Change in v4:
- remove the exit_with_primary option as Anatoly suggested.
- fix typo.
Change in v3:
- remove the redundant return.
Change in v2:
- add exit_with_primary option.
- use rte_eal_alarm_set to monitor the primary.
Suanming.Mou (1):
app/pdump: add pudmp exits with primary support.
app/pdump/main.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
On 30-Apr-19 11:37 AM, Varghese, Vipin wrote:
> snipped
>>> Thanks for the patch work with rte_eal_alaram. But I am not able to
>>> find
>>>
>>> 1. the documentation update.
>>> 2. cover letter.
>>
>> Why would a single patch need a cover letter? I don't think it's needed in this
>> case. The commit message is enough.
>
> In my opinion, the cover letter is to be added as it is new feature and explains the reasoning behind the new change. Please let me know if there change in the same?
>
> snipped
>
I'm obviously not an expert in cover letters, but in my view, cover
letter is only necessary whenever there is a complex patchset that
requires some explanation, background, etc. If there is only one patch,
everything that you could reasonably put in the cover letter should go
either into the commit message itself, or into commit notes if there is
some supplemental data (e.g. benchmark results etc.). Creating cover
letters for single patches is just unnecessary work IMO.
@@ -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)
@@ -864,12 +866,26 @@ struct parse_val {
return 0;
}
+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 inline void
dump_packets(void)
{
int i;
uint32_t lcore_id = 0;
+ /* Once primary exits, so will pdump. */
+ rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary, NULL);
+
if (!multiple_core_capture) {
printf(" core (%u), capture for (%d) tuples\n",
rte_lcore_id(), num_tuples);