From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
by inbox.dpdk.org (Postfix) with ESMTP id 6EABFA00C5;
Fri, 22 May 2020 02:55:50 +0200 (CEST)
Received: from [92.243.14.124] (localhost [127.0.0.1])
by dpdk.org (Postfix) with ESMTP id C56F91D736;
Fri, 22 May 2020 02:55:49 +0200 (CEST)
Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com
[209.85.167.68]) by dpdk.org (Postfix) with ESMTP id BC3461D727
for <dev@dpdk.org>; Fri, 22 May 2020 02:55:47 +0200 (CEST)
Received: by mail-lf1-f68.google.com with SMTP id r125so5526071lff.13
for <dev@dpdk.org>; Thu, 21 May 2020 17:55:47 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;
h=date:from:to:cc:subject:message-id:in-reply-to:references
:mime-version:content-transfer-encoding;
bh=+U8y09mERiVkIphgY4moBxZwI7IQ/B1yZZp0dhrnQuw=;
b=T4SakNEhFUXL7zefpD1fQA8hDGkAwz4S1Hzt/zp7M1DtN5itsekvNiZA3vdIZZBFV5
4uUfGBXddHAb4xttKJj4WiFh41xNkvYxAcS9Bwacvm9NHo0RG90G8NEka6TRcnn9J8fY
p9AfbF0aCOOS6bQroGn6FEiELYcpFuwxOKwoavqxLB+GIk3Nmov6hkLfOTRE1s3JTzvG
Gn04avWvcx071QcwZbgim9oTc2e4kAAtEzlNiTEGqoYMDAxivz8aj3/InTZsA7DHEAf2
LT5lecVj71VCpYp7oguigVqR6FYxcjKyOHOGiyjldHfPqKhk4GEqd/Yu+34yqi+gBmva
xkiQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20161025;
h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to
:references:mime-version:content-transfer-encoding;
bh=+U8y09mERiVkIphgY4moBxZwI7IQ/B1yZZp0dhrnQuw=;
b=Zt3GNGJs9jhpUYnUjP96iFiYAZ+arnqFHwyAxf/aSWzyCT7+Edr/aHHpRIwoxWE2Pa
u1FONcqlPtx9lmS2T1rRYrae7aczr9J5dHeqaN6ZuTXKshWsVRYsNMKWdLtozlZb4Nbn
/OkXpcCv+cxFJyIo99vjZbeuTTvb/aYAJ0bKkxIySKWiKRQk9y9JDqnLhsPmuSaF1uAm
QTbczY3Pb2W1+OqE0hb/67wzbURIIEtUNT7hADJ8T9G483rfZ3zLrJhjIL0eVo1X+/0b
YNW9+yw+/K6CCYuEmKjqG1KHOR7PfQRTpjIha1ykx/LJPR1EOd9DWRs/5jshF53pv9ex
0ODA==
X-Gm-Message-State: AOAM530ADd8nOcabjD0GnLijwwXleB3MOkrxG46rZ9RAuxDkpVTjhlA5
tJXLg9bD3H9QLf5C8xSLciI=
X-Google-Smtp-Source: ABdhPJzBqyhfHL+mfQBozga15o9tpjk4LwQlxhmEb49EtULf0rIYiQRHr2np3KHw1PRaGc76VMRd3g==
X-Received: by 2002:ac2:5597:: with SMTP id v23mr6200860lfg.42.1590108947219;
Thu, 21 May 2020 17:55:47 -0700 (PDT)
Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru.
[37.110.65.23])
by smtp.gmail.com with ESMTPSA id y28sm2040996ljn.4.2020.05.21.17.55.45
(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
Thu, 21 May 2020 17:55:46 -0700 (PDT)
Date: Fri, 22 May 2020 03:55:45 +0300
From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Tasnim Bashar <tbashar@mellanox.com>
Cc: dev@dpdk.org, harini.ramakrishnan@microsoft.com,
pallavi.kadam@intel.com, ranjit.menon@intel.com, ocardona@microsoft.com,
navasile@linux.microsoft.com, talshn@mellanox.com, fady@mellanox.com,
ophirmu@mellanox.com, thomas@monjalon.net
Message-ID: <20200522035545.76321d0a@sovereign>
In-Reply-To: <20200522001112.48932-1-tbashar@mellanox.com>
References: <20200522001112.48932-1-tbashar@mellanox.com>
X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH] eal/windows: fix invalid thread handle
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On Thu, 21 May 2020 17:11:12 -0700
Tasnim Bashar <tbashar@mellanox.com> wrote:
> Casting thread ID to handle is not accurate way to get thread handle.
> Need to use OpenThread function to get thread handle from thread ID.
>
> pthread_setaffinity_np and pthread_getaffinity_np functions
> for Windows are affected because of it.
Good catch.
Side note:
The sooner we move to a mature third-party pthread implementation, the better.
>
> Signed-off-by: Tasnim Bashar <tbashar@mellanox.com>
> ---
> lib/librte_eal/windows/include/pthread.h | 40 +++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/windows/include/pthread.h b/lib/librte_eal/windows/include/pthread.h
> index 0bbed5c3b8..d2a986f8fd 100644
> --- a/lib/librte_eal/windows/include/pthread.h
> +++ b/lib/librte_eal/windows/include/pthread.h
> @@ -18,6 +18,7 @@ extern "C" {
>
> #include <windows.h>
> #include <rte_common.h>
> +#include <rte_windows.h>
Build fails, because <rte_windows.h> doesn't include <rte_log.h>, macros from
which it uses. I'd rather include it there, so that <rte_windows.h> could be
used independently, but you can also add an include here.
If you include <rte_windows.h>, you don't need <windows.h> anymore.
>
> #define PTHREAD_BARRIER_SERIAL_THREAD TRUE
>
> @@ -50,7 +51,19 @@ typedef SYNCHRONIZATION_BARRIER pthread_barrier_t;
> static inline int
> eal_set_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
> {
> - SetThreadAffinityMask((HANDLE) threadid, *cpuset);
> + HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
> + if (thread_handle == NULL) {
> + RTE_LOG_WIN32_ERR("OpenThread()");
> + return -1;
> + }
> +
> + DWORD ret = SetThreadAffinityMask(thread_handle, *cpuset);
GetThreadAffinityMask() and SetThreadAffinityMask() operate on DWORD_PTR (8
bytes), not DWORD (4 byte). This applies to all usages of these functions in
the file and also to the type of "cpuset" parameter: it should be either "long
long *" (as rte_cpuset_t is declared) or just "rte_cpuset_t *".
Also, DPDK coding style suggests declaring variables at the start of the block
(i.e. function here and below).
> + if (ret == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> + CloseHandle(thread_handle);
> return 0;
> }
>
> @@ -60,12 +73,29 @@ eal_get_thread_affinity_mask(pthread_t threadid, unsigned long *cpuset)
> /* Workaround for the lack of a GetThreadAffinityMask()
> *API in Windows
> */
> - /* obtain previous mask by setting dummy mask */
> - DWORD dwprevaffinitymask =
> - SetThreadAffinityMask((HANDLE) threadid, 0x1);
Loss of data for the reason described above: here a 64-bit mask is
stored in a 32-bit variable, so later the wrong value of affinity will be
restored for cores 32 and on.
> + HANDLE thread_handle = OpenThread(THREAD_ALL_ACCESS, FALSE, threadid);
> + if (thread_handle == NULL) {
> + RTE_LOG_WIN32_ERR("OpenThread()");
> + return -1;
> + }
> +
> + /* obtain previous mask by setting dummy mask */
> + DWORD dwprevaffinitymask = SetThreadAffinityMask(thread_handle, 0x1);
> + if (dwprevaffinitymask == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> +
> /* set it back! */
> - SetThreadAffinityMask((HANDLE) threadid, dwprevaffinitymask);
> + DWORD ret = SetThreadAffinityMask(thread_handle, dwprevaffinitymask);
> + if (ret == 0) {
> + RTE_LOG_WIN32_ERR("SetThreadAffinityMask()");
> + CloseHandle(thread_handle);
> + return -1;
> + }
> *cpuset = dwprevaffinitymask;
> + CloseHandle(thread_handle);
> return 0;
> }
>
--
Dmitry Kozlyuk