diff mbox

[dpdk-dev,RFC,01/14] mbuf: rename RTE_MBUF_SCATTER_GATHER into RTE_MBUF_REFCNT

Message ID 1407789890-17355-2-git-send-email-bruce.richardson@intel.com (mailing list archive)
State RFC, archived
Headers

Commit Message

Bruce Richardson Aug. 11, 2014, 8:44 p.m. UTC 
  From: Olivier Matz <olivier.matz@6wind.com>

It seems that RTE_MBUF_SCATTER_GATHER is not the proper name for the
feature it provides. "Scatter gather" means that data is stored using
several buffers. RTE_MBUF_REFCNT seems to be a better name for that
feature as it provides a reference counter for mbufs.

The macro RTE_MBUF_SCATTER_GATHER is poisoned to ensure this
modification is seen by drivers or applications using it.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_mbuf.c                  | 16 ++++++++--------
 config/common_bsdapp                  |  2 +-
 config/common_linuxapp                |  2 +-
 doc/doxy-api.conf                     |  2 +-
 examples/ip_pipeline/main.c           |  2 +-
 examples/ipv4_multicast/Makefile      |  4 ++--
 examples/vhost/main.c                 |  4 ++--
 lib/librte_mbuf/rte_mbuf.c            |  2 +-
 lib/librte_mbuf/rte_mbuf.h            | 35 +++++++++++++++++++----------------
 lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c |  4 ++--
 10 files changed, 38 insertions(+), 35 deletions(-)

Comments

Stephen Hemminger Aug. 11, 2014, 9:45 p.m. UTC | #1 
On Mon, 11 Aug 2014 21:44:37 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> From: Olivier Matz <olivier.matz@6wind.com>
> 
> It seems that RTE_MBUF_SCATTER_GATHER is not the proper name for the
> feature it provides. "Scatter gather" means that data is stored using
> several buffers. RTE_MBUF_REFCNT seems to be a better name for that
> feature as it provides a reference counter for mbufs.
> 
> The macro RTE_MBUF_SCATTER_GATHER is poisoned to ensure this
> modification is seen by drivers or applications using it.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Olivier Matz Aug. 12, 2014, 7:59 a.m. UTC | #2 
Hi Bruce,

On 08/11/2014 11:45 PM, Stephen Hemminger wrote:
> On Mon, 11 Aug 2014 21:44:37 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
>
>> From: Olivier Matz <olivier.matz@6wind.com>
>>
>> It seems that RTE_MBUF_SCATTER_GATHER is not the proper name for the
>> feature it provides. "Scatter gather" means that data is stored using
>> several buffers. RTE_MBUF_REFCNT seems to be a better name for that
>> feature as it provides a reference counter for mbufs.
>>
>> The macro RTE_MBUF_SCATTER_GATHER is poisoned to ensure this
>> modification is seen by drivers or applications using it.
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

After applying this first patch, I still get references to
"scatter gather":

$ git grep RTE_MBUF_SCATTER_GATHER
examples/Makefile:DIRS-$(CONFIG_RTE_MBUF_SCATTER_GATHER) += ip_fragmentation
examples/Makefile:DIRS-$(CONFIG_RTE_MBUF_SCATTER_GATHER) += ipv4_multicast
examples/ip_fragmentation/Makefile:ifneq 
($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
examples/ip_fragmentation/Makefile:$(error This application requires 
RTE_MBUF_SCATTER_GATHER to be enabled)
examples/ip_pipeline/Makefile:ifeq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
lib/librte_mbuf/rte_mbuf.h:#pragma GCC poison RTE_MBUF_SCATTER_GATHER
lib/librte_port/Makefile:ifeq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
lib/librte_port/Makefile:ifeq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)

Olivier
Bruce Richardson Aug. 12, 2014, 4:25 p.m. UTC | #3 
Ok, thanks, I'll see about fixing those.
I see a number of comments about the format and structure of the patch set itself. I'll take those all on board, but I'll admit that I didn't rework the patchset much before submitting it as an RFC. I'm leaving that until I've finished on this and ready to start submitting non-RFC patchset for merge. Right now my primary concern is whether the reworked struct rte_mbuf has everything we need, and whether there are any performance regressions we need to fix due to the second cache line. 

Regards,
/Bruce

> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, August 12, 2014 1:00 AM
> To: Richardson, Bruce
> Cc: Stephen Hemminger; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC PATCH 01/14] mbuf: rename
> RTE_MBUF_SCATTER_GATHER into RTE_MBUF_REFCNT
> 
> Hi Bruce,
> 
> On 08/11/2014 11:45 PM, Stephen Hemminger wrote:
> > On Mon, 11 Aug 2014 21:44:37 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >
> >> From: Olivier Matz <olivier.matz@6wind.com>
> >>
> >> It seems that RTE_MBUF_SCATTER_GATHER is not the proper name for the
> >> feature it provides. "Scatter gather" means that data is stored using
> >> several buffers. RTE_MBUF_REFCNT seems to be a better name for that
> >> feature as it provides a reference counter for mbufs.
> >>
> >> The macro RTE_MBUF_SCATTER_GATHER is poisoned to ensure this
> >> modification is seen by drivers or applications using it.
> >>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> After applying this first patch, I still get references to
> "scatter gather":
> 
> $ git grep RTE_MBUF_SCATTER_GATHER
> examples/Makefile:DIRS-$(CONFIG_RTE_MBUF_SCATTER_GATHER) +=
> ip_fragmentation
> examples/Makefile:DIRS-$(CONFIG_RTE_MBUF_SCATTER_GATHER) +=
> ipv4_multicast
> examples/ip_fragmentation/Makefile:ifneq
> ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
> examples/ip_fragmentation/Makefile:$(error This application requires
> RTE_MBUF_SCATTER_GATHER to be enabled)
> examples/ip_pipeline/Makefile:ifeq
> ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
> lib/librte_mbuf/rte_mbuf.h:#pragma GCC poison RTE_MBUF_SCATTER_GATHER
> lib/librte_port/Makefile:ifeq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
> lib/librte_port/Makefile:ifeq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
> 
> Olivier
diff mbox

Patch

diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 2b87521..678c382 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -82,7 +82,7 @@ 
 static struct rte_mempool *pktmbuf_pool = NULL;
 static struct rte_mempool *ctrlmbuf_pool = NULL;
 
-#if defined RTE_MBUF_SCATTER_GATHER  && defined RTE_MBUF_REFCNT_ATOMIC
+#if defined RTE_MBUF_REFCNT  && defined RTE_MBUF_REFCNT_ATOMIC
 
 static struct rte_mempool *refcnt_pool = NULL;
 static struct rte_ring *refcnt_mbuf_ring = NULL;
@@ -365,7 +365,7 @@  fail:
 static int
 testclone_testupdate_testdetach(void)
 {
-#ifndef RTE_MBUF_SCATTER_GATHER
+#ifndef RTE_MBUF_REFCNT
 	return 0;
 #else
 	struct rte_mbuf *mc = NULL;
@@ -406,7 +406,7 @@  fail:
 	if (mc)
 		rte_pktmbuf_free(mc);
 	return -1;
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 }
 #undef GOTO_FAIL
 
