From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6) Date: Thu, 18 Sep 2014 16:16:00 +0100 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 Cc: "dev-VfR2kkLFssw@public.gmane.org" To: Neil Horman Return-path: Content-Disposition: inline In-Reply-To: <20140918150312.GF20389-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" 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-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org] > > > Sent: Thursday, September 18, 2014 1:25 PM > > > To: Thomas Monjalon > > > Cc: Richardson, Bruce; dev-VfR2kkLFssw@public.gmane.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