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 456BCA0C44; Wed, 9 Jun 2021 01:04:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2F3BD4111B; Wed, 9 Jun 2021 01:04:12 +0200 (CEST) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by mails.dpdk.org (Postfix) with ESMTP id 8183A4111A for ; Wed, 9 Jun 2021 01:04:11 +0200 (CEST) Received: by mail-lj1-f171.google.com with SMTP id x14so14858631ljp.7 for ; Tue, 08 Jun 2021 16:04:11 -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=7rKYn0Q3BWk+pcKTh05xTekPYaUnocFKBGRmX9VCjR8=; b=PakkgnVlu1jmrowa6wwejmhU5VBUdMPTiU6jA5tufR5osI9MMLj+ta0hnKj1aUpez2 pmzvQwp936NSWeMaUMhvRtRHTfD8aS9VG+2aZSssLcHhcmQz7S5qzi/Qwzz66oiaAqIU wEKFqVFwNWvXXYSx9V0F+rr5xNUDjOTGm7SSCKD0dIam4b0GxZV2FCoDf6EqJKiJRATY 4IiF3tf5pTyI6PPVlhTKH0ZRnFtqgXlKSpoBSCASNquozsiL7Dg6UXhidZsQLg7zAP9R xlG1KYf5GLP5BhbCcAHVC6LJnKUeGrA9c3OoYEqHapnCVR6/QNABEhVh6To69/fNl79h RLpQ== 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=7rKYn0Q3BWk+pcKTh05xTekPYaUnocFKBGRmX9VCjR8=; b=f6qmny8xFj5/rwCBAMWHIdAog0w/P/f0776p8+nlX0vw8byvmrInqbNRicQDyBL7/C SNUR/hq2f4Y/pyJwzZvzKbGt30LGFhBHALz4c6154q0E8M54BG35XjpW4qhIVh9LUV50 Gi6OID4oMi3hHHpFodgCrf8SZAPoOMFoc/eUpMgOuIP9gTwcdBUfip/Uj0izEi3D12Oo RHGiH90GFM7VNAbMz7y+lhKZwKUPvxeKO98S10w2P5VATc4NZD2dgn+DfbbKi729tsVk qSDFdwtO73+8GzGj5fKM/0reRc6STJJecye+s47IskzGd77386XuqBRHTVN3ehR5i8lu Fs4w== X-Gm-Message-State: AOAM530Udv4graNpAFj3qdkbxS4gYrp4VRVw9h0vMGGAZIqBLeji2hF/ hO5kUHI2iNlVZjDIK2BVhTQ= X-Google-Smtp-Source: ABdhPJwhItGBjXVqgBI/yO8HW5I9DUNB6EfDl9H8TyhQ9dtKmcY1vNuzVIHH3Tiv5pMu+aNvhvpZFw== X-Received: by 2002:a05:651c:a08:: with SMTP id k8mr19744803ljq.391.1623193451169; Tue, 08 Jun 2021 16:04:11 -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 c12sm122492lfd.105.2021.06.08.16.04.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Jun 2021 16:04:10 -0700 (PDT) Date: Wed, 9 Jun 2021 02:04:09 +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: <20210609020409.0c1fb82b@sovereign> In-Reply-To: <1622850274-6946-7-git-send-email-navasile@linux.microsoft.com> References: <1622849908-5710-1-git-send-email-navasile@linux.microsoft.com> <1622850274-6946-1-git-send-email-navasile@linux.microsoft.com> <1622850274-6946-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 v9 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-06-04 16:44 (UTC-0700), Narcisa Ana Maria Vasile: [...] > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h > index 5c54cd9d67..1d481b9ad5 100644 > --- a/lib/eal/include/rte_thread.h > +++ b/lib/eal/include/rte_thread.h > @@ -208,6 +208,73 @@ __rte_experimental > int rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr, > enum rte_thread_priority priority); > > +/** > + * Create a new thread that will invoke the 'thread_func' routine. > + * > + * @param thread_id > + * A pointer that will store the id of the newly created thread. > + * > + * @param thread_attr > + * Attributes that are used at the creation of the new thread. > + * > + * @param thread_func > + * The routine that the new thread will invoke when starting execution. > + * > + * @param args > + * Arguments to be passed to the 'thread_func' routine. > + * > + * @return > + * On success, return 0. > + * On failure, return a positive errno-style error number. > + */ > +__rte_experimental > +int rte_thread_create(rte_thread_t *thread_id, > + const rte_thread_attr_t *thread_attr, > + void *(*thread_func)(void *), void *args); 1. Thread function prototype is used at least in 4 places, maybe give it a name, like `rte_thread_func`? 2. We can't easily support it's `void*` return type on Windows, because it doesn't fit in DWORD. In `rte_thread_join` below you use `int`. All `pthread_join` usages in DPDK ignore return value, but I'd rather keep it. Do you think it's OK to stick to `int`? [...] > +/** > + * Terminates a thread. > + * > + * @param thread_id > + * The id of the thread to be cancelled. > + * > + * @return > + * On success, return 0. > + * On failure, return a positive errno-style error number. > + */ > +__rte_experimental > +int rte_thread_cancel(rte_thread_t thread_id); What do you think of making this function internal for now? We don't want applications to rely on this prototype. To hide it from Doxygen, `/*` comment or #ifndef __DOXYGEN__ can be used. It is worth noting in commit message that it's not implemented for Windows and why. > + > +/** > + * Detaches a thread. Please explain what it means, because detaching is a pthread concept. > + * > + * @param thread_id > + * The id of the thread to be cancelled. Not "cancelled". > + * > + * @return > + * On success, return 0. > + * On failure, return a positive errno-style error number. > + */ > +__rte_experimental > +int rte_thread_detach(rte_thread_t thread_id); > + > + Redundant empty line. > #ifdef RTE_HAS_CPUSET > > /** > diff --git a/lib/eal/windows/rte_thread.c b/lib/eal/windows/rte_thread.c > index 6dc3d575c0..5afdd54e15 100644 > --- a/lib/eal/windows/rte_thread.c > +++ b/lib/eal/windows/rte_thread.c > @@ -14,6 +14,11 @@ struct eal_tls_key { > DWORD thread_index; > }; > > +struct thread_routine_ctx { > + void* (*start_routine) (void*); > + void *routine_args; > +}; > + > /* Translates the most common error codes related to threads */ > static int > thread_translate_win32_error(DWORD error) > @@ -346,6 +351,163 @@ rte_thread_attr_set_priority(rte_thread_attr_t *thread_attr, > return 0; > } > > +static DWORD > +thread_func_wrapper(void *args) > +{ > + struct thread_routine_ctx *pctx = args; > + intptr_t func_ret = 0; > + struct thread_routine_ctx ctx = { NULL, NULL }; > + > + ctx.start_routine = pctx->start_routine; > + ctx.routine_args = pctx->routine_args; > + > + free(pctx); > + > + func_ret = (intptr_t)ctx.start_routine(ctx.routine_args); > + return (DWORD)func_ret; > +} > + > +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; > + DWORD tid = 0; > + HANDLE thread_handle = NULL; > + GROUP_AFFINITY thread_affinity; > + struct thread_routine_ctx *ctx = NULL; > + > + ctx = calloc(1, sizeof(*ctx)); Why use `calloc()` for a scalar? [...] > +int > +rte_thread_cancel(rte_thread_t thread_id) > +{ > + int ret = 0; > + HANDLE thread_handle = NULL; > + > + thread_handle = OpenThread(THREAD_TERMINATE, FALSE, thread_id.opaque_id); > + if (thread_handle == NULL) { > + ret = thread_log_last_error("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); > + if (ret != 0) { > + ret = thread_log_last_error("TerminateThread()"); > + goto cleanup; > + } > + > +cleanup: > + if (thread_handle != NULL) { > + CloseHandle(thread_handle); > + thread_handle = NULL; > + } > + return ret; > +} As we've discussed before, such implementation should never be used. I suggest removing it for now, otherwise if someone enables the code calling rte_thread_cancel() for Windows, it will almost certainly lead to deadlock bugs. > + > +int > +rte_thread_detach(rte_thread_t thread_id) > +{ > + (void)thread_id; RTE_SET_USED/__rte_unused > + return ENOTSUP; > +} > + It should return success as the thread is in detached state after the call. > int > rte_thread_key_create(rte_thread_key *key, > __rte_unused void (*destructor)(void *))