List comments

GET /api/patches/470/comments/
HTTP 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

[
    {
        "id": 961,
        "web_url": "https://patches.dpdk.org/comment/961/",
        "msgid": "<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/",
            "name": "Konstantin Ananyev",
            "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": {
            "Sender": "\"dev\" <dev-bounces@dpdk.org>",
            "Thread-Index": "AQHP1z/R3alMUtTchEOehHad5SXhxJwQYVhQ",
            "List-Id": "patches and discussions about DPDK <dev.dpdk.org>",
            "X-MS-Has-Attach": "",
            "X-Original-To": "patchwork@dpdk.org",
            "Date": "Wed, 24 Sep 2014 15:18:10 +0000",
            "List-Help": "<mailto:dev-request@dpdk.org?subject=help>",
            "In-Reply-To": "<1411484549-711-1-git-send-email-michalx.k.jastrzebski@intel.com>",
            "Message-ID": "<2601191342CEEE43887BDE71AB9772582136DDF1@IRSMSX105.ger.corp.intel.com>",
            "X-BeenThere": "dev@dpdk.org",
            "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>",
            "Accept-Language": "en-IE, en-US",
            "References": "<1411484549-711-1-git-send-email-michalx.k.jastrzebski@intel.com>",
            "X-MS-TNEF-Correlator": "",
            "Return-Path": "<dev-bounces@dpdk.org>",
            "x-originating-ip": "[163.33.239.181]",
            "Errors-To": "dev-bounces@dpdk.org",
            "List-Post": "<mailto:dev@dpdk.org>",
            "MIME-Version": "1.0",
            "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"
            ],
            "From": "\"Ananyev, Konstantin\" <konstantin.ananyev@intel.com>",
            "Precedence": "list",
            "Delivered-To": "patchwork@dpdk.org",
            "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>",
            "Content-Transfer-Encoding": "quoted-printable",
            "Subject": "Re: [dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.",
            "List-Archive": "<http://dpdk.org/ml/archives/dev/>",
            "To": "\"Jastrzebski, MichalX K\" <michalx.k.jastrzebski@intel.com>,\n\t\"dev@dpdk.org\" <dev@dpdk.org>",
            "Content-Language": "en-US",
            "Content-Type": "text/plain; charset=\"us-ascii\"",
            "X-Mailman-Version": "2.1.15",
            "X-ExtLoop1": "1",
            "Thread-Topic": "[dpdk-dev] [PATCH] Change alarm cancel function to thread-safe.",
            "X-IronPort-AV": "E=Sophos;i=\"5.04,589,1406617200\"; d=\"scan'208\";a=\"596223344\""
        }
    }
]