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

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

[Bruce Richardson]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
index a6f7fdf..203ddf7 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
@@ -723,7 +723,7 @@ ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq)
 		.nb_segs = 1,
 		.data_off = RTE_PKTMBUF_HEADROOM,
 #ifdef RTE_MBUF_REFCNT
-		.refcnt = 1,
+		{ .refcnt = 1, }
 #endif
 	};
 
-- 
1.9.3

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

[Thomas Monjalon]

> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>

Thanks Bruce, it is now applied.
-- 
Thomas

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

[Neil Horman]

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> 
> 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?

Neil

> -- 
> Thomas
> 

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

[Richardson, Bruce]

> -----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 <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> >
> > Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> >
> > 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

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

[Neil Horman]

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 <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > >
> > > Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > >
> > > 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?


diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 66bcbc5..5bd634e 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -760,14 +760,14 @@ test_failing_mbuf_sanity_check(void)
 
 #ifdef RTE_MBUF_REFCNT
 	badbuf = *buf;
-	badbuf.refcnt = 0;
+	rte_mbuf_refcnt_set(&badbuf, 0);
 	if (verify_mbuf_check_panics(&badbuf)) {
 		printf("Error with bad-refcnt(0) mbuf test\n");
 		return -1;
 	}
 
 	badbuf = *buf;
-	badbuf.refcnt = UINT16_MAX;
+	rte_mbuf_refcnt_set(&badbuf, UINT16_MAX);
 	if (verify_mbuf_check_panics(&badbuf)) {
 		printf("Error with bad-refcnt(MAX) mbuf test\n");
 		return -1;
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 5bee910..a001231 100644
--- 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
 
 #
@@ -381,7 +381,7 @@ CONFIG_RTE_LIBRTE_PIPELINE=y
 #
 # Compile librte_kni
 #
-CONFIG_RTE_LIBRTE_KNI=y
+CONFIG_RTE_LIBRTE_KNI=n
 CONFIG_RTE_KNI_KO_DEBUG=n
 CONFIG_RTE_KNI_VHOST=n
 CONFIG_RTE_KNI_VHOST_MAX_CACHE_SIZE=1024
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 1c6e115..3fca0fb 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -120,6 +120,15 @@ extern "C" {
 typedef void    *MARKER[0];   /**< generic marker for a point in a structure */
 typedef uint64_t MARKER64[0]; /**< marker that allows us to overwrite 8 bytes
                                * with a single assignment */
+
+#ifdef RTE_MBUF_REFCNT
+#ifdef RTE_MBUF_REFCNT_ATOMIC
+typedef rte_atomic16_t rte_refcnt;
+#else
+typedef uint16_t rte_refcnt;
+#endif
+#endif
+
 /**
  * The generic rte_mbuf, containing a packet mbuf.
  */
@@ -142,13 +151,9 @@ struct rte_mbuf {
 	 * or non-atomic) is controlled by the CONFIG_RTE_MBUF_REFCNT_ATOMIC
 	 * config option.
 	 */
-	union {
 #ifdef RTE_MBUF_REFCNT
-		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
-		uint16_t refcnt;              /**< Non-atomically accessed refcnt */
+	rte_refcnt refcnt;
 #endif
-		uint16_t refcnt_reserved;     /**< Do not use this field */
-	};
 	uint8_t nb_segs;          /**< Number of segments. */
 	uint8_t port;             /**< Input port. */
 
@@ -250,6 +255,7 @@ if (!(exp)) {                                                        \
 #ifdef RTE_MBUF_REFCNT
 #ifdef RTE_MBUF_REFCNT_ATOMIC
 
+
 /**
  * Adds given value to an mbuf's refcnt and returns its new value.
  * @param m
@@ -262,7 +268,7 @@ if (!(exp)) {                                                        \
 static inline uint16_t
 rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 {
-	return (uint16_t)(rte_atomic16_add_return(&m->refcnt_atomic, value));
+	return (uint16_t)(rte_atomic16_add_return(&m->refcnt, value));
 }
 
 /**
@@ -275,7 +281,7 @@ rte_mbuf_refcnt_update(struct rte_mbuf *m, int16_t value)
 static inline uint16_t
 rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 {
-	return (uint16_t)(rte_atomic16_read(&m->refcnt_atomic));
+	return (uint16_t)(rte_atomic16_read(&m->refcnt));
 }
 
 /**
@@ -288,7 +294,7 @@ rte_mbuf_refcnt_read(const struct rte_mbuf *m)
 static inline void
 rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 {
-	rte_atomic16_set(&m->refcnt_atomic, new_value);
+	rte_atomic16_set(&m->refcnt, new_value);
 }
 
 #else /* ! RTE_MBUF_REFCNT_ATOMIC */
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
index 203ddf7..4a80224 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
@@ -722,11 +722,9 @@ ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq)
 	static struct rte_mbuf mb_def = {
 		.nb_segs = 1,
 		.data_off = RTE_PKTMBUF_HEADROOM,
-#ifdef RTE_MBUF_REFCNT
-		{ .refcnt = 1, }
-#endif
 	};
 
+	rte_mbuf_refcnt_set(&mb_def, 1);
 	mb_def.buf_len = rxq->mb_pool->elt_size - sizeof(struct rte_mbuf);
 	mb_def.port = rxq->port_id;
 	rxq->mbuf_initializer = *((uint64_t *)&mb_def.rearm_data);

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

[Bruce Richardson]

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 <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > >
> > > > Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > > >
> > > > 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

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

[Neil Horman]

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-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 <bruce.richardson-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > >
> > > > > Acked-by: Thomas Monjalon <thomas.monjalon-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>
> > > > >
> > > > > 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

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

[Thomas Monjalon]

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 ;)

[...]
> -CONFIG_RTE_LIBRTE_KNI=y
> +CONFIG_RTE_LIBRTE_KNI=n
>  CONFIG_RTE_KNI_KO_DEBUG=n

> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -120,6 +120,15 @@ extern "C" {
>  typedef void    *MARKER[0];   /**< generic marker for a point in a
> structure */ typedef uint64_t MARKER64[0]; /**< marker that allows us to
> overwrite 8 bytes * with a single assignment */
> +
> +#ifdef RTE_MBUF_REFCNT
> +#ifdef RTE_MBUF_REFCNT_ATOMIC
> +typedef rte_atomic16_t rte_refcnt;
> +#else
> +typedef uint16_t rte_refcnt;
> +#endif
> +#endif
> +
>  /**
>   * The generic rte_mbuf, containing a packet mbuf.
>   */
> @@ -142,13 +151,9 @@ struct rte_mbuf {
>  	 * or non-atomic) is controlled by the CONFIG_RTE_MBUF_REFCNT_ATOMIC
>  	 * config option.
>  	 */
> -	union {
>  #ifdef RTE_MBUF_REFCNT
> -		rte_atomic16_t refcnt_atomic; /**< Atomically accessed refcnt */
> -		uint16_t refcnt;              /**< Non-atomically accessed refcnt */
> +	rte_refcnt refcnt;
>  #endif
> -		uint16_t refcnt_reserved;     /**< Do not use this field */
> -	};

You are changing the mbuf alignment if MBUF_REFCNT is disabled.

> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> @@ -722,11 +722,9 @@ ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq)
>  	static struct rte_mbuf mb_def = {
>  		.nb_segs = 1,
>  		.data_off = RTE_PKTMBUF_HEADROOM,
> -#ifdef RTE_MBUF_REFCNT
> -		{ .refcnt = 1, }
> -#endif
>  	};
> 
> +	rte_mbuf_refcnt_set(&mb_def, 1);

Performance impact must be checked here.

-- 
Thomas

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

[Neil Horman]

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

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

[Richardson, Bruce]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org]
> Sent: Thursday, September 18, 2014 4:51 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 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?

