DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6)
Date: Thu, 18 Sep 2014 11:46:22 -0400
Message-ID: <20140918154622.GH20389@hmsreliant.think-freely.org> (raw)
In-Reply-To: <20140918151600.GA12120@BRICHA3-MOBL>

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

  reply	other threads:[~2014-09-18 15:40 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-18 10:55 Bruce Richardson
2014-09-18 11:09 ` Thomas Monjalon
2014-09-18 12:25   ` Neil Horman
2014-09-18 12:35     ` Richardson, Bruce
2014-09-18 15:03       ` Neil Horman
2014-09-18 15:16         ` Bruce Richardson
2014-09-18 15:46           ` Neil Horman [this message]
2014-09-18 15:38         ` Thomas Monjalon
2014-09-18 15:50           ` Neil Horman
2014-09-18 16:01             ` Richardson, Bruce
2014-09-18 18:12               ` Neil Horman
2014-09-25  2:13 ` Tang, HaifengX
2014-09-25  7:02   ` Thomas Monjalon
2014-09-25 13:07     ` Cao, Waterman
2014-09-25 15:05       ` [dpdk-dev] patches validation Thomas Monjalon
2014-09-25 23:29         ` Ananyev, Konstantin
2014-09-26  9:23           ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140918154622.GH20389@hmsreliant.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git