@@ -439,7 +439,7 @@  test_pktmbuf_pool(void)
 		printf("Error pool not empty");
 		ret = -1;
 	}
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	extra = rte_pktmbuf_clone(m[0], pktmbuf_pool);
 	if(extra != NULL) {
 		printf("Error pool not empty");
@@ -548,11 +548,11 @@  test_pktmbuf_free_segment(void)
 /*
  * Stress test for rte_mbuf atomic refcnt.
  * Implies that:
- * RTE_MBUF_SCATTER_GATHER and RTE_MBUF_REFCNT_ATOMIC are both defined.
+ * RTE_MBUF_REFCNT and RTE_MBUF_REFCNT_ATOMIC are both defined.
  * For more efficency, recomended to run with RTE_LIBRTE_MBUF_DEBUG defined.
  */
 
-#if defined RTE_MBUF_SCATTER_GATHER  && defined RTE_MBUF_REFCNT_ATOMIC
+#if defined RTE_MBUF_REFCNT  && defined RTE_MBUF_REFCNT_ATOMIC
 
 static int
 test_refcnt_slave(__attribute__((unused)) void *arg)
@@ -657,7 +657,7 @@  test_refcnt_master(void)
 static int
 test_refcnt_mbuf(void)
 {
-#if defined RTE_MBUF_SCATTER_GATHER  && defined RTE_MBUF_REFCNT_ATOMIC
+#if defined RTE_MBUF_REFCNT  && defined RTE_MBUF_REFCNT_ATOMIC
 
 	unsigned lnum, master, slave, tref;
 
@@ -808,7 +808,7 @@  test_failing_mbuf_sanity_check(void)
 		return -1;
 	}
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	badbuf = *buf;
 	badbuf.refcnt = 0;
 	if (verify_mbuf_check_panics(&badbuf)) {
diff --git a/config/common_bsdapp b/config/common_bsdapp
index bf6d8a0..645949f 100644
--- a/config/common_bsdapp
+++ b/config/common_bsdapp
@@ -249,7 +249,7 @@  CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n
 #
 CONFIG_RTE_LIBRTE_MBUF=y
 CONFIG_RTE_LIBRTE_MBUF_DEBUG=n
-CONFIG_RTE_MBUF_SCATTER_GATHER=y
+CONFIG_RTE_MBUF_REFCNT=y
 CONFIG_RTE_MBUF_REFCNT_ATOMIC=y
 CONFIG_RTE_PKTMBUF_HEADROOM=128
 
diff --git a/config/common_linuxapp b/config/common_linuxapp
index 9047975..5bee910 100644
--- a/config/common_linuxapp
+++ b/config/common_linuxapp
@@ -277,7 +277,7 @@  CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n
 #
 CONFIG_RTE_LIBRTE_MBUF=y
 CONFIG_RTE_LIBRTE_MBUF_DEBUG=n
-CONFIG_RTE_MBUF_SCATTER_GATHER=y
+CONFIG_RTE_MBUF_REFCNT=y
 CONFIG_RTE_MBUF_REFCNT_ATOMIC=y
 CONFIG_RTE_PKTMBUF_HEADROOM=128
 
diff --git a/doc/doxy-api.conf b/doc/doxy-api.conf
index 1fd4492..63dc64d 100644
--- a/doc/doxy-api.conf
+++ b/doc/doxy-api.conf
@@ -56,7 +56,7 @@  FILE_PATTERNS           = rte_*.h \
                           cmdline.h
 PREDEFINED              = __DOXYGEN__ \
                           __attribute__(x)= \
-                          RTE_MBUF_SCATTER_GATHER
+                          RTE_MBUF_REFCNT
 
 OPTIMIZE_OUTPUT_FOR_C   = YES
 ENABLE_PREPROCESSING    = YES
diff --git a/examples/ip_pipeline/main.c b/examples/ip_pipeline/main.c
index f773958..e19412f 100644
--- a/examples/ip_pipeline/main.c
+++ b/examples/ip_pipeline/main.c
@@ -148,7 +148,7 @@  app_lcore_main_loop(__attribute__((unused)) void *arg)
 			rte_exit(EXIT_FAILURE, "ACL not present in build\n");
 #endif
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 		case APP_CORE_IPV4_FRAG:
 			app_main_loop_pipeline_ipv4_frag();
 			return 0;
diff --git a/examples/ipv4_multicast/Makefile b/examples/ipv4_multicast/Makefile
index 191574b..604cebe 100644
--- a/examples/ipv4_multicast/Makefile
+++ b/examples/ipv4_multicast/Makefile
@@ -39,8 +39,8 @@  RTE_TARGET ?= x86_64-native-linuxapp-gcc
 
 include $(RTE_SDK)/mk/rte.vars.mk
 
-ifneq ($(CONFIG_RTE_MBUF_SCATTER_GATHER),y)
-$(error This application requires RTE_MBUF_SCATTER_GATHER to be enabled)
+ifneq ($(CONFIG_RTE_MBUF_REFCNT),y)
+$(error This application requires RTE_MBUF_REFCNT to be enabled)
 endif
 
 # binary name
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 193aa25..fe28912 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -727,10 +727,10 @@  us_vhost_parse_args(int argc, char **argv)
 					zero_copy = ret;
 
 				if (zero_copy) {
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 					RTE_LOG(ERR, VHOST_CONFIG, "Before running "
 					"zero copy vhost APP, please "
-					"disable RTE_MBUF_SCATTER_GATHER\n"
+					"disable RTE_MBUF_REFCNT\n"
 					"in config file and then rebuild DPDK "
 					"core lib!\n"
 					"Otherwise please disable zero copy "
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 09d8934..a4967e3 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -160,7 +160,7 @@  rte_mbuf_sanity_check(const struct rte_mbuf *m, enum rte_mbuf_type t,
 	if (m->buf_addr == NULL)
 		rte_panic("bad virt addr\n");
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	uint16_t cnt = rte_mbuf_refcnt_read(m);
 	if ((cnt == 0) || (cnt == UINT16_MAX))
 		rte_panic("bad ref cnt\n");
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 2735f37..fbd1c7b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -67,6 +67,9 @@ 
 extern "C" {
 #endif
 
+/* deprecated feature, renamed in RTE_MBUF_REFCNT */
+#pragma GCC poison RTE_MBUF_SCATTER_GATHER
+
 /**
  * A control message buffer.
  */
@@ -185,7 +188,7 @@  struct rte_mbuf {
 	void *buf_addr;           /**< Virtual address of segment buffer. */
 	phys_addr_t buf_physaddr; /**< Physical address of segment buffer. */
 	uint16_t buf_len;         /**< Length of segment buffer. */
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	/**
 	 * 16-bit Reference counter.
 	 * It should only be accessed using the following functions:
@@ -298,7 +301,7 @@  if (!(exp)) {                                                        \
 
 #endif /*  RTE_LIBRTE_MBUF_DEBUG */
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 #ifdef RTE_MBUF_REFCNT_ATOMIC
 
 /**
@@ -380,14 +383,14 @@  rte_mbuf_refcnt_set(struct rte_mbuf *m, uint16_t new_value)
 		rte_prefetch0(m);               \
 } while (0)
 
-#else /* ! RTE_MBUF_SCATTER_GATHER */
+#else /* ! RTE_MBUF_REFCNT */
 
 /** Mbuf prefetch */
 #define RTE_MBUF_PREFETCH_TO_FREE(m) do { } while(0)
 
 #define rte_mbuf_refcnt_set(m,v) do { } while(0)
 
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 
 
 /**
@@ -426,10 +429,10 @@  static inline struct rte_mbuf *__rte_mbuf_raw_alloc(struct rte_mempool *mp)
 	if (rte_mempool_get(mp, &mb) < 0)
 		return NULL;
 	m = (struct rte_mbuf *)mb;
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(m) == 0);
 	rte_mbuf_refcnt_set(m, 1);
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 	return (m);
 }
 
@@ -444,9 +447,9 @@  static inline struct rte_mbuf *__rte_mbuf_raw_alloc(struct rte_mempool *mp)
 static inline void __attribute__((always_inline))
 __rte_mbuf_raw_free(struct rte_mbuf *m)
 {
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	RTE_MBUF_ASSERT(rte_mbuf_refcnt_read(m) == 0);
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 	rte_mempool_put(m->pool, m);
 }
 
@@ -506,9 +509,9 @@  static inline struct rte_mbuf *rte_ctrlmbuf_alloc(struct rte_mempool *mp)
 static inline void rte_ctrlmbuf_free(struct rte_mbuf *m)
 {
 	__rte_mbuf_sanity_check(m, RTE_MBUF_CTRL, 0);
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	if (rte_mbuf_refcnt_update(m, -1) == 0)
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 		__rte_mbuf_raw_free(m);
 }
 
@@ -623,7 +626,7 @@  static inline struct rte_mbuf *rte_pktmbuf_alloc(struct rte_mempool *mp)
 	return (m);
 }
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 
 /**
  * Attach packet mbuf to another packet mbuf.
@@ -691,7 +694,7 @@  static inline void rte_pktmbuf_detach(struct rte_mbuf *m)
 	m->pkt.data_len = 0;
 }
 
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 
 
 static inline struct rte_mbuf* __attribute__((always_inline))
@@ -699,7 +702,7 @@  __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 {
 	__rte_mbuf_sanity_check(m, RTE_MBUF_PKT, 0);
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	if (likely (rte_mbuf_refcnt_read(m) == 1) ||
 			likely (rte_mbuf_refcnt_update(m, -1) == 0)) {
 		struct rte_mbuf *md = RTE_MBUF_FROM_BADDR(m->buf_addr);
@@ -717,7 +720,7 @@  __rte_pktmbuf_prefree_seg(struct rte_mbuf *m)
 		}
 #endif
 		return(m);
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	}
 	return (NULL);
 #endif
@@ -761,7 +764,7 @@  static inline void rte_pktmbuf_free(struct rte_mbuf *m)
 	}
 }
 
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 
 /**
  * Creates a "clone" of the given packet mbuf.
@@ -837,7 +840,7 @@  static inline void rte_pktmbuf_refcnt_update(struct rte_mbuf *m, int16_t v)
 	} while ((m = m->pkt.next) != NULL);
 }
 
-#endif /* RTE_MBUF_SCATTER_GATHER */
+#endif /* RTE_MBUF_REFCNT */
 
 /**
  * Get the headroom in a packet mbuf.
diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
index 09e19a3..ba5f927 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
@@ -404,7 +404,7 @@  ixgbe_tx_free_bufs(struct igb_tx_queue *txq)
 	struct igb_tx_entry_seq *txsp;
 	uint32_t status;
 	uint32_t n, k;
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 	uint32_t i;
 	int nb_free = 0;
 	struct rte_mbuf *m, *free[RTE_IXGBE_TX_MAX_FREE_BUF_SZ];
@@ -427,7 +427,7 @@  ixgbe_tx_free_bufs(struct igb_tx_queue *txq)
 
 	while (n > 0) {
 		k = RTE_MIN(n, txsp[n-1].same_pool);
-#ifdef RTE_MBUF_SCATTER_GATHER
+#ifdef RTE_MBUF_REFCNT
 		for (i = 0; i < k; i++) {
 			m = __rte_pktmbuf_prefree_seg((txep+n-k+i)->mbuf);
 			if (m != NULL)