Re: [dpdk-dev] [PATCH v2] app/pdump: add exit_with_primary option support.

[Suanming.Mou <mousuanming@huawei.com>]

On 2019/4/29 17:14, Burakov, Anatoly wrote:
> On 28-Apr-19 5:58 AM, Suanming.Mou wrote:
>> When primary app exits, the residual running pdump will stop
>> the primary app to restart. Add an exit_with_primary option
>> to make pdump exit with primary.
>>
>> 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 | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/app/pdump/main.c b/app/pdump/main.c
>> index 3d20854..3909f15 100644
>> --- a/app/pdump/main.c
>> +++ b/app/pdump/main.c
>> @@ -26,11 +26,14 @@
>>   #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
>>   #define CMD_LINE_OPT_MULTI "multi"
>>   #define CMD_LINE_OPT_MULTI_NUM 257
>> +#define CMD_LINE_OPT_EXIT_WP "exit_with_primary"
>> +#define CMD_LINE_OPT_EXIT_WP_NUM 258
>
> Unrelated to this patch, but seems very flaky and prone to error. How 
> about replacing this stuff with enum-based automatic value assignment, 
> like in lib/librte_eal/common/eal_options.h ?
:)
>
>>   #define PDUMP_PORT_ARG "port"
>>   #define PDUMP_PCI_ARG "device_id"
>>   #define PDUMP_QUEUE_ARG "queue"
>> @@ -65,6 +68,7 @@
>>   #define SIZE 256
>>   #define BURST_SIZE 32
>>   #define NUM_VDEVS 2
>> +#define MONITOR_INTERVEL (500 * 1000)
>
> I believe it should be INTERVAL
Ah, yes, sorry for the typo.
>
>>     /* true if x is a power of 2 */
>>   #define POWEROF2(x) ((((x)-1) & (x)) == 0)
>> @@ -143,12 +147,14 @@ struct parse_val {
>>   static struct rte_eth_conf port_conf_default;
>>   static volatile uint8_t quit_signal;
>>   static uint8_t multiple_core_capture;
>> +static uint8_t exit_with_primary;
>
> <snip>
Could you please help to confirm that the 'snip' here mean we should 
delete the 'exit_with_primary' code?
>
>>   @@ -403,6 +410,9 @@ struct parse_val {
>>           case CMD_LINE_OPT_MULTI_NUM:
>>               multiple_core_capture = 1;
>>               break;
>> +        case CMD_LINE_OPT_EXIT_WP_NUM:
>> +            exit_with_primary = 1;
>> +            break;
>
> Any particular reason why it is not made the default?

It's OK to make it default.  How about Varghese ?

Thank you for the review.