/Bruce

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

[Neil Horman]

On Thu, Sep 18, 2014 at 04:01:32PM +0000, Richardson, Bruce wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org]
> > Sent: Thursday, September 18, 2014 4:51 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 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
> 

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

[Tang, HaifengX]

Tested-by: Haifeng Tang<haifengx.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch just  includes one file, and has been tested by Intel.
Please see the detail information from the attachment.


-----Original Message-----
From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Bruce Richardson
Sent: Thursday, September 18, 2014 6:56 PM
To: dev-VfR2kkLFssw@public.gmane.org
Subject: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6)

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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
index a6f7fdf..203ddf7 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
@@ -723,7 +723,7 @@ ixgbe_rxq_vec_setup(struct igb_rx_queue *rxq)
 		.nb_segs = 1,
 		.data_off = RTE_PKTMBUF_HEADROOM,
 #ifdef RTE_MBUF_REFCNT
-		.refcnt = 1,
+		{ .refcnt = 1, }
 #endif
 	};
 
--
1.9.3

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

[Thomas Monjalon]

2014-09-25 02:13, Tang, HaifengX:
> Tested-by: Haifeng Tang<haifengx.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch just  includes one file, and has been tested by Intel.
> Please see the detail information from the attachment.

Attachment is filtered out. Please do not try to attach some files for
the mailing list.

By the way, this patch has already been integrated. So this test report
is not really useful anymore.

-- 
Thomas

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

[Cao, Waterman]

Hi Thomas,

 I will work with team to see if we can improve test report.
 Because intel validation team will continue to upgrade test cases to verify feature,
 I think that it's still worth to verify patch or features even it has already integrated branch.

 Thanks
