Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6)

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Sep 18, 2014 at 04:16:00PM +0100, Bruce Richardson wrote:
> 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 <bruce.richardson@intel.com>
> > > > >
> > > > > Acked-by: Thomas Monjalon <thomas.monjalon@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?
We are, theres still an RTE_MBUF_REFCNT ifdef around the definition of the
refcnt variable.  The only change is that its consistently the same type
(rte_refcnt), its just the typedef that changes based on the build configuration
(which is in keeping with how the api implementation is toggled).

 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?
> 
I would strongly agree with this, I find it hard to fathom a situation in which
you don't want or need to refcount buffers.  I might be mistaken, but that seems
like folly.  Though I left that issue alone in this patch.

> 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.
> 
Its what you're documentation requires though (at least currently).  Certainly
check, but I would say the impact is zero, as the set routine is static inline,
and the assignment value is a constant, which means the compiler should be able
to optimize it to occur in parallel with the stack adjustment in the preamble.

> The simplest fix is probably just to add the braces, which isn't really much 
> of a big deal.
> 

Its duct tape.  Its a simple fix that seems like a great idea today,
specifically becuase its simple, but it will cause maintenence headaches in the
future, because you will constantly have to watch out in subsequent patch
submissions for someone out of hand just fixing what by all rights is a silly
extra set of braces that really shouldn't need to be there.

At the very least, consider this change because it means that you don't need to
sprinkle code if ifdef RTE_MBUF_REFCNT.  If you really feel like you need a
static initalizer, lets make something akin to linux's ATOMIC_INIT, in which you
can package the brace magic, or define to nothing if you don't compile
RTE_MBUF_REFCNT on.

Neil