From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 80FA4B3AA for ; Thu, 18 Sep 2014 17:11:23 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 18 Sep 2014 08:16:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,548,1406617200"; d="scan'208";a="593304974" Received: from bricha3-mobl.ger.corp.intel.com (HELO bricha3-mobl.ir.intel.com) ([10.237.220.58]) by fmsmga001.fm.intel.com with SMTP; 18 Sep 2014 08:16:01 -0700 Received: by bricha3-mobl.ir.intel.com (sSMTP sendmail emulation); Thu, 18 Sep 2014 16:16:00 +0001 Date: Thu, 18 Sep 2014 16:16:00 +0100 From: Bruce Richardson To: Neil Horman Message-ID: <20140918151600.GA12120@BRICHA3-MOBL> References: <1411037752-8000-1-git-send-email-bruce.richardson@intel.com> <5076244.KSjCyF24zI@xps13> <20140918122527.GE20389@hmsreliant.think-freely.org> <59AF69C657FD0841A61C55336867B5B0343F3406@IRSMSX103.ger.corp.intel.com> <20140918150312.GF20389@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20140918150312.GF20389@hmsreliant.think-freely.org> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.22 (2013-10-16) Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Sep 2014 15:11:24 -0000 On Thu, Sep 18, 2014 at 11:03:12AM -0400, Neil Horman wrote: > On Thu, Sep 18, 2014 at 12:35:21PM +0000, Richardson, Bruce wrote: > > > -----Original Message----- > > > From: Neil Horman [mailto:nhorman@tuxdriver.com] > > > Sent: Thursday, September 18, 2014 1:25 PM > > > To: Thomas Monjalon > > > Cc: Richardson, Bruce; dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL > > > 6) > > > > > > On Thu, Sep 18, 2014 at 01:09:16PM +0200, Thomas Monjalon wrote: > > > > > The refcnt field is contained within an anonymous union within the mbuf > > > > > data structure, and gcc 4.4 gives an error about an unknown field unless > > > > > the initialiser for the field is contained within extra braces. > > > > > > > > > > Signed-off-by: Bruce Richardson > > > > > > > > Acked-by: Thomas Monjalon > > > > > > > > Thanks Bruce, it is now applied. > > > > > > Hang on here, we use anonymous unions all the time in RHEL6, and make > > > assignments to them frequently, and the compiler doesn't complain (see the > > > dropcount variable in sk_buff for an example). Not saying that this is a big > > > deal, but can you explain a little more about what you're seeing when this error > > > occurs, before we just paper over it? > > > > > > > Originally reported on RHEL6 as a build failure. When I use gcc4.4 on Fedora 20, I get the following without this change: > > > > CC ixgbe_rxtx_vec.o > > == Build lib/librte_table > > /home/bruce/dpdk.org/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c: In function 'ixgbe_rxq_vec_setup': > > /home/bruce/dpdk.org/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c:726: error: unknown field 'refcnt' specified in initializer > > compilation terminated due to -Wfatal-errors. > > make[5]: *** [ixgbe_rxtx_vec.o] Error 1 > > make[4]: *** [librte_pmd_ixgbe] Error 2 > > make[4]: *** Waiting for unfinished jobs.... > > make[3]: *** [lib] Error 2 > > make[2]: *** [all] Error 2 > > make[1]: *** [x86_64-native-linuxapp-gcc_install] Error 2 > > make: *** [install] Error 2 > > > > Regards, > > /Bruce > > > > > Thank you, I've reproduced the problem here, and you're right, it is a compiler > bug, but I really hate the idea of just throwing braces around something to shut > the compiler up, especially when the compiler has since been fixed. Looking at > it a bit more closely it seems like the the thing to do is actually just > consistently use rte_mbuf_refcnt_set, since thats what the rte_mbuf header > documentation says to do, and protects you if the internal structure changes, as > well as prevents you from having to spread ifdef RTE_MBUF_REFCNT all over the > place. > > This patch does a bit more than that too. With a little creative typedef-ing we > don't need the anonymous union at all, and lets us just use a single refcnt > variable, and I think eliminate that odd refcnt_reserved member of the union, > that, as far as I can see, does nothing. > > Thoughts? > I like the idea of using a typedef, though are we not adjusting the mbuf layout if the refcount is disabled? A follow-on question would also be - do we really need an option to disable the reference count, do we not always just leave it on? As for using refcnt_set - that would be a solution that would work too, but that approach needs to be checked for performance impacts as we are going from a constant initializer value to having an extra assignment instruction in the fast-path. The simplest fix is probably just to add the braces, which isn't really much of a big deal. /Bruce