Waterman 

>-----Original Message-----
>From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Thomas Monjalon
>Sent: Thursday, September 25, 2014 3:02 PM
>To: Tang, HaifengX
>Cc: dev-VfR2kkLFssw@public.gmane.org
>Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6)
>
>2014-09-25 02:13, Tang, HaifengX:
>> Tested-by: Haifeng Tang<haifengx.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> 
>> This patch just  includes one file, and has been tested by Intel.
>> Please see the detail information from the attachment.
>
>Attachment is filtered out. Please do not try to attach some files for the mailing list.
>
>By the way, this patch has already been integrated. So this test report is not really useful anymore.
>
>--
>Thomas

Re: patches validation

[Thomas Monjalon]

2014-09-25 13:07, Cao, Waterman:
>  I will work with team to see if we can improve test report.
>  Because intel validation team will continue to upgrade test cases to verify feature,
>  I think that it's still worth to verify patch or features even it has already integrated branch.

Of course, it's important to continue validation after integration.
But please do not send test report on the list for patches which are
already integrated, except for 2 cases:
1) there is an error
2) this is a new feature and you want to explain how to test it
(btw, how do you test "zero copy" and "one copy" for virtio?)

About report content, please add these informations:
- commit id or tag used as a base to apply the patch
- tools used for the test (testpmd, sample, qemu, etc)
- command parameters if relevant
- test topology if relevant

If someone think about an useful information I missed, please share it.

We could write some guidelines for test and bug reports and publish it
on the website. Drafts are welcome.

Thanks
-- 
Thomas

Re: patches validation

[Ananyev, Konstantin]

> From: dev [mailto:dev-bounces-VfR2kkLFssw@public.gmane.org] On Behalf Of Thomas Monjalon
> Sent: Thursday, September 25, 2014 4:05 PM
> To: Cao, Waterman
> Cc: dev-VfR2kkLFssw@public.gmane.org
> Subject: Re: [dpdk-dev] patches validation
> 
> 2014-09-25 13:07, Cao, Waterman:
> >  I will work with team to see if we can improve test report.
> >  Because intel validation team will continue to upgrade test cases to verify feature,
> >  I think that it's still worth to verify patch or features even it has already integrated branch.
> 
> Of course, it's important to continue validation after integration.
> But please do not send test report on the list for patches which are
> already integrated, except for 2 cases:
> 1) there is an error
> 2) this is a new feature and you want to explain how to test it
> (btw, how do you test "zero copy" and "one copy" for virtio?)
> 
> About report content, please add these informations:
> - commit id or tag used as a base to apply the patch
> - tools used for the test (testpmd, sample, qemu, etc)
> - command parameters if relevant
> - test topology if relevant
> 
> If someone think about an useful information I missed, please share it.

May be it is just me, but what's wrong with mail for every tested patch?
At least it makes easy to check was the patch formally validated or not - all you have to do - grep through mail archives.
Konstantin

> We could write some guidelines for test and bug reports and publish it
> on the website. Drafts are welcome.
> 
> Thanks
> --
> Thomas

Re: patches validation

[Thomas Monjalon]

2014-09-25 23:29, Ananyev, Konstantin:
> From: Thomas Monjalon
> > 2014-09-25 13:07, Cao, Waterman:
> > >  I will work with team to see if we can improve test report.
> > >  Because intel validation team will continue to upgrade test cases to verify feature,
> > >  I think that it's still worth to verify patch or features even it has already integrated branch.
> > 
> > Of course, it's important to continue validation after integration.
> > But please do not send test report on the list for patches which are
> > already integrated, except for 2 cases:
> > 1) there is an error
> > 2) this is a new feature and you want to explain how to test it
> > (btw, how do you test "zero copy" and "one copy" for virtio?)
> > 
> > About report content, please add these informations:
> > - commit id or tag used as a base to apply the patch
> > - tools used for the test (testpmd, sample, qemu, etc)
> > - command parameters if relevant
> > - test topology if relevant
> > 
> > If someone think about an useful information I missed, please share it.
> 
> May be it is just me, but what's wrong with mail for every tested patch?
> At least it makes easy to check was the patch formally validated or not
> - all you have to do - grep through mail archives.

The right place to check something about a patch is the git history.
So it's important to send test reports before having it integrated in git.
Doing so, without any reference to commit id, imply that the patch is pending.
If you think it's really important to send test report about an integrated
patch, the commit id must be clearly visible to quickly understand its status.
There is something else wrong about these test reports: there is no useful
information about how to reproduce the test.
So it's not forbidden to send any email you want but please try to be more
informative and easy to understand. We are getting a huge email traffic so
everyone must be concerned about how to make it effective.

Thanks
-- 
Thomas