On Thu, Sep 18, 2014 at 04:01:32PM +0000, Richardson, Bruce wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman at tuxdriver.com]
> > Sent: Thursday, September 18, 2014 4:51 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 05:38:58PM +0200, Thomas Monjalon wrote:
> > > Hi Neil,
> > >
> > > 2014-09-18 11:03, Neil Horman:
> > > > 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.
> > >
> > > > --- a/config/common_linuxapp
> > > > +++ b/config/common_linuxapp
> > > > @@ -123,7 +123,7 @@ CONFIG_RTE_LOG_HISTORY=256
> > > > CONFIG_RTE_LIBEAL_USE_HPET=n
> > > > CONFIG_RTE_EAL_ALLOW_INV_SOCKET_ID=n
> > > > CONFIG_RTE_EAL_ALWAYS_PANIC_ON_ERROR=n
> > > > -CONFIG_RTE_EAL_IGB_UIO=y
> > > > +CONFIG_RTE_EAL_IGB_UIO=n
> > > > CONFIG_RTE_EAL_VFIO=y
> > >
> > > Hum, indeed this patch does a bit more ;)
> > >
> > Sorry, meant to clip that out, I was building without kernel headers, so I
> > had
> > to disable igb_uio and kni. I'll propose this patch properly if theres
> > consensus on the valid bits.
> > Neil
>
> If we get rid of the REFCNT compile-time option (but not the REFCNT_ATOMIC
> one), then we should be able to get rid of the union, as we can do the
> atomic/non-atomic entirely inside the refcnt manipulation routines (since a
> regular uint16_t can be typecast directly to an atomic form of the same).
> Once we get rid of the union we can get rid of the extra braces that go along
> with the union, and we can just have the static initialiser cleanly put in
> the code.
> Whaddaya think?
>
Yeah, that sounds good. Reduces the code complexity by a good amount, makes
configuration simpler and allows for easy static initalization.
> /Bruce
>
