Patch Comment List
List patch comments
GET /api/patches/470/comments/?format=api
[ { "id": 961, "web_url": "https://patches.dpdk.org/comment/961/", "msgid": "<2601191342CEEE43887BDE71AB9772582136DDF1@IRSMSX105.ger.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/2601191342CEEE43887BDE71AB9772582136DDF1@IRSMSX105.ger.corp.intel.com", "date": "2014-09-24T15:18:10", "subject": "Re: [dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.", "submitter": { "id": 33, "url": "https://patches.dpdk.org/api/people/33/?format=api", "name": "Ananyev, Konstantin", "email": "konstantin.ananyev@intel.com" }, "content": "Hi Michal,\n\n> -----Original Message-----\n> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Michal Jastrzebski\n> Sent: Tuesday, September 23, 2014 4:02 PM\n> To: dev@dpdk.org\n> Subject: [dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.\n> \n> It eliminates a race between threads using rte_alarm_cancel and rte_alarm_set.\n> \n> Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>\n> Reviewed-by: Michal Jastrzebski <michalx.k.jastrzebski@intel.com>\n\n\nThe patch looks good, but I think it is incomplete.\nAt rte_eal_alarm_set(), for newly allocated alarm_entry we never reset value of 'executing' to zero.\nYes, it seems that problem is not new, and was here for a while.\nProbably the easiest way to fix it:\n\n@@ -150,7 +150,7 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)\n if (us < 1 || us > (UINT64_MAX - US_PER_S) || cb_fn == NULL)\n return -EINVAL;\n\n- new_alarm = rte_malloc(NULL, sizeof(*new_alarm), 0);\n+ new_alarm = rte_zmalloc(NULL, sizeof(*new_alarm), 0);\n if (new_alarm == NULL)\n return -ENOMEM;\n\nPlus two nits, see below.\n\nThanks\nKonstantin\n\n> ---\n> lib/librte_eal/common/include/rte_alarm.h | 3 +-\n> lib/librte_eal/linuxapp/eal/eal_alarm.c | 68 +++++++++++++++++++----------\n> 2 files changed, 46 insertions(+), 25 deletions(-)\n> \n> diff --git a/lib/librte_eal/common/include/rte_alarm.h b/lib/librte_eal/common/include/rte_alarm.h\n> index d451522..f5f7de4 100644\n> --- a/lib/librte_eal/common/include/rte_alarm.h\n> +++ b/lib/librte_eal/common/include/rte_alarm.h\n> @@ -76,7 +76,8 @@ typedef void (*rte_eal_alarm_callback)(void *arg);\n> int rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb, void *cb_arg);\n> \n> /**\n> - * Function to cancel an alarm callback which has been registered before.\n> + * Function to cancel an alarm callback which has been registered before. If\n> + * used ouside alarm callback it wait for all callbacks to finish its execution.\n\ns/ouside/outside/\n\n\n> *\n> * @param cb_fn\n> * alarm callback\n> diff --git a/lib/librte_eal/linuxapp/eal/eal_alarm.c b/lib/librte_eal/linuxapp/eal/eal_alarm.c\n> index 480f0cb..0561dbf 100644\n> --- a/lib/librte_eal/linuxapp/eal/eal_alarm.c\n> +++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c\n> @@ -69,12 +69,14 @@ struct alarm_entry {\n> \tstruct timeval time;\n> \trte_eal_alarm_callback cb_fn;\n> \tvoid *cb_arg;\n> -\tvolatile int executing;\n> +\tvolatile uint8_t executing;\n> +\tvolatile pthread_t executing_id;\n> };\n> \n> static LIST_HEAD(alarm_list, alarm_entry) alarm_list = LIST_HEAD_INITIALIZER();\n> static rte_spinlock_t alarm_list_lk = RTE_SPINLOCK_INITIALIZER;\n> \n> +\n> static struct rte_intr_handle intr_handle = {.fd = -1 };\n> static int handler_registered = 0;\n> static void eal_alarm_callback(struct rte_intr_handle *hdl, void *arg);\n> @@ -108,11 +110,14 @@ eal_alarm_callback(struct rte_intr_handle *hdl __rte_unused,\n> \t\t\t(ap->time.tv_sec < now.tv_sec || (ap->time.tv_sec == now.tv_sec &&\n> \t\t\t\t\t\tap->time.tv_usec <= now.tv_usec))){\n> \t\tap->executing = 1;\n> +\t\tap->executing_id = pthread_self();\n> \t\trte_spinlock_unlock(&alarm_list_lk);\n> \n> \t\tap->cb_fn(ap->cb_arg);\n> \n> \t\trte_spinlock_lock(&alarm_list_lk);\n> +\t\tap->executing = 0;\n> +\n\nI don't think you need:\nap->executing = 0\nhere.\nYou are going to free ap anyway. \n\n> \t\tLIST_REMOVE(ap, next);\n> \t\trte_free(ap);\n> \t}\n> @@ -156,7 +161,6 @@ rte_eal_alarm_set(uint64_t us, rte_eal_alarm_callback cb_fn, void *cb_arg)\n> \tnew_alarm->cb_arg = cb_arg;\n> \tnew_alarm->time.tv_usec = (now.tv_usec + us) % US_PER_S;\n> \tnew_alarm->time.tv_sec = now.tv_sec + ((now.tv_usec + us) / US_PER_S);\n> -\tnew_alarm->executing = 0;\n> \n> \trte_spinlock_lock(&alarm_list_lk);\n> \tif (!handler_registered) {\n> @@ -202,34 +206,50 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg)\n> {\n> \tstruct alarm_entry *ap, *ap_prev;\n> \tint count = 0;\n> +\tint executing;\n> \n> \tif (!cb_fn)\n> \t\treturn -1;\n> \n> -\trte_spinlock_lock(&alarm_list_lk);\n> -\t/* remove any matches at the start of the list */\n> -\twhile ((ap = LIST_FIRST(&alarm_list)) != NULL &&\n> -\t\t\tcb_fn == ap->cb_fn && ap->executing == 0 &&\n> -\t\t\t(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {\n> -\t\tLIST_REMOVE(ap, next);\n> -\t\trte_free(ap);\n> -\t\tcount++;\n> -\t}\n> -\tap_prev = ap;\n> -\n> -\t/* now go through list, removing entries not at start */\n> -\tLIST_FOREACH(ap, &alarm_list, next) {\n> -\t\t/* this won't be true first time through */\n> -\t\tif (cb_fn == ap->cb_fn && ap->executing == 0 &&\n> +\tdo {\n> +\t\texecuting = 0;\n> +\t\trte_spinlock_lock(&alarm_list_lk);\n> +\t\t/* remove any matches at the start of the list */\n> +\t\twhile ((ap = LIST_FIRST(&alarm_list)) != NULL &&\n> +\t\t\t\tcb_fn == ap->cb_fn &&\n> \t\t\t\t(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {\n> -\t\t\tLIST_REMOVE(ap,next);\n> -\t\t\trte_free(ap);\n> -\t\t\tcount++;\n> -\t\t\tap = ap_prev;\n> +\n> +\t\t\tif (ap->executing == 0) {\n> +\t\t\t\tLIST_REMOVE(ap, next);\n> +\t\t\t\trte_free(ap);\n> +\t\t\t\tcount++;\n> +\t\t\t} else {\n> +\t\t\t\tif (pthread_equal(ap->executing_id, pthread_self()) == 0)\n> +\t\t\t\t\texecuting++;\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \t\t}\n> \t\tap_prev = ap;\n> -\t}\n> -\trte_spinlock_unlock(&alarm_list_lk);\n> +\n> +\t\t/* now go through list, removing entries not at start */\n> +\t\tLIST_FOREACH(ap, &alarm_list, next) {\n> +\t\t\t/* this won't be true first time through */\n> +\t\t\tif (cb_fn == ap->cb_fn &&\n> +\t\t\t\t\t(cb_arg == (void *)-1 || cb_arg == ap->cb_arg)) {\n> +\n> +\t\t\t\tif (ap->executing == 0) {\n> +\t\t\t\t\tLIST_REMOVE(ap, next);\n> +\t\t\t\t\trte_free(ap);\n> +\t\t\t\t\tcount++;\n> +\t\t\t\t\tap = ap_prev;\n> +\t\t\t\t} else if (pthread_equal(ap->executing_id, pthread_self()) == 0)\n> +\t\t\t\t\texecuting++;\n> +\t\t\t}\n> +\t\t\tap_prev = ap;\n> +\t\t}\n> +\t\trte_spinlock_unlock(&alarm_list_lk);\n> +\t} while (executing != 0);\n> +\n> \treturn count;\n> }\n> -\n> --\n> 1.7.9.5", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [IPv6:::1])\n\tby dpdk.org (Postfix) with ESMTP id 081B3AFCA;\n\tWed, 24 Sep 2014 17:13:21 +0200 (CEST)", "from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby dpdk.org (Postfix) with ESMTP id 407866896\n\tfor <dev@dpdk.org>; Wed, 24 Sep 2014 17:13:18 +0200 (CEST)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby fmsmga101.fm.intel.com with ESMTP; 24 Sep 2014 08:18:24 -0700", "from irsmsx101.ger.corp.intel.com ([163.33.3.153])\n\tby fmsmga001.fm.intel.com with ESMTP; 24 Sep 2014 08:18:23 -0700", "from irsmsx105.ger.corp.intel.com ([169.254.7.158]) by\n\tIRSMSX101.ger.corp.intel.com ([169.254.1.194]) with mapi id\n\t14.03.0195.001; Wed, 24 Sep 2014 16:18:10 +0100" ], "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.04,589,1406617200\"; d=\"scan'208\";a=\"596223344\"", "From": "\"Ananyev, Konstantin\" <konstantin.ananyev@intel.com>", "To": "\"Jastrzebski, MichalX K\" <michalx.k.jastrzebski@intel.com>,\n\t\"dev@dpdk.org\" <dev@dpdk.org>", "Thread-Topic": "[dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.", "Thread-Index": "AQHP1z/R3alMUtTchEOehHad5SXhxJwQYVhQ", "Date": "Wed, 24 Sep 2014 15:18:10 +0000", "Message-ID": "<2601191342CEEE43887BDE71AB9772582136DDF1@IRSMSX105.ger.corp.intel.com>", "References": "<1411484549-711-1-git-send-email-michalx.k.jastrzebski@intel.com>", "In-Reply-To": "<1411484549-711-1-git-send-email-michalx.k.jastrzebski@intel.com>", "Accept-Language": "en-IE, en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-originating-ip": "[163.33.239.181]", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "quoted-printable", "MIME-Version": "1.0", "Subject": "Re: [dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null } ]