[v8] 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>
Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
---
V8:
* reword the print info in monitor_primary.
* add release_19_05.rst update.
* add `Reviewed-by` tag.
app/pdump/main.c | 47 +++++++++++++++++++++++++++++++++-
doc/guides/rel_notes/release_19_05.rst | 4 +++
doc/guides/tools/pdump.rst | 2 ++
3 files changed, 52 insertions(+), 1 deletion(-)
Comments
Hi,
03/05/2019 07:48, Suanming. Mou:
> When primary app exits, the residual running pdump will stop the
> primary app to restart. Add pdump exits with primary support.
Sorry I fail to parse this sentence.
Maybe it should be longer to be more explicit about
what it the current issue and how it is solved.
Some comments in the code may also be improved.
> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
I think you should remove the dot in your name.
> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> V8:
> * reword the print info in monitor_primary.
> * add release_19_05.rst update.
As it is not a fix, it will slip in 19.08.
On 2019/5/5 5:17, Thomas Monjalon wrote:
> Hi,
>
> 03/05/2019 07:48, Suanming. Mou:
>> When primary app exits, the residual running pdump will stop the
>> primary app to restart. Add pdump exits with primary support.
> Sorry I fail to parse this sentence.
> Maybe it should be longer to be more explicit about
> what it the current issue and how it is solved.
Thanks for the suggestion. It's fine to add more contents.
> Some comments in the code may also be improved.
Could you please help to show the detail? Or it will be hard to do the
improvement.
` There are a thousand Hamlets in a thousand people's eyes.`
>
>> Signed-off-by: Suanming.Mou <mousuanming@huawei.com>
> I think you should remove the dot in your name.
Sorry for that, maybe I missed some important instructions which notes
the dot.
I will remove it later.
>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Reviewed-by: Vipin Varghese <vipin.varghese@intel.com>
>> ---
>> V8:
>> * reword the print info in monitor_primary.
>> * add release_19_05.rst update.
> As it is not a fix, it will slip in 19.08.
It seems 19.08 is still not started yet.
Does that mean the patch should be hung till 19.08 be started?
>
>
>
>
>
05/05/2019 03:20, Suanming. Mou:
> On 2019/5/5 5:17, Thomas Monjalon wrote:
> > Hi,
> >
> > 03/05/2019 07:48, Suanming. Mou:
> >> When primary app exits, the residual running pdump will stop the
> >> primary app to restart. Add pdump exits with primary support.
> > Sorry I fail to parse this sentence.
> > Maybe it should be longer to be more explicit about
> > what it the current issue and how it is solved.
> Thanks for the suggestion. It's fine to add more contents.
> > Some comments in the code may also be improved.
>
> Could you please help to show the detail? Or it will be hard to do the
> improvement.
>
> ` There are a thousand Hamlets in a thousand people's eyes.`
Yes, you're right :)
[..]
> >> V8:
> >> * reword the print info in monitor_primary.
> >> * add release_19_05.rst update.
> > As it is not a fix, it will slip in 19.08.
>
> It seems 19.08 is still not started yet.
>
> Does that mean the patch should be hung till 19.08 be started?
Yes, either to wait one week, or fake the 19.08 release notes template
to do your patch on top.
On 2019/5/5 17:42, Thomas Monjalon wrote:
> Yes, either to wait one week, or fake the 19.08 release notes template
> to do your patch on top.
Thanks. I think it will be updated when 19.08 started.
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();
On 2019/5/8 16:04, Thomas Monjalon wrote:
> Hi,
>
> I try to suggest some rewording below.
Thanks very much.
First, let me explain what is the patch work for.
As we all know, the pdump tool works as the secondary process.
When the primary process exits, if the secondary process keeps running
there, it will make the primary process can't start up again.
Since the ex-fbarry files are still attached by the secondary process
pdump, the 'new' primary process can't get these files locked.
The patch is just to set up an alarm which runs every 0.5s periodically
to monitor the primary process in the pdump.
Once the primary exits, so will the pdump.
>
> 03/05/2019 07:48, Suanming. Mou:
>> +/* Enough to set it to 500ms for exiting. */
>> +#define MONITOR_INTERVAL (500 * 1000)
> What is "enough"?
The 'enough' here means it will be fine to make the alarm to run
periodically in every 500ms to check if the primary process exits.
(Yes, someone can also suggest, "Well , I think xxxms will be better.")
> What is "it"?
So, the "it" here means the MONITOR_INTERVAL...
> What is the relation between MONITOR_INTERVAL and exiting?
Once the primary exits, as the alarm runs every 500ms, the alarm will
help the pdump to exit, the worst case will take 500ms for the pudmp to
exit.
>
> [...]
>> + /*
>> + * 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?
The 'it' and 'that' both means the pdump 'monitor_primary' recognises
the primary process exited and set the 'quit_signal' case.
In that case, the 'monitor_primary' is not readd to the alarm. I add the
comment in case someone want to say that "You are canceling a
non-existent alarm."
It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just
return 0 and set the ENOENT errno.
>
>> + 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.
That's OK.
>
>> create_mp_ring_vdev();
>> enable_pdump();
>> + enable_primary_monitor();
>
>
>
And one more word, the 'app' in the git log means 'application'.
Maybe it's better to change it to 'process' to make it describes more
clearly.
Thanks again for the suggestions.
BR,
Mou
About the title, you can write: "app/pdump: exit with primary process"
08/05/2019 11:37, Suanming. Mou:
> On 2019/5/8 16:04, Thomas Monjalon wrote:
> > Hi,
> >
> > I try to suggest some rewording below.
>
> Thanks very much.
>
> First, let me explain what is the patch work for.
>
> As we all know, the pdump tool works as the secondary process.
>
> When the primary process exits, if the secondary process keeps running
> there, it will make the primary process can't start up again.
>
> Since the ex-fbarry files are still attached by the secondary process
> pdump, the 'new' primary process can't get these files locked.
>
> The patch is just to set up an alarm which runs every 0.5s periodically
> to monitor the primary process in the pdump.
>
> Once the primary exits, so will the pdump.
If you feel some explanation is missing,
feel free to add it in the commit log.
> > 03/05/2019 07:48, Suanming. Mou:
> >> +/* Enough to set it to 500ms for exiting. */
> >> +#define MONITOR_INTERVAL (500 * 1000)
> > What is "enough"?
>
> The 'enough' here means it will be fine to make the alarm to run
> periodically in every 500ms to check if the primary process exits.
>
> (Yes, someone can also suggest, "Well , I think xxxms will be better.")
So it looks like you reply to a ghost in the code comment :)
> > What is "it"?
>
> So, the "it" here means the MONITOR_INTERVAL...
>
> > What is the relation between MONITOR_INTERVAL and exiting?
>
> Once the primary exits, as the alarm runs every 500ms, the alarm will
> help the pdump to exit, the worst case will take 500ms for the pudmp to
> exit.
Let's write it in a simpler descriptive form:
/* Maximum delay for exiting after primary process. */
> > [...]
> >> + /*
> >> + * 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?
>
> The 'it' and 'that' both means the pdump 'monitor_primary' recognises
> the primary process exited and set the 'quit_signal' case.
>
> In that case, the 'monitor_primary' is not readd to the alarm. I add the
> comment in case someone want to say that "You are canceling a
> non-existent alarm."
>
> It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just
> return 0 and set the ENOENT errno.
OK, so let's be more explicit:
"
Cancel monitoring of primary process.
There will be no error if no alarm is set
(in case primary process kill was detected earlier).
"
> >> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
> >> + if (ret < 0)
> >> + printf("Fail to disable monitor:%d\n", ret);
[...]
> And one more word, the 'app' in the git log means 'application'.
>
> Maybe it's better to change it to 'process' to make it describes more
> clearly.
Indeed, "process" is more correct in this context.
> Thanks again for the suggestions.
You're welcome.
On 2019/5/8 18:22, Thomas Monjalon wrote:
> About the title, you can write: "app/pdump: exit with primary process"
>
> 08/05/2019 11:37, Suanming. Mou:
>> On 2019/5/8 16:04, Thomas Monjalon wrote:
>>> Hi,
>>>
>>> I try to suggest some rewording below.
>> Thanks very much.
>>
>> First, let me explain what is the patch work for.
>>
>> As we all know, the pdump tool works as the secondary process.
>>
>> When the primary process exits, if the secondary process keeps running
>> there, it will make the primary process can't start up again.
>>
>> Since the ex-fbarry files are still attached by the secondary process
>> pdump, the 'new' primary process can't get these files locked.
>>
>> The patch is just to set up an alarm which runs every 0.5s periodically
>> to monitor the primary process in the pdump.
>>
>> Once the primary exits, so will the pdump.
> If you feel some explanation is missing,
> feel free to add it in the commit log.
Yes, it will be updated in the next patch.
>
>>> 03/05/2019 07:48, Suanming. Mou:
>>>> +/* Enough to set it to 500ms for exiting. */
>>>> +#define MONITOR_INTERVAL (500 * 1000)
>>> What is "enough"?
>> The 'enough' here means it will be fine to make the alarm to run
>> periodically in every 500ms to check if the primary process exits.
>>
>> (Yes, someone can also suggest, "Well , I think xxxms will be better.")
> So it looks like you reply to a ghost in the code comment :)
:)
>
>>> What is "it"?
>> So, the "it" here means the MONITOR_INTERVAL...
>>
>>> What is the relation between MONITOR_INTERVAL and exiting?
>> Once the primary exits, as the alarm runs every 500ms, the alarm will
>> help the pdump to exit, the worst case will take 500ms for the pudmp to
>> exit.
> Let's write it in a simpler descriptive form:
> /* Maximum delay for exiting after primary process. */
Got it.
>
>>> [...]
>>>> + /*
>>>> + * 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?
>> The 'it' and 'that' both means the pdump 'monitor_primary' recognises
>> the primary process exited and set the 'quit_signal' case.
>>
>> In that case, the 'monitor_primary' is not readd to the alarm. I add the
>> comment in case someone want to say that "You are canceling a
>> non-existent alarm."
>>
>> It's OK to cancel a non-existent alarm. The 'rte_eal_alarm_cancel' just
>> return 0 and set the ENOENT errno.
> OK, so let's be more explicit:
> "
> Cancel monitoring of primary process.
> There will be no error if no alarm is set
> (in case primary process kill was detected earlier).
> "
Great, it becomes more official now. The original one seems little
emotional.
>>>> + ret = rte_eal_alarm_cancel(monitor_primary, NULL);
>>>> + if (ret < 0)
>>>> + printf("Fail to disable monitor:%d\n", ret);
> [...]
>> And one more word, the 'app' in the git log means 'application'.
>>
>> Maybe it's better to change it to 'process' to make it describes more
>> clearly.
> Indeed, "process" is more correct in this context.
>
>> Thanks again for the suggestions.
> You're welcome.
>
>
>
> .
>
@@ -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,21 @@ 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);
+ return;
+ }
+
+ printf("Primary process is no longer active, exiting...\n");
+ quit_signal = 1;
+}
+
+static void
print_pdump_stats(void)
{
int i;
@@ -537,6 +555,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 +942,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 +993,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();
@@ -200,6 +200,10 @@ New Features
Improved testpmd application performance on ARM platform. For ``macswap``
forwarding mode, NEON intrinsics were used to do swap to save CPU cycles.
+* **Updated the pdump application.**
+
+ Add support for pdump to exit with primary process.
+
Removed Items
-------------
@@ -26,6 +26,8 @@ a DPDK secondary process and is capable of enabling packet capture on dpdk ports
Once the libpcap development files are installed, the libpcap based PMD
can be enabled by setting CONFIG_RTE_LIBRTE_PMD_PCAP=y and recompiling the DPDK.
+ * The ``dpdk-pdump`` tool runs as a DPDK secondary process. It exits when
+ the primary application exits.
Running the Application
-----------------------