From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 560AFB3A3 for ; Thu, 18 Sep 2014 16:57:37 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1XUdEn-0002M3-1W; Thu, 18 Sep 2014 11:03:19 -0400 Date: Thu, 18 Sep 2014 11:03:12 -0400 From: Neil Horman To: "Richardson, Bruce" Message-ID: <20140918150312.GF20389@hmsreliant.think-freely.org> References: <1411037752-8000-1-git-send-email-bruce.richardson@intel.com> <5076244.KSjCyF24zI@xps13> <20140918122527.GE20389@hmsreliant.think-freely.org> <59AF69C657FD0841A61C55336867B5B0343F3406@IRSMSX103.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <59AF69C657FD0841A61C55336867B5B0343F3406@IRSMSX103.ger.corp.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] ixgbe: fix compile error with gcc4.4 (used RHEL 6) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Sep 2014 14:57:37 -0000 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 > > > > > > Acked-by: Thomas Monjalon > > > > > > 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);