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 at tuxdriver.com] > > > Sent: Thursday, September 18, 2014 1:25 PM > > > To: Thomas Monjalon > > > Cc: Richardson, Bruce; dev at 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 <bruce.richardson at intel.com> > > > > > > > > Acked-by: Thomas Monjalon <thomas.monjalon at 6wind.com> > > > > > > > > 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
