Message ID | 20200706205234.8040-1-david.marchand@redhat.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 72995A00C5; Mon, 6 Jul 2020 22:53:06 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 746071DD84; Mon, 6 Jul 2020 22:52:59 +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 DBBE11DD73 for <dev@dpdk.org>; Mon, 6 Jul 2020 22:52:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594068776; 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: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=C2rVlB5pbg1xVMf/gWlD25SmlVY+4MMMuZsPJXfZ0/w=; b=LeeZBW+yOhzbXFn2BCljELucgrBlAVTC57SQcifC8rWUIQVMEd/u0ykFuQRABMIKbf0KVE KlYOkSv/s4iGbjJAkLH9HXe8VkZHAGSJrYFi5pc2YvisqFcMvVUpIgXq9KplTUAtqnmxgF A9PDL8vppw9cfEnK2mowTSEEbIy8erM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-399-7suJOjVHNCm42tZEvQc_zQ-1; Mon, 06 Jul 2020 16:52:48 -0400 X-MC-Unique: 7suJOjVHNCm42tZEvQc_zQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 30432EC1A4; Mon, 6 Jul 2020 20:52:46 +0000 (UTC) Received: from dmarchan.remote.csb (unknown [10.40.195.188]) by smtp.corp.redhat.com (Postfix) with ESMTP id C0B2B10013D7; Mon, 6 Jul 2020 20:52:41 +0000 (UTC) From: David Marchand <david.marchand@redhat.com> To: dev@dpdk.org Cc: jerinjacobk@gmail.com, bruce.richardson@intel.com, mdr@ashroe.eu, thomas@monjalon.net, arybchenko@solarflare.com, ktraynor@redhat.com, ian.stokes@intel.com, i.maximets@ovn.org, olivier.matz@6wind.com, konstantin.ananyev@intel.com Date: Mon, 6 Jul 2020 22:52:24 +0200 Message-Id: <20200706205234.8040-1-david.marchand@redhat.com> In-Reply-To: <20200610144506.30505-1-david.marchand@redhat.com> References: <20200610144506.30505-1-david.marchand@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=david.marchand@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8bit Subject: [dpdk-dev] [PATCH v6 00/10] Register non-EAL threads as lcore 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> |
Series |
Register non-EAL threads as lcore
|
|
Message
David Marchand
July 6, 2020, 8:52 p.m. UTC
OVS and some other applications have been hacking into DPDK internals to fake EAL threads and avoid performance penalty of only having non-EAL threads. This series proposes to add a new type of lcores and maps those threads to such lcores. non-EAL threads won't run the DPDK eal mainloop. As a consequence, part of the EAL threads API cannot work. Having new lcores appearing during the process lifetime is not expected by some DPDK components. This is addressed by introducing init/uninit callacks invoked when hotplugging of such lcore. There is still some work/discussion: - refuse new lcore role in incompatible EAL threads API (or document it only as those API were already incompatible?), - think about deprecation notices for existing RTE_FOREACH_LCORE macros and consorts, it is probably worth discussing on how to iterate over lcores, For the interested parties, I have a patch [1] against dpdk-latest OVS branch that makes use of this series (this patch probably won't work with v5, it will be rebased once dpdk side is ready). 1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200626123017.28555-1-david.marchand@redhat.com/ Changes since v5: - fixed windows build, Changes since v4: - added separate API to control mp feature activation, - addressed Konstantin and Olivier comments, Changes since v3: - added init failure when trying to use in conjunction with multiprocess, - addressed Andrew comments, Changes since v2: - fixed windows build error due to missing trace stub, - fixed bug when rolling back on lcore register, Changes since v1: - rebased on master (conflicts on merged Windows series), - separated lcore role code cleanup in a patch, - tried to use a single naming, so kept non-EAL threads as the main notion. non-EAL threads are then distinguished between registered and unregistered non-EAL threads, - added unit tests (still missing some coverage, marked with a FIXME), - reworked callbacks call under a common rwlock lock which protects lcores allocations and callbacks registration, - introduced lcore iterators and converted the bucket mempool driver,
Comments
Hi David, > OVS and some other applications have been hacking into DPDK internals to > fake EAL threads and avoid performance penalty of only having non-EAL > threads. > > This series proposes to add a new type of lcores and maps those threads > to such lcores. > non-EAL threads won't run the DPDK eal mainloop. > As a consequence, part of the EAL threads API cannot work. > > Having new lcores appearing during the process lifetime is not expected > by some DPDK components. This is addressed by introducing init/uninit > callacks invoked when hotplugging of such lcore. > > There is still some work/discussion: > - refuse new lcore role in incompatible EAL threads API (or document it > only as those API were already incompatible?), > - think about deprecation notices for existing RTE_FOREACH_LCORE macros > and consorts, it is probably worth discussing on how to iterate over > lcores, > > For the interested parties, I have a patch [1] against dpdk-latest OVS > branch that makes use of this series (this patch probably won't work with > v5, it will be rebased once dpdk side is ready). > > 1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200626123017.28555-1-david.marchand@redhat.com/ > > Changes since v5: > - fixed windows build, > > Changes since v4: > - added separate API to control mp feature activation, > - addressed Konstantin and Olivier comments, > > Changes since v3: > - added init failure when trying to use in conjunction with multiprocess, > - addressed Andrew comments, > > Changes since v2: > - fixed windows build error due to missing trace stub, > - fixed bug when rolling back on lcore register, > > Changes since v1: > - rebased on master (conflicts on merged Windows series), > - separated lcore role code cleanup in a patch, > - tried to use a single naming, so kept non-EAL threads as the main > notion. non-EAL threads are then distinguished between registered and > unregistered non-EAL threads, > - added unit tests (still missing some coverage, marked with a FIXME), > - reworked callbacks call under a common rwlock lock which protects > lcores allocations and callbacks registration, > - introduced lcore iterators and converted the bucket mempool driver, > LGTM, just 2 nits see below. Apart from that: Series Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com> 1. +void +rte_lcore_callback_unregister(void *handle) +{ + struct rte_config *cfg = rte_eal_get_configuration(); + struct lcore_callback *callback = handle; + unsigned int lcore_id; Seems like forgot to add formal parameter check here: if (callback == NULL) ... + + rte_rwlock_write_lock(&lcore_lock); + if (callback->uninit == NULL) 2. +bool +rte_mp_disable(void) +{ + return set_mp_status(MP_STATUS_DISABLED); +} Probably name it rte_eal_multiprocess_enable (or so) to make it clear from naming and follow more closely our own name convention. + +bool +eal_enable_multiprocess(void) +{ + return set_mp_status(MP_STATUS_ENABLED); +} Might be worth to make that function public too. Then user will have a proper pair to use: rte_eal_multiprocess_(enable|disable).
On Tue, Jul 7, 2020 at 1:23 AM Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote: > > OVS and some other applications have been hacking into DPDK internals to > > fake EAL threads and avoid performance penalty of only having non-EAL > > threads. > > > > This series proposes to add a new type of lcores and maps those threads > > to such lcores. > > non-EAL threads won't run the DPDK eal mainloop. > > As a consequence, part of the EAL threads API cannot work. > > > > Having new lcores appearing during the process lifetime is not expected > > by some DPDK components. This is addressed by introducing init/uninit > > callacks invoked when hotplugging of such lcore. > > > > There is still some work/discussion: > > - refuse new lcore role in incompatible EAL threads API (or document it > > only as those API were already incompatible?), > > - think about deprecation notices for existing RTE_FOREACH_LCORE macros > > and consorts, it is probably worth discussing on how to iterate over > > lcores, > > > > For the interested parties, I have a patch [1] against dpdk-latest OVS > > branch that makes use of this series (this patch probably won't work with > > v5, it will be rebased once dpdk side is ready). > > > > 1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200626123017.28555-1-david.marchand@redhat.com/ > > > > Changes since v5: > > - fixed windows build, > > > > Changes since v4: > > - added separate API to control mp feature activation, > > - addressed Konstantin and Olivier comments, > > > > Changes since v3: > > - added init failure when trying to use in conjunction with multiprocess, > > - addressed Andrew comments, > > > > Changes since v2: > > - fixed windows build error due to missing trace stub, > > - fixed bug when rolling back on lcore register, > > > > Changes since v1: > > - rebased on master (conflicts on merged Windows series), > > - separated lcore role code cleanup in a patch, > > - tried to use a single naming, so kept non-EAL threads as the main > > notion. non-EAL threads are then distinguished between registered and > > unregistered non-EAL threads, > > - added unit tests (still missing some coverage, marked with a FIXME), > > - reworked callbacks call under a common rwlock lock which protects > > lcores allocations and callbacks registration, > > - introduced lcore iterators and converted the bucket mempool driver, > > > > LGTM, just 2 nits see below. > Apart from that: > Series Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com> Thanks for the review. > > 1. > +void > +rte_lcore_callback_unregister(void *handle) > +{ > + struct rte_config *cfg = rte_eal_get_configuration(); > + struct lcore_callback *callback = handle; > + unsigned int lcore_id; > > Seems like forgot to add formal parameter check here: > if (callback == NULL) ... Indeed, fixed. > > + > + rte_rwlock_write_lock(&lcore_lock); > + if (callback->uninit == NULL) > > 2. > > +bool > +rte_mp_disable(void) > +{ > + return set_mp_status(MP_STATUS_DISABLED); > +} > > Probably name it rte_eal_multiprocess_enable (or so) > to make it clear from naming and follow > more closely our own name convention. > Apis for mp have the rte_mp_ prefix. lib/librte_eal/rte_eal_version.map: rte_mp_action_register; lib/librte_eal/rte_eal_version.map: rte_mp_action_unregister; lib/librte_eal/rte_eal_version.map: rte_mp_reply; lib/librte_eal/rte_eal_version.map: rte_mp_sendmsg; lib/librte_eal/rte_eal_version.map: rte_mp_request_async; lib/librte_eal/rte_eal_version.map: rte_mp_request_sync; I prefer to stick to it. > + > +bool > +eal_enable_multiprocess(void) > +{ > + return set_mp_status(MP_STATUS_ENABLED); > +} I will go with __rte_mp_ to indicate the private aspect. > > Might be worth to make that function public too. > Then user will have a proper pair to use: > rte_eal_multiprocess_(enable|disable). > I don't see the need for now. If you feel strong about it, I can send a followup patch later. Passed the checks again and pushed to the main branch.
On Mon, Jul 6, 2020 at 10:53 PM David Marchand <david.marchand@redhat.com> wrote: > > OVS and some other applications have been hacking into DPDK internals to > fake EAL threads and avoid performance penalty of only having non-EAL > threads. > > This series proposes to add a new type of lcores and maps those threads > to such lcores. > non-EAL threads won't run the DPDK eal mainloop. > As a consequence, part of the EAL threads API cannot work. > > Having new lcores appearing during the process lifetime is not expected > by some DPDK components. This is addressed by introducing init/uninit > callacks invoked when hotplugging of such lcore. > > There is still some work/discussion: > - refuse new lcore role in incompatible EAL threads API (or document it > only as those API were already incompatible?), > - think about deprecation notices for existing RTE_FOREACH_LCORE macros > and consorts, it is probably worth discussing on how to iterate over > lcores, > > For the interested parties, I have a patch [1] against dpdk-latest OVS > branch that makes use of this series (this patch probably won't work with > v5, it will be rebased once dpdk side is ready). > > 1: https://patchwork.ozlabs.org/project/openvswitch/patch/20200626123017.28555-1-david.marchand@redhat.com/ Series applied with last comments from Konstantin.