From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id C451CA0350; Wed, 1 Jul 2020 11:11:09 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 128381C2B8; Wed, 1 Jul 2020 11:11:09 +0200 (CEST) Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by dpdk.org (Postfix) with ESMTP id 473A11C2AA for ; Wed, 1 Jul 2020 11:11:07 +0200 (CEST) Received: by mail-wm1-f65.google.com with SMTP id w3so9954257wmi.4 for ; Wed, 01 Jul 2020 02:11:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=L6AYoFa5XhzA4Z42o/b81DLLxK3dLa0pxz2qUvqWSLs=; b=VOFLgmyLVG2rTgUabtLBXsExu+tk2Oisjs13ckC/AMciNEf4YgGMtd028AD8pi/QMx 3oUXnxg+Z16/37ccd4HIy8OZRQOqWfCOC51exlzgqmgEueZ9xD8KXCxULuZASiyZoRZ7 C+7z00PJxT0ynkpSazuD5hvg2WwBIXu1rVXuAEgbScb4E1IomjG+euEDngILYnPenS2R F9d94Z7k3LxNhglAYEMjTn502PjY24tWu3x2ILt6sRbvc1GdGVnO8rXJYFglNgy40bDE yxbqh49hpcUgZdOtQ7ju+mlH7k/ow20S4XpUUrFS77PQkqqOpB7z/ldcb6C2SB6Bt5/H ZQNw== 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:references :mime-version:content-disposition:in-reply-to:user-agent; bh=L6AYoFa5XhzA4Z42o/b81DLLxK3dLa0pxz2qUvqWSLs=; b=Onu4Ubxtzb1fyt+MK/J8l36u+GBV7OK7nqVxoJ0SdEX+0tPfmx9Ww5PgrGuMy4pYcl 6h9dcL/alBSKSV8GcoiXTb7Byvn1D38+9Z+aaH/UH1P/ARI0f4qRn4jUZMvXHTCZZ5+U EGuq81tmS5BKZKrHFzwcM79SFXtm/eFFKnLKTczkMefkGqv2poHwjCcPKl1TSFdGpo0D p8XpJ3OUb/YVc01+UbpNfFIyoxeaT9rZkv/7AQkl8FW0PhZthwKBw+xRfxdC1lbfnia3 r2i7+/pmJBj2V9SR8fMYZESQmqT91zD5kDE9ewuwdmF8VYLmV6D0dMIY6+suD0b0PGt4 x/Ag== X-Gm-Message-State: AOAM531yEJg4TCjLFP3hVo3xBlAK/v34MDue9zRPeX1EMP7C7vThXAK6 +zeA9haoCwjXwy8Q6mhDBJ2rzQ== X-Google-Smtp-Source: ABdhPJyeSVpfVoRRZJ7c89Y0H0Ifku06ceA+NnAk7hv4bR2nUB+/zy0RzkI1FV2/O06kfMSklC8dRg== X-Received: by 2002:a1c:345:: with SMTP id 66mr10461402wmd.31.1593594666781; Wed, 01 Jul 2020 02:11:06 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id v18sm6955849wrv.49.2020.07.01.02.11.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Jul 2020 02:11:06 -0700 (PDT) Date: Wed, 1 Jul 2020 11:11:05 +0200 From: Olivier Matz To: David Marchand Cc: dev , Jerin Jacob , Bruce Richardson , Ray Kinsella , Thomas Monjalon , Andrew Rybchenko , Kevin Traynor , Ian Stokes , Ilya Maximets , John McNamara , Marko Kovacevic , Anatoly Burakov , Neil Horman Message-ID: <20200701091105.GO5869@platinum> References: <20200610144506.30505-1-david.marchand@redhat.com> <20200626144736.11011-1-david.marchand@redhat.com> <20200626144736.11011-7-david.marchand@redhat.com> <20200630100717.GF5869@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-dev] [PATCH v4 6/9] eal: register non-EAL threads as lcores X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" On Wed, Jul 01, 2020 at 09:13:36AM +0200, David Marchand wrote: > On Tue, Jun 30, 2020 at 12:07 PM Olivier Matz wrote: > > > > On Fri, Jun 26, 2020 at 04:47:33PM +0200, David Marchand wrote: > > > DPDK allows calling some part of its API from a non-EAL thread but this > > > has some limitations. > > > OVS (and other applications) has its own thread management but still > > > want to avoid such limitations by hacking RTE_PER_LCORE(_lcore_id) and > > > faking EAL threads potentially unknown of some DPDK component. > > > > > > Introduce a new API to register non-EAL thread and associate them to a > > > free lcore with a new NON_EAL role. > > > This role denotes lcores that do not run DPDK mainloop and as such > > > prevents use of rte_eal_wait_lcore() and consorts. > > > > > > Multiprocess is not supported as the need for cohabitation with this new > > > feature is unclear at the moment. > > > > > > Signed-off-by: David Marchand > > > Acked-by: Andrew Rybchenko > > > --- > > > Changes since v2: > > > - refused multiprocess init once rte_thread_register got called, and > > > vice versa, > > > - added warning on multiprocess in rte_thread_register doxygen, > > > > > > Changes since v1: > > > - moved cleanup on lcore role code in patch 5, > > > - added unit test, > > > - updated documentation, > > > - changed naming from "external thread" to "registered non-EAL thread" > > > > > > --- > > > MAINTAINERS | 1 + > > > app/test/Makefile | 1 + > > > app/test/autotest_data.py | 6 + > > > app/test/meson.build | 2 + > > > app/test/test_lcores.c | 139 ++++++++++++++++++ > > > doc/guides/howto/debug_troubleshoot.rst | 5 +- > > > .../prog_guide/env_abstraction_layer.rst | 22 +-- > > > doc/guides/prog_guide/mempool_lib.rst | 2 +- > > > lib/librte_eal/common/eal_common_lcore.c | 50 ++++++- > > > lib/librte_eal/common/eal_common_mcfg.c | 36 +++++ > > > lib/librte_eal/common/eal_common_thread.c | 33 +++++ > > > lib/librte_eal/common/eal_memcfg.h | 10 ++ > > > lib/librte_eal/common/eal_private.h | 18 +++ > > > lib/librte_eal/freebsd/eal.c | 4 + > > > lib/librte_eal/include/rte_lcore.h | 25 +++- > > > lib/librte_eal/linux/eal.c | 4 + > > > lib/librte_eal/rte_eal_version.map | 2 + > > > lib/librte_mempool/rte_mempool.h | 11 +- > > > 18 files changed, 349 insertions(+), 22 deletions(-) > > > create mode 100644 app/test/test_lcores.c > > > > > > > [...] > > > > > diff --git a/app/test/test_lcores.c b/app/test/test_lcores.c > > > new file mode 100644 > > > index 0000000000..864bcbade7 > > > --- /dev/null > > > +++ b/app/test/test_lcores.c > > > @@ -0,0 +1,139 @@ > > > +/* SPDX-License-Identifier: BSD-3-Clause > > > + * Copyright (c) 2020 Red Hat, Inc. > > > + */ > > > + > > > +#include > > > +#include > > > + > > > +#include > > > + > > > +#include "test.h" > > > + > > > +struct thread_context { > > > + enum { INIT, ERROR, DONE } state; > > > + bool lcore_id_any; > > > + pthread_t id; > > > + unsigned int *registered_count; > > > +}; > > > +static void *thread_loop(void *arg) > > > +{ > > > > missing an empty line here > > > > > + struct thread_context *t = arg; > > > + unsigned int lcore_id; > > > + > > > + lcore_id = rte_lcore_id(); > > > + if (lcore_id != LCORE_ID_ANY) { > > > + printf("Incorrect lcore id for new thread %u\n", lcore_id); > > > + t->state = ERROR; > > > + } > > > + rte_thread_register(); > > > + lcore_id = rte_lcore_id(); > > > + if ((t->lcore_id_any && lcore_id != LCORE_ID_ANY) || > > > + (!t->lcore_id_any && lcore_id == LCORE_ID_ANY)) { > > > + printf("Could not register new thread, got %u while %sexpecting %u\n", > > > + lcore_id, t->lcore_id_any ? "" : "not ", LCORE_ID_ANY); > > > + t->state = ERROR; > > > + } > > > > To check if rte_thread_register() succedeed, we need to look at > > lcore_id. I wonder if rte_thread_register() shouldn't return the lcore > > id on success, and -1 on error (rte_errno could be set to give some > > info on the error). > > lcore_id are unsigned integers with the special value LCORE_ID_ANY > mapped to UINT32_MAX (should be UINT_MAX? anyway...). > > rte_thread_register could return an error code as there are no ERROR > level logs about why a lcore allocation failed. > We could then distinguish a shortage of lcore (or init callback > refusal) from an invalid call before rte_eal_init() or when mp is in > use. > > About returning the lcore_id as part of the return code, this would > map to -1 for LCORE_ID_ANY. > This is probably not a problem but still odd. Yes, it would be a bit odd like this. What about changing the definition of LCORE_ID_ANY to ((unsigned int)-1) ? I think it does not change the effective value on any architecture, but would make the above change clearer. > > > > The same could be done for rte_thread_init() > > ? > Not sure where this one could fail. I was thinking about __rte_trace_mem_per_thread_alloc(), but maybe it's not needed. > > > > [...] > > > > > diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c > > > index a7ae0691bf..1cbddc4b5b 100644 > > > --- a/lib/librte_eal/common/eal_common_thread.c > > > +++ b/lib/librte_eal/common/eal_common_thread.c > > > @@ -236,3 +236,36 @@ rte_ctrl_thread_create(pthread_t *thread, const char *name, > > > pthread_join(*thread, NULL); > > > return -ret; > > > } > > > + > > > +void > > > +rte_thread_register(void) > > > +{ > > > + unsigned int lcore_id; > > > + rte_cpuset_t cpuset; > > > + > > > + /* EAL init flushes all lcores, we can't register before. */ > > > + assert(internal_config.init_complete == 1); > > > + if (pthread_getaffinity_np(pthread_self(), sizeof(cpuset), > > > + &cpuset) != 0) > > > + CPU_ZERO(&cpuset); > > > + lcore_id = eal_lcore_non_eal_allocate(); > > > + if (lcore_id >= RTE_MAX_LCORE) > > > + lcore_id = LCORE_ID_ANY; > > > + rte_thread_init(lcore_id, &cpuset); > > > + if (lcore_id != LCORE_ID_ANY) > > > + RTE_LOG(DEBUG, EAL, "Registered non-EAL thread as lcore %u.\n", > > > + lcore_id); > > > +} > > > > So, in this case, the affinity of the pthread is kept and saved, in other > > words there is no link between the lcore id and the affinity. It means we > > are allowing an application to register lcores for dataplane with conflicting > > affinities. > > This is not something new, applications using --lcores option already > live with this. > We have warnings in the documentation about non-EAL threads and about > the dangers of conflicting affinities. > Hopefully, the users of this API know what they are doing since they > chose not to use EAL threads. > > > > > > I wonder if it could be useful to have an API that automatically sets > > the affinity according to the lcore_id. Or a function that creates a > > pthread using the specified lcore id, and setting the correct affinity. > > I could simplify the work for applications that want to create/destroy > > dataplane threads dynamically. > > Do you mean EAL threads dynamic creation/suppression? > > > > > > This could be done later however, just an idea. > > For now, I don't see the need. > > > > > > [...] > > > diff --git a/lib/librte_eal/freebsd/eal.c b/lib/librte_eal/freebsd/eal.c > > > index 13e5de006f..32a3d999b8 100644 > > > --- a/lib/librte_eal/freebsd/eal.c > > > +++ b/lib/librte_eal/freebsd/eal.c > > > @@ -424,6 +424,10 @@ rte_config_init(void) > > > } > > > if (rte_eal_config_reattach() < 0) > > > return -1; > > > + if (!eal_mcfg_enable_multiprocess()) { > > > + RTE_LOG(ERR, EAL, "Primary process refused secondary attachment\n"); > > > + return -1; > > > + } > > > eal_mcfg_update_internal(); > > > break; > > > case RTE_PROC_AUTO: > > > diff --git a/lib/librte_eal/include/rte_lcore.h b/lib/librte_eal/include/rte_lcore.h > > > index 3968c40693..43747e88df 100644 > > > --- a/lib/librte_eal/include/rte_lcore.h > > > +++ b/lib/librte_eal/include/rte_lcore.h > > > @@ -31,6 +31,7 @@ enum rte_lcore_role_t { > > > ROLE_RTE, > > > ROLE_OFF, > > > ROLE_SERVICE, > > > + ROLE_NON_EAL, > > > }; > > > > If find the name ROLE_NON_EAL a bit heavy (this was also my impression > > when reading the doc part). > > > > I understand that there are several types of threads: > > > > - eal (pthread created by eal): ROLE_RTE and ROLE_SERVICE > > - unregistered (pthread not created by eal, and not registered): ROLE_OFF > > (note that ROLE_OFF also applies for unexistant threads) > > - dynamic: pthread not created by eal, but registered > > Last two cases both are non-EAL threads as described in the doc so far. Yes, but only the last case has the NON_EAL role. I feel that currently role != thread_type, so may be a bit confusing to use a pthread_type name in the role enum. > > > > > > What about using ROLE_DYN ? I'm not sure about this name either, it's just > > to open the discussion :) > > > > Well, at the moment, all those new lcores are mapped only to non-EAL threads. > A dynamic role feels like you want to take dynamic EAL threads into > account from the start. > I prefer to stick to non-EAL. > > > -- > David Marchand >