From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id D8716D1EA for ; Fri, 24 Mar 2017 17:57:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1490374643; x=1521910643; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=R2HUfCg6CV4Q9F3Po8LrptF6R+zvD0DHqiNl+RG/kSU=; b=sNVPRfd+dro7ssNbaGAr1v08PDmm8ymE3Wy9ZoTBWtkPwctQz9jLYTO/ SMno/55ipJlDeW3Hl0H3wMWxDspR7g==; Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Mar 2017 09:57:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,215,1486454400"; d="scan'208";a="70351010" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.140]) by orsmga004.jf.intel.com with SMTP; 24 Mar 2017 09:57:19 -0700 Received: by (sSMTP sendmail emulation); Fri, 24 Mar 2017 16:57:19 +0000 Date: Fri, 24 Mar 2017 16:57:18 +0000 From: Bruce Richardson To: Olivier Matz Cc: Thomas Monjalon , dev@dpdk.org, jerin.jacob@caviumnetworks.com Message-ID: <20170324165718.GA17268@bricha3-MOBL3.ger.corp.intel.com> References: <20170223172407.27664-1-bruce.richardson@intel.com> <20170307113217.11077-1-bruce.richardson@intel.com> <20170307113217.11077-3-bruce.richardson@intel.com> <14749117.M51NuFqa78@xps13> <20170324145536.GA17076@bricha3-MOBL3.ger.corp.intel.com> <20170324174134.6aab6dfe@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170324174134.6aab6dfe@platinum> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.0 (2017-02-23) Subject: Re: [dpdk-dev] [PATCH v2 02/14] ring: create common structure for prod and cons metadata 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: , X-List-Received-Date: Fri, 24 Mar 2017 16:57:23 -0000 On Fri, Mar 24, 2017 at 05:41:34PM +0100, Olivier Matz wrote: > Hi Bruce, > > On Fri, 24 Mar 2017 14:55:36 +0000, Bruce Richardson wrote: > > On Wed, Mar 15, 2017 at 03:01:49PM +0100, Thomas Monjalon wrote: > > > clang error below: > > > > > > 2017-03-07 11:32, Bruce Richardson: > > > > + union { > > > > + uint32_t sp_enqueue; /**< True, if single producer. */ > > > > + uint32_t sc_dequeue; /**< True, if single consumer. */ > > > > + }; > > > > > > error: anonymous unions are a C11 extension > > > > Olivier, Thomas, feedback on suggestions for fixing this? Note: I'm > > still waiting to hear back on what compiler settings are needed to > > trigger this error. > > > > Two immediately obvious options: > > * replace the union with a single variable called e.g. "single", i.e. > > prod.single indicates single producer, and cons.single indicates > > single consumer. The downside of this approach is that it makes the > > patch a little bigger - as other code needs to be modified to use the > > new name - and is not backward compatible for apps which > > may reference this public structure memeber. > > * just remove the union without renaming anything, leaving two structure > > members called sp_enqueue and sc_dequeue. This uses a little more > > space in the structure, which is not a big deal since it needs to fill > > a cacheline anyway, but it is backward compatible in that no other > > code should need to be modified. > > > > Other options? My preference is for the first one. Given we are breaking > > the ring API anyway, I think we might as well use the shorter name and > > eliminate the need for the union, or multiple variables. > > What about adding RTE_STD_C11 like it's done in rte_mbuf? > > I didn't try, but since mbuf compiles, it should solve this issue in ring. > Yes, it might well. However, looking at the resulting code, I actually think it's cleaner to have just one variable called "single" in the structure. The union is really for backward compatibility, and there is little point in doing so since we are changing the rest of the structure in other ways. Struct now looks like: /* structure to hold a pair of head/tail values and other metadata */ struct rte_ring_headtail { volatile uint32_t head; /**< Prod/consumer head. */ volatile uint32_t tail; /**< Prod/consumer tail. */ uint32_t single; /**< True if single prod/cons */ }; And the code checks read e.g. for single producer: if (r->prod.single) /Bruce