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 1DE67A0547; Wed, 29 Sep 2021 17:31:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AA623410EA; Wed, 29 Sep 2021 17:31:42 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mails.dpdk.org (Postfix) with ESMTP id CBBAE410E5; Wed, 29 Sep 2021 17:31:40 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10122"; a="212221916" X-IronPort-AV: E=Sophos;i="5.85,332,1624345200"; d="scan'208";a="212221916" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2021 08:31:38 -0700 X-IronPort-AV: E=Sophos;i="5.85,332,1624345200"; d="scan'208";a="563376860" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.20.220]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 29 Sep 2021 08:31:36 -0700 Date: Wed, 29 Sep 2021 16:31:32 +0100 From: Bruce Richardson To: Kevin Traynor Cc: dev@dpdk.org, ciara.power@intel.com, anatoly.burakov@intel.com, stable@dpdk.org, David Marchand Message-ID: References: <20210915141030.23514-1-bruce.richardson@intel.com> <20210924161842.2879019-1-bruce.richardson@intel.com> <6ea20d4e-a7dd-afcb-3ca1-ffc023114d72@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v2] telemetry: fix "in-memory" process socket conflicts 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" On Wed, Sep 29, 2021 at 04:24:06PM +0100, Bruce Richardson wrote: > On Wed, Sep 29, 2021 at 03:54:48PM +0100, Kevin Traynor wrote: > > On 29/09/2021 14:32, Bruce Richardson wrote: > > > On Wed, Sep 29, 2021 at 01:28:53PM +0100, Kevin Traynor wrote: > > > > Hi Bruce, > > > > > > > > On 24/09/2021 17:18, Bruce Richardson wrote: > > > > > When DPDK is run with --in-memory mode, multiple processes can run > > > > > simultaneously using the same runtime dir. This leads to each process > > > > > removing another process' telemetry socket as it started up, giving > > > > > unexpected behaviour. > > > > > > > > > > This patch changes that behaviour to first check if the existing socket > > > > > is active. If not, it's an old socket to be cleaned up and can be > > > > > removed. If it is active, telemetry initialization fails and an error > > > > > message is printed out giving instructions on how to remove the error; > > > > > either by using file-prefix to have a different runtime dir (and > > > > > therefore socket path) or by disabling telemetry if it not needed. > > > > > > > > > > > > > telemetry is enabled by default but it may not be used by the application. > > > > Hitting this issue will cause rte_eal_init() to fail which will probably > > > > stop or severely limit the application. > > > > > > > > So it could change a working application to a non-working one (albeit one > > > > that doesn't interfere with other process' sockets). > > > > > > > > Can it just print a warning that telemetry will not be enabled and continue > > > > so it's not returning an rte_eal_init failure? > > > > > > > > > > For a backported fix, yes, that would probably be better behaviour, but for > > > the latest branch, I think returning error and having the user explicitly > > > choose the resolution they want to occur is best. I'll see about doing a > > > separate backport patch for 20.11. > > > > > > > But this is a runtime message dependent on runtime environment. The user may > > not have access or know how to change eal parameters. > > True. But on the other hand, this problem only occurs with non-default EAL > parameters anyway, so someone must have configured this with the > --in-memory flag. > > > > > In the case where the application doesn't care about telemetry, they have > > gone from not having telemetry to rte_eal_init() failing, which probably has > > severe consequence. > > > > Yes, I agree, which I why I would suggest that for any backport of this > fix, the error be made non-fatal as you suggest. [Having looked into it, > having it as a non-fatal error is rather awkward, so it may be best just > left unfixed and the current behaviour documented as known-issue]. > > However, for any application being updated and rebuilt against 21.11, I > would have thought it reasonable to flag this as an error, as any such > application would require revalidation anyway. > > > I could maybe agree if telemetry was default disable and the application had > > set the --telemetry flag indicating that they want/need it. As it is, it > > feels like it's possibly a worse outcome for the user. > > > > Perhaps, but I believe the only case of there being an issue would be where: > 1) a user who cannot modify the EAL parameters > 2) runs an application which has been updated and rebuilt against 21.11 > 3) where that application is hard-coded to use in-memory mode and > 4) has never been verified with two or more instances of that running? > Or am I missing something here? > Let me also go back to the drawing board on the solution here a bit, and see if I can come up with something better. If I can find a reasonable way to make it so that we can always create a socket in in-memory mode, despite other processes running, it would sidestep this problem completely. Not sure if it's possible, but let me see if I can come up with some ideas. [One idea I did try is using abstract sockets on linux, but with those we lose out on the permissions/protection we get from having a filesystem path, so were a no-go for me because of that] /Bruce