From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E0FC3A0546; Thu, 29 Apr 2021 22:44:19 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 601D9410DD; Thu, 29 Apr 2021 22:44:19 +0200 (CEST) Received: from mail-lf1-f48.google.com (mail-lf1-f48.google.com [209.85.167.48]) by mails.dpdk.org (Postfix) with ESMTP id C0B40410D7 for ; Thu, 29 Apr 2021 22:44:17 +0200 (CEST) Received: by mail-lf1-f48.google.com with SMTP id n138so107007476lfa.3 for ; Thu, 29 Apr 2021 13:44:17 -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=RG5Y9Nzs5HetSCuLQscDNvwvCkpftIKRG/Q9B/5FikM=; b=iH1x4mj00i14YcYyP+dSFX9ZUJ7bnI3kNVmP/3FceXRAs+eH9vespXXf4QwWgrWSfB 4Dcfa/IwfBfJn7wlI1/co4XMagJ4f3YEj0xxAWxbWxLB0/R8mXvmF0f5/sosftMvBY4E KH/hp4L4qLGGoKS7SjvvFZKsYagW/qYXLTVD5pRvkQdVbyiT/QhPWgaplXEMI/WkafWF YDFzBst0LjS/N9cj6b1lChT8JugnLOilNKkDJWRM+KDu8A1nUTBLhtJtt81d35Ro9BZZ MEegmCuIfKTj3TSrmz2koH4OOx4tSBPjhGvvgn/iHwF4moLJpIgxH5PVFQDiFYVZbB3Y 8XoA== 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=RG5Y9Nzs5HetSCuLQscDNvwvCkpftIKRG/Q9B/5FikM=; b=nuiiASdZr7nByI0SwJWGnDH+pmH9c04DXLbsigEvBAHTbPXDDRX1JJ2yOVa9HJbSZJ lDDhA2NPs97QnV8fL5/fxzfR5WoJtH6Z2AyTW2tx66eep/+Bfv50RjlLsiOiYbBDbmY2 2WQVwCVWa5DEC4JdpJP2BbSYRntSSee+v6UBkMm/gSWIXhyajfC7AfCcnRr6Wu5lKgU2 5s62+wjJm0AgGyElIcJG3ZQiTGTLNimv9fgkc9zupe5thPGFbbk6zi+ScmDOvvS6xINb yF39JyKy9CrySZQAYkUNaMlN/e/ZzWb5UUK/Lhi0CwtBK/3diDpsV0vy9LUD3fUT5FW6 r6qg== X-Gm-Message-State: AOAM531ZaUZaytSdAwRd6D4ciRDOH/LCSbYUgTbCfa4UiEuIIU5FXCN4 2JfNE0AW46ctwhTaketsV/0= X-Google-Smtp-Source: ABdhPJxeZZd0JdhmtYvw85L3fhewbIXpJ6nJ5LpBUVRhEImRZ9GWyckGYumQJwez6NHbnev4XeUiww== X-Received: by 2002:a19:6a07:: with SMTP id u7mr925994lfu.579.1619729057302; Thu, 29 Apr 2021 13:44:17 -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 n5sm84024lfh.25.2021.04.29.13.44.15 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Apr 2021 13:44:15 -0700 (PDT) Date: Thu, 29 Apr 2021 23:44:14 +0300 From: Dmitry Kozlyuk To: Narcisa Ana Maria Vasile Cc: dev@dpdk.org, thomas@monjalon.net, khot@microsoft.com, navasile@microsoft.com, dmitrym@microsoft.com, roretzla@microsoft.com, talshn@nvidia.com, ocardona@microsoft.com, bruce.richardson@intel.com, david.marchand@redhat.com, pallavi.kadam@intel.com Message-ID: <20210429234414.73ce39d7@sovereign> In-Reply-To: <1617413948-10504-7-git-send-email-navasile@linux.microsoft.com> References: <1617057640-24301-2-git-send-email-navasile@linux.microsoft.com> <1617413948-10504-1-git-send-email-navasile@linux.microsoft.com> <1617413948-10504-7-git-send-email-navasile@linux.microsoft.com> X-Mailer: Claws Mail 3.17.6 (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 v6 06/10] eal: add thread lifetime management X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" 2021-04-02 18:39 (UTC-0700), Narcisa Ana Maria Vasile: [...] > diff --git a/lib/librte_eal/windows/rte_thread.c b/lib/librte_eal/windows/rte_thread.c > index f61103bbc..86bbd7bc2 100644 > --- a/lib/librte_eal/windows/rte_thread.c > +++ b/lib/librte_eal/windows/rte_thread.c > @@ -329,6 +329,131 @@ rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr, > return 0; > } > > +int > +rte_thread_create(rte_thread_t *thread_id, > + const rte_thread_attr_t *thread_attr, > + void *(*thread_func)(void *), void *args) > +{ > + int ret = 0; > + HANDLE thread_handle = NULL; > + GROUP_AFFINITY thread_affinity; > + > + thread_handle = CreateThread(NULL, 0, > + (LPTHREAD_START_ROUTINE)(ULONG_PTR)thread_func, thread_func returns void* (8 bytes), LPTHREAD_START_ROUTING returns DWORD (4 byte), is this cast safe? It seems to be on x86, can't say for ARM64. > + args, 0, thread_id); > + if (thread_handle == NULL) { > + ret = rte_thread_translate_win32_error(); > + RTE_LOG_WIN32_ERR("CreateThread()"); > + goto cleanup; > + } After this point, in case of errors the thread remains running, while the function reports failure. It's better to create the thread suspended, configure its attributes (destroying the thread on failure), then resume (start) the thread. > + > + if (thread_attr != NULL) { > + if (CPU_COUNT(&thread_attr->cpuset) > 0) { > + ret = rte_convert_cpuset_to_affinity(&thread_attr->cpuset, &thread_affinity); > + if (ret != 0) { > + RTE_LOG(DEBUG, EAL, "Unable to convert cpuset to thread affinity\n"); > + goto cleanup; > + } > + > + if (!SetThreadGroupAffinity(thread_handle, &thread_affinity, NULL)) { > + ret = rte_thread_translate_win32_error(); > + RTE_LOG_WIN32_ERR("SetThreadGroupAffinity()"); > + goto cleanup; > + } > + } > + ret = rte_thread_set_priority(*thread_id, thread_attr->priority); > + if (ret != 0) { > + RTE_LOG(DEBUG, EAL, "Unable to set thread priority\n"); > + goto cleanup; > + } > + } > + > + return 0; > + > +cleanup: > + if (thread_handle != NULL) { > + CloseHandle(thread_handle); > + thread_handle = NULL; > + } > + return ret; > +} [...] > + > +int > +rte_thread_cancel(rte_thread_t thread_id) > +{ > + int ret = 0; > + HANDLE thread_handle = NULL; > + > + thread_handle = OpenThread(THREAD_TERMINATE, FALSE, thread_id); > + if (thread_handle == NULL) { > + ret = rte_thread_translate_win32_error(); > + RTE_LOG_WIN32_ERR("OpenThread()"); > + goto cleanup; > + } > + > + /* > + * TODO: Behavior is different between POSIX and Windows threads. > + * POSIX threads wait for a cancellation point. > + * Current Windows emulation kills thread at any point. > + */ > + ret = TerminateThread(thread_handle, 0); This is not acceptable. There is a handful of pthread_cancel() usages in DPDK. The threads they cancel take spinlocks and mutexes, so interrupt at arbitrary point may cause a deadlock. User Cancellation points Notes ---- ------------------- ----- net/ipn3ke libfdt functions? net/kni usleep Linux-only raw/ifpga open/read/write/close vdpa/ifc read vdpa/mlx5 usleep, nanosleep lib/eventdev rte_epoll_wait no pthread CP Possible solutions: 1. Make specific DPDK functions cancellation points for this API and ensure DPDK users of pthread_cancel() call it. This way is almost non-invasive, but it's more work in EAL. 2. Divert from pthread style: add cancellation token concept, so that DPDK users can check themselves if they're cancelled. This is invasive, but very explicit and flexible. > + if (ret != 0) { > + ret = rte_thread_translate_win32_error(); > + RTE_LOG_WIN32_ERR("TerminateThread()"); > + goto cleanup; > + } > + > +cleanup: > + if (thread_handle != NULL) { > + CloseHandle(thread_handle); > + thread_handle = NULL; This line is not needed. > + } > + return ret; > +} > + > int > rte_thread_key_create(rte_thread_key *key, > __rte_unused void (*destructor)(void *))