From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id DC3B358CB for ; Tue, 30 Apr 2019 18:39:16 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Apr 2019 09:39:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,414,1549958400"; d="scan'208";a="342200386" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.113]) ([10.237.220.113]) by fmsmga006.fm.intel.com with ESMTP; 30 Apr 2019 09:39:14 -0700 To: "Suanming.Mou" , dev@dpdk.org Cc: vipin.varghese@intel.com References: <1556595548-53745-1-git-send-email-mousuanming@huawei.com> <1556624124-54930-1-git-send-email-mousuanming@huawei.com> <1556624124-54930-2-git-send-email-mousuanming@huawei.com> <655f3c30-b277-fad7-6a68-64dd70fd3601@intel.com> <49788495-9412-972b-5500-9ff48b36eafe@huawei.com> From: "Burakov, Anatoly" Message-ID: <491c12ba-2cd8-e1bd-6622-0549439ecbc8@intel.com> Date: Tue, 30 Apr 2019 17:39:14 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <49788495-9412-972b-5500-9ff48b36eafe@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 30 Apr 2019 16:39:17 -0000 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 >>> Suggested-by: Burakov, Anatoly >>> Signed-off-by: Suanming.Mou >>> --- >> >> >> >>>   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(); >>> >> >> > > -- Thanks, Anatoly From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id C6FCCA0679 for ; Tue, 30 Apr 2019 18:39:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 35CAA5A6E; Tue, 30 Apr 2019 18:39:18 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id DC3B358CB for ; Tue, 30 Apr 2019 18:39:16 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Apr 2019 09:39:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,414,1549958400"; d="scan'208";a="342200386" Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.113]) ([10.237.220.113]) by fmsmga006.fm.intel.com with ESMTP; 30 Apr 2019 09:39:14 -0700 To: "Suanming.Mou" , dev@dpdk.org Cc: vipin.varghese@intel.com References: <1556595548-53745-1-git-send-email-mousuanming@huawei.com> <1556624124-54930-1-git-send-email-mousuanming@huawei.com> <1556624124-54930-2-git-send-email-mousuanming@huawei.com> <655f3c30-b277-fad7-6a68-64dd70fd3601@intel.com> <49788495-9412-972b-5500-9ff48b36eafe@huawei.com> From: "Burakov, Anatoly" Message-ID: <491c12ba-2cd8-e1bd-6622-0549439ecbc8@intel.com> Date: Tue, 30 Apr 2019 17:39:14 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <49788495-9412-972b-5500-9ff48b36eafe@huawei.com> Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v5] app/pdump: add pudmp exits with primary support. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Message-ID: <20190430163914.7P6U94lAmT6dNUXevNxQDDFje_vqEJz-iu-GQqGtO7w@z> 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 >>> Suggested-by: Burakov, Anatoly >>> Signed-off-by: Suanming.Mou >>> --- >> >> >> >>>   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(); >>> >> >> > > -- Thanks, Anatoly