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 CD39BA0350; Wed, 1 Jul 2020 09:13:53 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A82C31C10F; Wed, 1 Jul 2020 09:13:53 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by dpdk.org (Postfix) with ESMTP id DDCB81C0C3 for ; Wed, 1 Jul 2020 09:13:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1593587631; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=jKbAE69s7jxaImu0PChpfWTyEwPQpPmCglqbdMjAoks=; b=OP5cYg58jEb7otWl63CQe9bl+qOx2q84/MOgmfzZg/YJl9v6H2rOO+1Ts0+6N/7Gc6zZ// dvunTAGAoFyZlRW+k4J7IBEffqyhZj9lpnw6JkjyNqauCc5LmsT3sDdGAItB1DMpneJnNA FjUvimX6g1d6ETiI6nr8ujNAFLocpys= Received: from mail-vk1-f199.google.com (mail-vk1-f199.google.com [209.85.221.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-409-G_pOfUf3Od2wi-kkzLBJ1Q-1; Wed, 01 Jul 2020 03:13:49 -0400 X-MC-Unique: G_pOfUf3Od2wi-kkzLBJ1Q-1 Received: by mail-vk1-f199.google.com with SMTP id l8so4015210vkm.10 for ; Wed, 01 Jul 2020 00:13:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jKbAE69s7jxaImu0PChpfWTyEwPQpPmCglqbdMjAoks=; b=UqMfY9qv2xflamcWq4e8/RGfLGaEhZaWQ/x60bzdgK8ajZQblPYvhaZiqPQxQ+SGlZ z+WjabfBrlG7jhYmpybYQyD+j4efxyFOdIKnuDkaVyaEL1FJCQoB19dfoTNLzsJjiHpo WXRyBuwdNOMoo5ywUlTnA13Odmasyh/FYsZT5qDxk14cF+/eBTF/Zb645pzoCMdJcAzm VJFv4kgruEhBiIBULuI1Y1LXL59W3mWOv/Fa9546ZdbjrzaETn4pN0T02TuNMa1wQkp3 aFlWIRM57bxTDNoadUXvL5o+WYBwUeGRTSoNPNHSvTee8dMTXKvUwNrH3v/X7jYHe2Cq kgDQ== X-Gm-Message-State: AOAM531M9tQElp+WoDLZuWrhEH2WPrybfBUKjTYhj6uo2DoeXUvAcc+a HzMqOWmyQgrjKlgwWpkUIO9m/xWcKyS+cNzdR5p3L3IWcE9s/2t+Sw+twWzpJZ0NA/yKm8/2c2L PlGse0srmWDAppu5q7ZM= X-Received: by 2002:ab0:316:: with SMTP id 22mr17826969uat.41.1593587629105; Wed, 01 Jul 2020 00:13:49 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxLrEZN0S4MlmzDIj2ZdXRbxbB3Ic1DwrKwoN3JY6Ggkrqt6Evqz1yGehAdSnl5TTZkzFN9QpV5vDNzUL9skSw= X-Received: by 2002:ab0:316:: with SMTP id 22mr17826944uat.41.1593587628690; Wed, 01 Jul 2020 00:13:48 -0700 (PDT) MIME-Version: 1.0 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> In-Reply-To: <20200630100717.GF5869@platinum> From: David Marchand Date: Wed, 1 Jul 2020 09:13:36 +0200 Message-ID: To: Olivier Matz 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 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" 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 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. > > The same could be done for rte_thread_init() ? Not sure where this one could fail. > > [...] > > > 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. > > 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