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 CC628A0526; Wed, 8 Jul 2020 15:06:23 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 960051DB65; Wed, 8 Jul 2020 15:06:22 +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 7B3971DB57 for ; Wed, 8 Jul 2020 15:06:21 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594213581; 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=VUdmOCbBG/lqqYDiViMjG1p5FsIgijLOYSlMRC6S1ns=; b=hW/ICp6TQ5lf/ahEh/jRSC3qMfCh2C7Xs094glYwbhFUq+AiICkksKCB2akmdMraPN8ds0 Yz1iP3Bdt88VwseTfrOWuLGhhwnNLSAFKrQLADPETQQbe3vs9jGHVsdcnUCAMTtqWKZlHG sDJZ5ObeyE5+c0mL2D4dvPMdCY7kS8c= Received: from mail-vk1-f197.google.com (mail-vk1-f197.google.com [209.85.221.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-33-m5x5srxSPamlNMzOUAjYVA-1; Wed, 08 Jul 2020 09:06:05 -0400 X-MC-Unique: m5x5srxSPamlNMzOUAjYVA-1 Received: by mail-vk1-f197.google.com with SMTP id r80so9173237vkf.12 for ; Wed, 08 Jul 2020 06:06:05 -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=VUdmOCbBG/lqqYDiViMjG1p5FsIgijLOYSlMRC6S1ns=; b=YTowkX5UL53CnXzzF4iHwtDg/n6hqUtd5IuAuI1ceuFF/RvRJAE53ZsadRgmyVuIn1 pvdhX/F0Iw/gxU16B/H0phmQ+WVvJcoa6gxhcfw5J3AtGWwJ1eRz3PrSpEukAmWnOyo1 HxxlJ/XWTzP3i4VJAPvnHyL7KjRXROFcD9ennkEQ2wheDBUGSlroUvXlSIY2qcCsrr9t KL0gBxItT7ZlsVgMKXw+CmmXShCDHacH5A9VtnwyL3uVsL2bDcaMIB1bJFp5XlvG6qQN 1i5s9UrU4qHJHqj+uLNuMwKFEe1lLMDPD11nIFUm1Ts4dzpR248Vu3vm91IyJsxw8aAf PQ6A== X-Gm-Message-State: AOAM533bv4v+yinM9KNHN5SU/hLsgDlDIjXWBwcN95nj591bPCNkIl8L w8sTMmOBYhS4UBFLKC9XwtJEgKWR/PK/PPaqNlYVPlasKha1QtcBDYlgE+35BvYV3gtMcQePLNS At+4IcvlFIX3g59yW5AE= X-Received: by 2002:ab0:2c3:: with SMTP id 61mr40491863uah.87.1594213564898; Wed, 08 Jul 2020 06:06:04 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx+iZ5F/zO87rF8hzIAFE2N7ekh5K27F31fvRntLyPIfZaWbpk/2I+8DCqDi3XhV617wN4VEpPUeYTf5Aurx74= X-Received: by 2002:ab0:2c3:: with SMTP id 61mr40491760uah.87.1594213564020; Wed, 08 Jul 2020 06:06:04 -0700 (PDT) MIME-Version: 1.0 References: <20200610144506.30505-1-david.marchand@redhat.com> <20200706205234.8040-1-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Wed, 8 Jul 2020 15:05:52 +0200 Message-ID: To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" , "jerinjacobk@gmail.com" , "Richardson, Bruce" , "mdr@ashroe.eu" , "thomas@monjalon.net" , "arybchenko@solarflare.com" , "ktraynor@redhat.com" , "Stokes, Ian" , "i.maximets@ovn.org" , "olivier.matz@6wind.com" 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 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Tue, Jul 7, 2020 at 1:23 AM Ananyev, Konstantin 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 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. -- David Marchand