From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 06CA0A0547;
	Wed, 29 Sep 2021 17:24:14 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 7F4C5410EA;
	Wed, 29 Sep 2021 17:24:14 +0200 (CEST)
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
 by mails.dpdk.org (Postfix) with ESMTP id 6E145410E5;
 Wed, 29 Sep 2021 17:24:12 +0200 (CEST)
X-IronPort-AV: E=McAfee;i="6200,9189,10122"; a="221759714"
X-IronPort-AV: E=Sophos;i="5.85,332,1624345200"; d="scan'208";a="221759714"
Received: from fmsmga002.fm.intel.com ([10.253.24.26])
 by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
 29 Sep 2021 08:24:11 -0700
X-IronPort-AV: E=Sophos;i="5.85,332,1624345200"; d="scan'208";a="563373279"
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:24:09 -0700
Date: Wed, 29 Sep 2021 16:24:06 +0100
From: Bruce Richardson <bruce.richardson@intel.com>
To: Kevin Traynor <ktraynor@redhat.com>
Cc: dev@dpdk.org, ciara.power@intel.com, anatoly.burakov@intel.com,
 stable@dpdk.org, David Marchand <david.marchand@redhat.com>
Message-ID: <YVSFFpNsMMOSvpkn@bricha3-MOBL.ger.corp.intel.com>
References: <20210915141030.23514-1-bruce.richardson@intel.com>
 <20210924161842.2879019-1-bruce.richardson@intel.com>
 <6ea20d4e-a7dd-afcb-3ca1-ffc023114d72@redhat.com>
 <YVRq0ru/7zD1U13w@bricha3-MOBL.ger.corp.intel.com>
 <ced61c0d-bd81-30b1-ea10-39736cf2bfe0@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ced61c0d-bd81-30b1-ea10-39736cf2bfe0@redhat.com>
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 <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>

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?

Regards,
/Bruce