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 BB101428BA; Mon, 3 Apr 2023 19:35:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4ADD040A7E; Mon, 3 Apr 2023 19:35:06 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 5498D400D6 for ; Mon, 3 Apr 2023 19:35:05 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id 7341D210CB35; Mon, 3 Apr 2023 10:35:04 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 7341D210CB35 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1680543304; bh=0d4POzhMcz4ksECbs71iP6Bfz9DJVizHxBLbLj8xtYI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bnKUXF82COISiZ9fQdRhyAUPFIc8y4uFzJAGEC70buHv87Hlg+bUxwdjVOGMl/i7R upmTT0tRQWxreXajVZZujFF69L7+g47oy873XOYfdydx7mn64VeCy3XS2hLy2XBnwr OxhWU8oRo3zYVbQd/XuqrSpYwutEhTwGK+zG2Bko= Date: Mon, 3 Apr 2023 10:35:04 -0700 From: Tyler Retzlaff To: Bruce Richardson Cc: dev@dpdk.org, ciara.power@intel.com, david.marchand@redhat.com, thomas@monjalon.net Subject: Re: [PATCH 0/2] improve code portability Message-ID: <20230403173504.GB10756@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1680539424-20255-1-git-send-email-roretzla@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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 On Mon, Apr 03, 2023 at 06:04:08PM +0100, Bruce Richardson wrote: > On Mon, Apr 03, 2023 at 09:30:22AM -0700, Tyler Retzlaff wrote: > > Improve portability of telemetry code to allow it to be compiled by msvc > > unconditionally. > > > > Remove use of VLA and instead dynamically allocate. MSVC will never > > implement VLAs due to misuse / security concerns. > > > > Remove use of ranged based initializer (a gcc extension) instead just > > explicitly initialize individual array elements. > > > > Tyler Retzlaff (2): > > telemetry: use malloc instead of variable length array > > telemetry: use portable syntax to initialize array > > Bruce I was hoping to solicit your response specifically on this series so thanks. > Is this worth doing, given that DPDK telemetry uses a unix domain socket > for it's connectivity, which would not be available on windows anyway? > I don't particularly like these patches as: > * The removal of the VLAs means we will potentially be doing a *lot* of > malloc and free calls inside the telemetry code. It may not be a data > path or particularly performance critical, but I know for things like CPU > busyness, users may want to call telemetry functions hundreds (or > potentially thousands) of times a second. It also makes the code slightly > harder to read, and introduces the possibility of us having memory leaks. Yeah, malloc/free at high frequency is a bit ugly we could use alloca but I think equally as unsafe as VLAs. Is the preference here to just not compile any of this code for Windows regardless of compiler? If that is preferred I can look at refactoring to do that. Another option is I could use conditionally compiled code narrowly here using a Windows only API malloca which is vaguely safer? > * The second patch just makes the code uglier. True, it's non-standard, but > it really does make the code a whole lot more readable and managable. If > we need to make this standards-conforming, then I think we need to drop > the "const", and do runtime init of this array with loops for the ranges. I considered this. I thought the preference was to preserve const but if initialization in a loop is preferrable i'll do that instead. > > All that said, if we do have a path to get telemetry working on windows, I > think we can work together to get a suitable patchset in for it. It's unfortunate that EAL depends on telemetry but it doesn't work on Windows (right now). Telemetry is unfortunately not of much value if the code doesn't build and run so thus a lower priority. What I can ask for here (if the community accepts) is getting the code to compile, that's what unblocks progress for now and we can re-visit making telemetry work properly ~later. Thanks > > /Bruce