[v6] 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.
Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
---
Change in V6:
- remove "Suggested-by" tags and head line '.' in git log.
- adjust the rte_alarm.h head file position.
- add comment for MONITOR_INTERVAL.
- remove redunt fail in log.
- treat rte_eal_alarm_set fail as warning only.
- adjust the multiple line comments coding style.
---
app/pdump/main.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 43 insertions(+), 1 deletion(-)
Comments
Hi Suanming,
snipped
>
> /* true if x is a power of 2 */
> #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct
Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
> 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;
> +}
This is suggestion, why not omit else part with
`
if (rte_eal_primary_proc_alive(NULL)) {
rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
return;
}
`
Snipped
As suggested in v4 can you update the `pdump.rst` on the new behaviour?
Thanks
Vipin Varghese
On 2019/5/2 16:04, Varghese, Vipin wrote:
> Hi Suanming,
>
> snipped
>> /* true if x is a power of 2 */
>> #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct
> Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
I'm sorry, but that line is not add by this patch this time.
Maybe another commit is more suitable to fix the previous code.
>
>> 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;
>> +}
> This is suggestion, why not omit else part with
>
> `
> if (rte_eal_primary_proc_alive(NULL)) {
> rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
> return;
> }
> `
Thanks for the suggestion. It's OK for me. If there's one more vote, I
will do it.
>
> Snipped
>
> As suggested in v4 can you update the `pdump.rst` on the new behaviour?
As noted in the cover letter in v5. The `exit with primary`
configuration should be made as default or not is still not confirmed
from the maintainer.
Since `exit with primary` is now removed in the patch and made as
default per Anatoly's suggestion and not get more information from the
maintainer, the update of the doc is also got hung up.
>
> Thanks
> Vipin Varghese
>
>
On 02-May-19 9:32 AM, Suanming.Mou wrote:
>
> On 2019/5/2 16:04, Varghese, Vipin wrote:
>> Hi Suanming,
>>
>> snipped
>>> /* true if x is a power of 2 */
>>> #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@ struct
>> Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
>
> I'm sorry, but that line is not add by this patch this time.
>
> Maybe another commit is more suitable to fix the previous code.
Yes, if there are issues with the code that aren't directly related to
the patch and aren't touched by it, they should be addressed as a
separate patch.
>
>>
>>> 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;
>>> +}
>> This is suggestion, why not omit else part with
>>
>> `
>> if (rte_eal_primary_proc_alive(NULL)) {
>> rte_eal_alarm_set(MONITOR_INTERVAL, monitor_primary,NULL);
>> return;
>> }
>> `
> Thanks for the suggestion. It's OK for me. If there's one more vote, I
> will do it.
No preference. Either way works, so i'd keep it as is.
HI Suanming,
snipped
> >> /* true if x is a power of 2 */
> >> #define POWEROF2(x) ((((x)-1) & (x)) == 0) @@ -413,6 +416,18 @@
> >> struct
> > Can we use ` RTE_IS_POWER_OF_2(n) ' instead of ` POWEROF2`?
>
> I'm sorry, but that line is not add by this patch this time.
>
> Maybe another commit is more suitable to fix the previous code.
Ok
Snipped
> >
> > As suggested in v4 can you update the `pdump.rst` on the new behaviour?
>
> As noted in the cover letter in v5. The `exit with primary` configuration should be
> made as default or not is still not confirmed from the maintainer.
>
> Since `exit with primary` is now removed in the patch and made as default per
> Anatoly's suggestion and not get more information from the maintainer, the
> update of the doc is also got hung up.
I am not clear with this, if there is change in behaviour of the application it has to be documented.
Thanks
Vipin Varghese
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
<snip>
> 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;
> +}
> +
Adding the log message saying primary existing so, secondary.. would be helpful here.
I am ok to have it as default behaviour.
As Vipin mentioned, can you also update the document doc/guides/tools/pdump.rst
Thanks,
Reshma
Hi,
Thanks for the advises from you all.
The latest v7 patch has sent out.
Br,
Mou
On 2019/5/2 17:54, Pattan, Reshma wrote:
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Suanming.Mou
>
> <snip>
>
>> 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;
>> +}
>> +
> Adding the log message saying primary existing so, secondary.. would be helpful here.
> I am ok to have it as default behaviour.
> As Vipin mentioned, can you also update the document doc/guides/tools/pdump.rst
>
> Thanks,
> Reshma
>
>
@@ -13,6 +13,7 @@
#include <net/if.h>
#include <rte_eal.h>
+#include <rte_alarm.h>
#include <rte_common.h>
#include <rte_debug.h>
#include <rte_ethdev.h>
@@ -65,6 +66,8 @@
#define SIZE 256
#define BURST_SIZE 32
#define NUM_VDEVS 2
+/* Enough to set it to 500ms for exiting. */
+#define MONITOR_INTERVAL (500 * 1000)
/* true if x is a power of 2 */
#define POWEROF2(x) ((((x)-1) & (x)) == 0)
@@ -413,6 +416,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 +552,20 @@ 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. Ignore the ENOENT case.
+ */
+ ret = rte_eal_alarm_cancel(monitor_primary, NULL);
+ if (ret < 0)
+ printf("Fail to disable monitor:%d\n", ret);
+}
+
+static void
signal_handler(int sig_num)
{
if (sig_num == SIGINT) {
@@ -910,6 +939,17 @@ 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)
+ printf("Fail to enable monitor:%d\n", ret);
+}
+
int
main(int argc, char **argv)
{
@@ -950,11 +990,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();