Add user defined tag calculation callback to librte_distributor.

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

app/test/test_distributor.c +3 -3

index ce06436..6ea4943 100644
@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
 	char *name = NULL;
 
 	d = rte_distributor_create(name, rte_socket_id(),
-			rte_lcore_count() - 1);
+			rte_lcore_count() - 1, NULL);
 	if (d != NULL || rte_errno != EINVAL) {
 		printf("ERROR: No error on create() with NULL name param\n");
 		return -1;

@@ -467,7 +467,7 @@ int test_error_distributor_create_numworkers(void)
 {
 	struct rte_distributor *d = NULL;
 	d = rte_distributor_create("test_numworkers", rte_socket_id(),
-			RTE_MAX_LCORE + 10);
+			RTE_MAX_LCORE + 10, NULL);
 	if (d != NULL || rte_errno != EINVAL) {
 		printf("ERROR: No error on create() with num_workers > MAX\n");
 		return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
 	if (d == NULL) {
 		d = rte_distributor_create("Test_distributor", rte_socket_id(),
-				rte_lcore_count() - 1);
+				rte_lcore_count() - 1, NULL);
 		if (d == NULL) {
 			printf("Error creating distributor\n");
 			return -1;

app/test/test_distributor_perf.c +1 -1

index b04864c..507e446 100644
@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
 	if (d == NULL) {
 		d = rte_distributor_create("Test_perf", rte_socket_id(),
-				rte_lcore_count() - 1);
+				rte_lcore_count() - 1, NULL);
 		if (d == NULL) {
 			printf("Error creating distributor\n");
 			return -1;

lib/librte_distributor/rte_distributor.c +10 -2

index 585ff88..78c92bd 100644
@@ -97,6 +97,7 @@ struct rte_distributor {
 	union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 
 	struct rte_distributor_returned_pkts returns;
+	rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor *d,
 	struct rte_mbuf *next_mb = NULL;
 	int64_t next_value = 0;
 	uint32_t new_tag = 0;
+	rte_distributor_tag_fn tag_cb = d->tag_cb;
 	unsigned ret_start = d->returns.start,
 			ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor *d,
 			next_mb = mbufs[next_idx++];
 			next_value = (((int64_t)(uintptr_t)next_mb)
 					<< RTE_DISTRIB_FLAG_BITS);
-			new_tag = (next_mb->hash.rss | 1);
+			if (tag_cb) {
+				new_tag = tag_cb(next_mb);
+			} else {
+				new_tag = (next_mb->hash.rss | 1);
+			}
 
 			uint32_t match = 0;
 			unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct rte_distributor *d)
 struct rte_distributor *
 rte_distributor_create(const char *name,
 		unsigned socket_id,
-		unsigned num_workers)
+		unsigned num_workers,
+		rte_distributor_tag_fn tag_cb)
 {
 	struct rte_distributor *d;
 	struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
 	d = mz->addr;
 	snprintf(d->name, sizeof(d->name), "%s", name);
 	d->num_workers = num_workers;
+	d->tag_cb = tag_cb;
 
 	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	TAILQ_INSERT_TAIL(distributor_list, d, next);

lib/librte_distributor/rte_distributor.h +6 -1

index ec0d74a..844d325 100644
@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 
+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from this
  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-		unsigned num_workers);
+		unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*

Signature · 1 line

1.7.1

wrote:

Quoted 3 lines · Qinglai Xiao

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Interesting idea.
Did you investigate was there any performance improvement or regression comparing
whether the callback was called per-packet as packets were dequeued for distribution
(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets initially? I suspect
there probably isn't much performance difference either way, but it may be worth
checking.
One other point, is that I think the callback to extract the tag should have
additional parameters - at least one, if not two. I would suggest that the distributor pointer be passed in, as well as an arbitrary void * pointer.

Regards,
/Bruce

Quoted 13 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

app/test/test_distributor.c +0 -0

index ce06436..6ea4943 100644

app/test/test_distributor.c int test_error_distributor_create_name(void) · line 457

Quoted 7 lines

app/test/test_distributor.c +1 -1

@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
      char *name = NULL;
 
      d = rte_distributor_create(name, rte_socket_id(),
-                     rte_lcore_count() - 1);
+                     rte_lcore_count() - 1, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with NULL name

param\n");

app/test/test_distributor.c int test_error_distributor_create_numworkers(void) · line 472

Quoted 9 lines

app/test/test_distributor.c +1 -1

              return -1;

@@ -467,7 +467,7 @@ int test_error_distributor_create_numworkers(void)
 {
      struct rte_distributor *d = NULL;
      d = rte_distributor_create("test_numworkers", rte_socket_id(),
-                     RTE_MAX_LCORE + 10);
+                     RTE_MAX_LCORE + 10, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with num_workers >

MAX\n");

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Review

Quoted 13 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +1 -1

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_perf", rte_socket_id(),
-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct rte_distributor

Quoted 33 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +7 -1

@@ -97,6 +97,7 @@ struct rte_distributor {
      union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 
      struct rte_distributor_returned_pkts returns;
+     rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor *d,
      struct rte_mbuf *next_mb = NULL;
      int64_t next_value = 0;
      uint32_t new_tag = 0;
+     rte_distributor_tag_fn tag_cb = d->tag_cb;
      unsigned ret_start = d->returns.start,
                      ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor *d,
                      next_mb = mbufs[next_idx++];
                      next_value = (((int64_t)(uintptr_t)next_mb)
                                      << RTE_DISTRIB_FLAG_BITS);
-                     new_tag = (next_mb->hash.rss | 1);
+                     if (tag_cb) {
+                             new_tag = tag_cb(next_mb);
+                     } else {
+                             new_tag = (next_mb->hash.rss | 1);
+                     }
 
                      uint32_t match = 0;
                      unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct rte_distributor

*d)

Review

Quoted 18 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
              unsigned socket_id,
-             unsigned num_workers)
+             unsigned num_workers,
+             rte_distributor_tag_fn tag_cb)
 {
      struct rte_distributor *d;
      struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
      d = mz->addr;
      snprintf(d->name, sizeof(d->name), "%s", name);
      d->num_workers = num_workers;
+     d->tag_cb = tag_cb;
 
      rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
      TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 32 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +6 -2

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 
+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from this
  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-             unsigned num_workers);
+             unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

Hi Bruce,

Thanks for reply.
The idea is triggered by real life use case, where the flow id is buried in L3 payload. Deep packet inspection is one of the scenarios, tunneled pkts is another.
However, only functionality is verified. Performance impact has not been checked yet.

To add distributor and another void * as params is nice.

Your advice of extract tags in a row inspired me another solution, which is to change the union hash inside rte_mbuf:

lib/librte_mbuf/rte_mbuf.h +1 -0

index e8f9bfc..5b13c0b 100644
@@ -185,6 +185,7 @@ struct rte_mbuf {
                        uint16_t id;
                } fdir;           /**< Filter identifier if FDIR enabled */
                uint32_t sched;   /**< Hierarchical scheduler */
+               uint32_t user;    /**< User defined hash tag */
        } hash;                   /**< hash information */
 
        /* second cache line - fields only used in slow path or on TX */

The new union field user is actually for documentation purpose only, coz user application can set hash.rss value and have the same result. Therefore, the user application is free to calculate the tag in burst mode before calling rte_distributor_process.

Then rte_distributor_process needs to read next_mb->hash.user. Does it sounds better?

I have another question: why the logical OR 1 is added to new_tag?

thx &
rgds,
-qinglai

On Wed, Nov 5, 2014 at 4:27 PM, Bruce Richardson <[email protected]

Quoted 170 lines

wrote:

Quoted 3 lines · Qinglai Xiao

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Interesting idea.
Did you investigate was there any performance improvement or regression comparing
whether the callback was called per-packet as packets were dequeued for distribution
(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets initially? I suspect
there probably isn't much performance difference either way, but it may be worth
checking.
One other point, is that I think the callback to extract the tag should have
additional parameters - at least one, if not two. I would suggest that the distributor pointer be passed in, as well as an arbitrary void * pointer.

Regards,
/Bruce

Quoted 13 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

app/test/test_distributor.c +0 -0

index ce06436..6ea4943 100644

app/test/test_distributor.c int test_error_distributor_create_name(void) · line 457

Quoted 7 lines

app/test/test_distributor.c +1 -1

@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
      char *name = NULL;
 
      d = rte_distributor_create(name, rte_socket_id(),
-                     rte_lcore_count() - 1);
+                     rte_lcore_count() - 1, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with NULL name

param\n");

app/test/test_distributor.c int test_error_distributor_create_numworkers(void) · line 472

Quoted 9 lines

app/test/test_distributor.c +1 -1

              return -1;

@@ -467,7 +467,7 @@ int test_error_distributor_create_numworkers(void)
 {
      struct rte_distributor *d = NULL;
      d = rte_distributor_create("test_numworkers", rte_socket_id(),
-                     RTE_MAX_LCORE + 10);
+                     RTE_MAX_LCORE + 10, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with num_workers >

MAX\n");

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Review

Quoted 13 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +1 -1

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_perf", rte_socket_id(),
-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct rte_distributor

Quoted 33 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +7 -1

@@ -97,6 +97,7 @@ struct rte_distributor {
      union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 
      struct rte_distributor_returned_pkts returns;
+     rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor *d,
      struct rte_mbuf *next_mb = NULL;
      int64_t next_value = 0;
      uint32_t new_tag = 0;
+     rte_distributor_tag_fn tag_cb = d->tag_cb;
      unsigned ret_start = d->returns.start,
                      ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor *d,
                      next_mb = mbufs[next_idx++];
                      next_value = (((int64_t)(uintptr_t)next_mb)
                                      << RTE_DISTRIB_FLAG_BITS);
-                     new_tag = (next_mb->hash.rss | 1);
+                     if (tag_cb) {
+                             new_tag = tag_cb(next_mb);
+                     } else {
+                             new_tag = (next_mb->hash.rss | 1);
+                     }
 
                      uint32_t match = 0;
                      unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct rte_distributor

*d)

Review

Quoted 18 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
              unsigned socket_id,
-             unsigned num_workers)
+             unsigned num_workers,
+             rte_distributor_tag_fn tag_cb)
 {
      struct rte_distributor *d;
      struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
      d = mz->addr;
      snprintf(d->name, sizeof(d->name), "%s", name);
      d->num_workers = num_workers;
+     d->tag_cb = tag_cb;
 
      rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
      TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 32 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +6 -2

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 
+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from this
  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-             unsigned num_workers);
+             unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

wrote:

Quoted 4 lines · jigsaw

Hi Bruce,

Thanks for reply.
The idea is triggered by real life use case, where the flow id is buried

in

Quoted 8 lines

L3 payload. Deep packet inspection is one of the scenarios, tunneled pkts is another.
However, only functionality is verified. Performance impact has not been checked yet.

To add distributor and another void * as params is nice.

Your advice of extract tags in a row inspired me another solution, which

is

lib/librte_mbuf/rte_mbuf.h struct rte_mbuf { · line 186

Quoted 9 lines

to change the union hash inside rte_mbuf:

lib/librte_mbuf/rte_mbuf.h +0 -0

index e8f9bfc..5b13c0b 100644
@@ -185,6 +185,7 @@ struct rte_mbuf {
                        uint16_t id;
                } fdir;           /**< Filter identifier if FDIR enabled

*/

Quoted 9 lines

lib/librte_mbuf/rte_mbuf.h +1 -0

                uint32_t sched;   /**< Hierarchical scheduler */
+               uint32_t user;    /**< User defined hash tag */
        } hash;                   /**< hash information */
 
        /* second cache line - fields only used in slow path or on TX */

The new union field user is actually for documentation purpose only, coz user application can set hash.rss value and have the same result. Therefore, the user application is free to calculate the tag in burst

mode

Quoted 4 lines

before calling rte_distributor_process.

Then rte_distributor_process needs to read next_mb->hash.user. Does it sounds better?

What you propose is the exact original intent, though I did not try to add a new union member purely for documentation purposes. I had planned, but perhaps did not explain well enough, that the application would itself set up
the tag as it thought best before passing packets to the distributor. I suspect
that overloading the RSS field for this impeded that idea geting through.

/Bruce

Quoted 13 lines

I have another question: why the logical OR 1 is added to new_tag?

thx &
rgds,
-qinglai

Quoted 1 line

On Wed, Nov 5, 2014 at 4:27 PM, Bruce Richardson <

[email protected]

Quoted 7 lines

Quoted 7 lines

wrote:

Quoted 3 lines · Qinglai Xiao

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Quoted 9 lines

Quoted 9 lines

Interesting idea.
Did you investigate was there any performance improvement or regression comparing
whether the callback was called per-packet as packets were dequeued for distribution
(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets initially? I suspect
there probably isn't much performance difference either way, but it

may be

Quoted 5 lines

Quoted 5 lines

worth
checking.
One other point, is that I think the callback to extract the tag should have
additional parameters - at least one, if not two. I would suggest that

the

Quoted 1 line

Quoted 1 line

distributor pointer be passed in, as well as an arbitrary void *

pointer.

Quoted 12 lines

Quoted 12 lines

Regards,
/Bruce

Quoted 8 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/app/test/test_distributor.c

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

index ce06436..6ea4943 100644

app/test/test_distributor.c +0 -0

@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
      char *name = NULL;
 

Quoted 8 lines

Quoted 8 lines

app/test/test_distributor.c int test_error_distributor_create_name(void) · line 457

Quoted 5 lines

app/test/test_distributor.c +1 -1

      d = rte_distributor_create(name, rte_socket_id(),
-                     rte_lcore_count() - 1);
+                     rte_lcore_count() - 1, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with NULL name

param\n");

app/test/test_distributor.c int

Quoted 2 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -467,7 +467,7 @@ int

test_error_distributor_create_numworkers(void)

Quoted 21 lines

Quoted 21 lines

app/test/test_distributor.c int · line 472

Quoted 7 lines

app/test/test_distributor.c +1 -1

 {
      struct rte_distributor *d = NULL;
      d = rte_distributor_create("test_numworkers", rte_socket_id(),
-                     RTE_MAX_LCORE + 10);
+                     RTE_MAX_LCORE + 10, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with num_workers >

MAX\n");

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Quoted 7 lines

Quoted 7 lines

app/test/test_distributor_perf.c test_distributor_perf(void) · line 229

Quoted 7 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +0 -0

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_perf",

rte_socket_id(),

Quoted 13 lines

Quoted 13 lines

Review

Quoted 6 lines

app/test/test_distributor_perf.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

Quoted 6 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +0 -0

@@ -97,6 +97,7 @@ struct rte_distributor {
      union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 

Quoted 6 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_distributor_returned_pkts returns;
+     rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 8 lines

Quoted 8 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 8 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_mbuf *next_mb = NULL;
      int64_t next_value = 0;
      uint32_t new_tag = 0;
+     rte_distributor_tag_fn tag_cb = d->tag_cb;
      unsigned ret_start = d->returns.start,
                      ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +5 -1

                      next_mb = mbufs[next_idx++];
                      next_value = (((int64_t)(uintptr_t)next_mb)
                                      << RTE_DISTRIB_FLAG_BITS);
-                     new_tag = (next_mb->hash.rss | 1);
+                     if (tag_cb) {
+                             new_tag = tag_cb(next_mb);
+                     } else {
+                             new_tag = (next_mb->hash.rss | 1);
+                     }
 

Quoted 3 lines

Quoted 3 lines

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +0 -0

                      uint32_t match = 0;
                      unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct

rte_distributor

Quoted 27 lines

Quoted 27 lines

*d)

Review

Quoted 18 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
              unsigned socket_id,
-             unsigned num_workers)
+             unsigned num_workers,
+             rte_distributor_tag_fn tag_cb)
 {
      struct rte_distributor *d;
      struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
      d = mz->addr;
      snprintf(d->name, sizeof(d->name), "%s", name);
      d->num_workers = num_workers;
+     d->tag_cb = tag_cb;
 
      rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
      TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 7 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +0 -0

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 

Quoted 9 lines

Quoted 9 lines

lib/librte_distributor/rte_distributor.h struct rte_distributor; · line 69

Quoted 9 lines

lib/librte_distributor/rte_distributor.h +3 -0

+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from

this

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

lib/librte_distributor/rte_distributor.h +3 -2

  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-             unsigned num_workers);
+             unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

Hi Bruce,

OK understood. Then there's no real need to make any change. But the question remains about this line:

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

thx &
rgds,

-qinglai

On Wed, Nov 5, 2014 at 6:36 PM, Bruce Richardson <[email protected]

Quoted 248 lines

wrote:

Quoted 4 lines · jigsaw

Hi Bruce,

Thanks for reply.
The idea is triggered by real life use case, where the flow id is buried

in

Quoted 8 lines

L3 payload. Deep packet inspection is one of the scenarios, tunneled pkts is another.
However, only functionality is verified. Performance impact has not been checked yet.

To add distributor and another void * as params is nice.

Your advice of extract tags in a row inspired me another solution, which

is

lib/librte_mbuf/rte_mbuf.h struct rte_mbuf { · line 186

Quoted 9 lines

to change the union hash inside rte_mbuf:

lib/librte_mbuf/rte_mbuf.h +0 -0

index e8f9bfc..5b13c0b 100644
@@ -185,6 +185,7 @@ struct rte_mbuf {
                        uint16_t id;
                } fdir;           /**< Filter identifier if FDIR enabled

*/

Quoted 9 lines

lib/librte_mbuf/rte_mbuf.h +1 -0

                uint32_t sched;   /**< Hierarchical scheduler */
+               uint32_t user;    /**< User defined hash tag */
        } hash;                   /**< hash information */
 
        /* second cache line - fields only used in slow path or on TX */

The new union field user is actually for documentation purpose only, coz user application can set hash.rss value and have the same result. Therefore, the user application is free to calculate the tag in burst

mode

Quoted 4 lines

before calling rte_distributor_process.

Then rte_distributor_process needs to read next_mb->hash.user. Does it sounds better?

What you propose is the exact original intent, though I did not try to add a new union member purely for documentation purposes. I had planned, but perhaps did not explain well enough, that the application would itself set up
the tag as it thought best before passing packets to the distributor. I suspect
that overloading the RSS field for this impeded that idea geting through.

/Bruce

Quoted 13 lines

I have another question: why the logical OR 1 is added to new_tag?

thx &
rgds,
-qinglai

Quoted 1 line

On Wed, Nov 5, 2014 at 4:27 PM, Bruce Richardson <

[email protected]

Quoted 7 lines

Quoted 7 lines

wrote:

Quoted 3 lines · Qinglai Xiao

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Quoted 9 lines

Quoted 9 lines

Interesting idea.
Did you investigate was there any performance improvement or regression comparing
whether the callback was called per-packet as packets were dequeued for distribution
(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets initially? I suspect
there probably isn't much performance difference either way, but it

may be

Quoted 5 lines

Quoted 5 lines

worth
checking.
One other point, is that I think the callback to extract the tag should have
additional parameters - at least one, if not two. I would suggest that

the

Quoted 1 line

Quoted 1 line

distributor pointer be passed in, as well as an arbitrary void *

pointer.

Quoted 12 lines

Quoted 12 lines

Regards,
/Bruce

Quoted 8 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/app/test/test_distributor.c

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

index ce06436..6ea4943 100644

app/test/test_distributor.c +0 -0

@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
      char *name = NULL;
 

Quoted 8 lines

Quoted 8 lines

app/test/test_distributor.c int test_error_distributor_create_name(void) · line 457

Quoted 5 lines

app/test/test_distributor.c +1 -1

      d = rte_distributor_create(name, rte_socket_id(),
-                     rte_lcore_count() - 1);
+                     rte_lcore_count() - 1, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with NULL name

param\n");

app/test/test_distributor.c int

Quoted 2 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -467,7 +467,7 @@ int

test_error_distributor_create_numworkers(void)

Quoted 21 lines

Quoted 21 lines

app/test/test_distributor.c int · line 472

Quoted 7 lines

app/test/test_distributor.c +1 -1

 {
      struct rte_distributor *d = NULL;
      d = rte_distributor_create("test_numworkers", rte_socket_id(),
-                     RTE_MAX_LCORE + 10);
+                     RTE_MAX_LCORE + 10, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with num_workers >

MAX\n");

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Quoted 7 lines

Quoted 7 lines

app/test/test_distributor_perf.c test_distributor_perf(void) · line 229

Quoted 7 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +0 -0

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_perf",

rte_socket_id(),

Quoted 13 lines

Quoted 13 lines

Review

Quoted 6 lines

app/test/test_distributor_perf.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

Quoted 6 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +0 -0

@@ -97,6 +97,7 @@ struct rte_distributor {
      union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 

Quoted 6 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_distributor_returned_pkts returns;
+     rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 8 lines

Quoted 8 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 8 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_mbuf *next_mb = NULL;
      int64_t next_value = 0;
      uint32_t new_tag = 0;
+     rte_distributor_tag_fn tag_cb = d->tag_cb;
      unsigned ret_start = d->returns.start,
                      ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +5 -1

                      next_mb = mbufs[next_idx++];
                      next_value = (((int64_t)(uintptr_t)next_mb)
                                      << RTE_DISTRIB_FLAG_BITS);
-                     new_tag = (next_mb->hash.rss | 1);
+                     if (tag_cb) {
+                             new_tag = tag_cb(next_mb);
+                     } else {
+                             new_tag = (next_mb->hash.rss | 1);
+                     }
 

Quoted 3 lines

Quoted 3 lines

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +0 -0

                      uint32_t match = 0;
                      unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct

rte_distributor

Quoted 27 lines

Quoted 27 lines

*d)

Review

Quoted 18 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
              unsigned socket_id,
-             unsigned num_workers)
+             unsigned num_workers,
+             rte_distributor_tag_fn tag_cb)
 {
      struct rte_distributor *d;
      struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
      d = mz->addr;
      snprintf(d->name, sizeof(d->name), "%s", name);
      d->num_workers = num_workers;
+     d->tag_cb = tag_cb;
 
      rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
      TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 7 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +0 -0

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 

Quoted 9 lines

Quoted 9 lines

lib/librte_distributor/rte_distributor.h struct rte_distributor; · line 69

Quoted 9 lines

lib/librte_distributor/rte_distributor.h +3 -0

+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from

this

Quoted 17 lines

Quoted 17 lines

Quoted 16 lines

lib/librte_distributor/rte_distributor.h +3 -2

  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-             unsigned num_workers);
+             unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

Quoted 6 lines · jigsaw

Hi Bruce,

OK understood. Then there's no real need to make any change. But the question remains about this line:

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual value of zero.
We instead always force the low bit to be 1, so that we can use zero as an "empty" value.

/Bruce

Quoted 7 lines

thx &
rgds,

-qinglai

On Wed, Nov 5, 2014 at 6:36 PM, Bruce Richardson <

[email protected]

Quoted 7 lines

Quoted 7 lines

wrote:

Quoted 4 lines · jigsaw

Hi Bruce,

Thanks for reply.
The idea is triggered by real life use case, where the flow id is

buried

Quoted 2 lines

Quoted 2 lines

in

Quoted 1 line

L3 payload. Deep packet inspection is one of the scenarios, tunneled

pkts

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

is another.
However, only functionality is verified. Performance impact has not

been

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

checked yet.

To add distributor and another void * as params is nice.

Your advice of extract tags in a row inspired me another solution,

which

Quoted 10 lines

Quoted 10 lines

is

lib/librte_mbuf/rte_mbuf.h struct rte_mbuf { · line 186

Quoted 9 lines

to change the union hash inside rte_mbuf:

lib/librte_mbuf/rte_mbuf.h +0 -0

index e8f9bfc..5b13c0b 100644
@@ -185,6 +185,7 @@ struct rte_mbuf {
                        uint16_t id;
                } fdir;           /**< Filter identifier if FDIR

enabled

Quoted 6 lines

Quoted 6 lines

*/

Quoted 5 lines

lib/librte_mbuf/rte_mbuf.h +1 -0

                uint32_t sched;   /**< Hierarchical scheduler */
+               uint32_t user;    /**< User defined hash tag */
        } hash;                   /**< hash information */
 
        /* second cache line - fields only used in slow path or on

TX */

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

The new union field user is actually for documentation purpose only,

coz

Quoted 9 lines

Quoted 9 lines

Quoted 2 lines

user application can set hash.rss value and have the same result. Therefore, the user application is free to calculate the tag in burst

mode

Quoted 4 lines

before calling rte_distributor_process.

Then rte_distributor_process needs to read next_mb->hash.user. Does it sounds better?

What you propose is the exact original intent, though I did not try to

add

Quoted 1 line

Quoted 1 line

a new union member purely for documentation purposes. I had planned,

but

Quoted 1 line

Quoted 1 line

perhaps did not explain well enough, that the application would itself

set

Quoted 4 lines

Quoted 4 lines

up
the tag as it thought best before passing packets to the distributor. I suspect
that overloading the RSS field for this impeded that idea geting

through.

Quoted 20 lines

Quoted 20 lines

/Bruce

Quoted 13 lines

I have another question: why the logical OR 1 is added to new_tag?

thx &
rgds,
-qinglai

Quoted 1 line

On Wed, Nov 5, 2014 at 4:27 PM, Bruce Richardson <

[email protected]

Quoted 2 lines

Quoted 1 line

wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines · Qinglai Xiao

Quoted 3 lines

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Interesting idea.
Did you investigate was there any performance improvement or

regression

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

comparing
whether the callback was called per-packet as packets were

dequeued for

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

distribution
(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets

initially? I

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

suspect
there probably isn't much performance difference either way, but it

may be

Quoted 3 lines

Quoted 3 lines

worth
checking.
One other point, is that I think the callback to extract the tag

should

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

have
additional parameters - at least one, if not two. I would suggest

that

Quoted 29 lines

Quoted 29 lines

the

Quoted 1 line

Quoted 1 line

distributor pointer be passed in, as well as an arbitrary void *

pointer.

Quoted 12 lines

Quoted 12 lines

Regards,
/Bruce

Quoted 8 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

Quoted 1 line

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/app/test/test_distributor.c

Quoted 12 lines

Quoted 12 lines

app/test/test_distributor.c int test_error_distributor_create_name(void) · line 457

Quoted 11 lines

index ce06436..6ea4943 100644

app/test/test_distributor.c +1 -1

@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
      char *name = NULL;
 
      d = rte_distributor_create(name, rte_socket_id(),
-                     rte_lcore_count() - 1);
+                     rte_lcore_count() - 1, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with NULL name

param\n");

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

app/test/test_distributor.c int

Quoted 2 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -467,7 +467,7 @@ int

test_error_distributor_create_numworkers(void)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

app/test/test_distributor.c +0 -0

 {
      struct rte_distributor *d = NULL;
      d = rte_distributor_create("test_numworkers",

rte_socket_id(),

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

app/test/test_distributor.c int · line 472

Quoted 4 lines

app/test/test_distributor.c +1 -1

-                     RTE_MAX_LCORE + 10);
+                     RTE_MAX_LCORE + 10, NULL);
      if (d != NULL || rte_errno != EINVAL) {
              printf("ERROR: No error on create() with

num_workers >

Quoted 35 lines

Quoted 35 lines

Quoted 14 lines

Quoted 14 lines

MAX\n");

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

              return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Quoted 7 lines

Quoted 7 lines

app/test/test_distributor_perf.c test_distributor_perf(void) · line 229

Quoted 7 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +0 -0

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
      if (d == NULL) {
              d = rte_distributor_create("Test_perf",

rte_socket_id(),

Quoted 13 lines

Quoted 13 lines

Review

Quoted 6 lines

app/test/test_distributor_perf.c +1 -1

-                             rte_lcore_count() - 1);
+                             rte_lcore_count() - 1, NULL);
              if (d == NULL) {
                      printf("Error creating distributor\n");
                      return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

Quoted 6 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +0 -0

@@ -97,6 +97,7 @@ struct rte_distributor {
      union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_distributor_returned_pkts returns;
+     rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct

rte_distributor

Quoted 9 lines

Quoted 9 lines

*d,

Quoted 8 lines

Quoted 8 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct

Quoted 8 lines

lib/librte_distributor/rte_distributor.c +1 -0

      struct rte_mbuf *next_mb = NULL;
      int64_t next_value = 0;
      uint32_t new_tag = 0;
+     rte_distributor_tag_fn tag_cb = d->tag_cb;
      unsigned ret_start = d->returns.start,
                      ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct

rte_distributor

Quoted 42 lines

Quoted 42 lines

*d,

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +5 -1

                      next_mb = mbufs[next_idx++];
                      next_value = (((int64_t)(uintptr_t)next_mb)
                                      << RTE_DISTRIB_FLAG_BITS);
-                     new_tag = (next_mb->hash.rss | 1);
+                     if (tag_cb) {
+                             new_tag = tag_cb(next_mb);
+                     } else {
+                             new_tag = (next_mb->hash.rss | 1);
+                     }
 

Quoted 3 lines

Quoted 3 lines

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +0 -0

                      uint32_t match = 0;
                      unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct

rte_distributor

Quoted 27 lines

Quoted 27 lines

*d)

Review

Quoted 18 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
              unsigned socket_id,
-             unsigned num_workers)
+             unsigned num_workers,
+             rte_distributor_tag_fn tag_cb)
 {
      struct rte_distributor *d;
      struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
      d = mz->addr;
      snprintf(d->name, sizeof(d->name), "%s", name);
      d->num_workers = num_workers;
+     d->tag_cb = tag_cb;
 
      rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
      TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 7 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +0 -0

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

lib/librte_distributor/rte_distributor.h struct rte_distributor; · line 69

Quoted 9 lines

lib/librte_distributor/rte_distributor.h +3 -0

+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets

from

Quoted 11 lines

Quoted 11 lines

this

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

lib/librte_distributor/rte_distributor.h +3 -1

  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-             unsigned num_workers);
+             unsigned num_workers, rte_distributor_tag_fn

tag_cb);

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.h +0 -1

 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

Quoted 5 lines

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual value of zero. We instead always force the low bit to be 1, so that we can use zero as an "empty" value.

Quoted 5 lines

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual value of zero. We instead always force the low bit to be 1, so that we can use zero as an "empty" value.

Hi Bruce,

There is a subtle case in which tag values are 2 and 3, respectively. Then these two tags cannot be distinguished. There should be a better way so as to handle this situation.

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback tolibrte_distributor.

2014-11-06 09:22, Bruce Richardson:

Quoted 10 lines · jigsaw

Quoted 5 lines

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual value of zero. We instead always force the low bit to be 1, so that we can use zero as an "empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs more explanations in the code or doxygen.

Thanks

Signature · 1 line

Thomas

wrote:

Quoted 3 lines · Qinglai Xiao

Hi Bruce,

There is a subtle case in which tag values are 2 and 3, respectively.

Then these two tags cannot be distinguished. There should be a better way so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags differ by
only a single bit. I've been assuming that the tag is likely to be a hash value in most cases - given that it's only 32-bit - in which case it just doesn't
matter which bit we chose to permanently set to 1, but if there are scenarios
where it's likely that the low bits are used but the high ones not so, we can
look to change which bit is set to 1. Either way, the distributor just uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 11 lines

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual value

of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can use zero

as an

Quoted 9 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs more explanations in the code or doxygen.

Thanks
--
Thomas

Hi Bruce,

In my use case, unfortunately the tag is not hash. And the tag can be on either low or high bits, depending on configuration.
I wonder if it is possible to let the user to decide which bit to mask, i.e. to add another param to rte_distributor_create to define the mask.

thx &
rgds,
-qinglai

Quoted 58 lines · Bruce Richardson

Quoted 3 lines · Qinglai Xiao

Hi Bruce,

There is a subtle case in which tag values are 2 and 3, respectively.

Then these two tags cannot be distinguished. There should be a better way so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags differ by
only a single bit. I've been assuming that the tag is likely to be a hash value in most cases - given that it's only 32-bit - in which case it just doesn't
matter which bit we chose to permanently set to 1, but if there are scenarios
where it's likely that the low bits are used but the high ones not so, we can
look to change which bit is set to 1. Either way, the distributor just uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 11 lines

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual

value of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can use zero

as an

Quoted 9 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs more explanations in the code or doxygen.

Thanks
--
Thomas

Hi Bruce,

Actually IMHO it is good to leave the freedom to user to decide how to interpret the tag value, i.e. remove the OR 1 bit.
If the tag value is zero, then we assume the programmer know what he is doing. Of course this shall be clearly documented in the comment/doxgen.

thx &
rgds,
-qinglai

Quoted 75 lines

Hi Bruce,

In my use case, unfortunately the tag is not hash. And the tag can be on either low or high bits, depending on configuration.
I wonder if it is possible to let the user to decide which bit to mask, i.e. to add another param to rte_distributor_create to define the mask.

thx &
rgds,
-qinglai

Quoted 58 lines · Bruce Richardson

Quoted 3 lines · Qinglai Xiao

Hi Bruce,

There is a subtle case in which tag values are 2 and 3, respectively.

Then these two tags cannot be distinguished. There should be a better way so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags differ by
only a single bit. I've been assuming that the tag is likely to be a hash value in most cases - given that it's only 32-bit - in which case it just doesn't
matter which bit we chose to permanently set to 1, but if there are scenarios
where it's likely that the low bits are used but the high ones not so, we can
look to change which bit is set to 1. Either way, the distributor just uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 11 lines

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual

value of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can use zero

as an

Quoted 9 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs more explanations in the code or doxygen.

Thanks
--
Thomas

Quoted 1 line

If a tag value of zero is ever passed in, then it will start matching

Quoted 11 lines · jigsaw

Hi Bruce,

Actually IMHO it is good to leave the freedom to user to decide how to interpret the tag value, i.e. remove the OR 1 bit.
If the tag value is zero, then we assume the programmer know what he is doing. Of course this shall be clearly documented in the comment/doxgen.

thx &
rgds,
-qinglai

I don't believe that will work. If a tag value of zero is ever passed in, then it will start matching against cores which are not doing any processing. Then it will get queued up to get sent to those cores, and so never get processed.
We need a bit somewhere inside the tag to permanently set - though it can be configurable.

/Bruce

Quoted 6 lines · jigsaw

Quoted 4 lines

Hi Bruce,

In my use case, unfortunately the tag is not hash. And the tag can be

on

Quoted 8 lines

Quoted 8 lines

either low or high bits, depending on configuration.
I wonder if it is possible to let the user to decide which bit to mask, i.e. to add another param to rte_distributor_create to define the mask.

thx &
rgds,
-qinglai

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines · Bruce Richardson

Quoted 3 lines · Qinglai Xiao

Hi Bruce,

There is a subtle case in which tag values are 2 and 3,

respectively.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Then these two tags cannot be distinguished. There should be a better

way

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags

differ

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

by
only a single bit. I've been assuming that the tag is likely to be a

hash

Quoted 1 line

Quoted 1 line

Quoted 1 line

value in most cases - given that it's only 32-bit - in which case it

just

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

doesn't
matter which bit we chose to permanently set to 1, but if there are scenarios
where it's likely that the low bits are used but the high ones not

so, we

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

can
look to change which bit is set to 1. Either way, the distributor just uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 11 lines

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual

value of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can use

zero

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

as an

Quoted 4 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs

more

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

explanations in the code or doxygen.

Thanks
--
Thomas

Hi Bruce,

Quoted 1 line

Quoted 1 line

If a tag value of zero is ever passed in, then it will start matching

against cores which are not doing any processing.

Yes, this is true according to current bookkeeping of inflight tags.

But if the slot in in_flight_tags is not a uint32_t but a struct which has a filed as indication of "on/off", and also with corresponding changes in looking for a matched tag, then the need for 1 bit mask can be eliminated. Of course this change requires a little bit more, O(n), memory space and costs O(n) more branch misses. But the benefit is a more free interface to user app.

This is just another trade-off. Since I am in need of such freedom, I am more interested in the free use of 32bits.

thx &
rgds,
-qinglai

Quoted 110 lines · Bruce Richardson

Quoted 11 lines · jigsaw

Hi Bruce,

Actually IMHO it is good to leave the freedom to user to decide how to interpret the tag value, i.e. remove the OR 1 bit.
If the tag value is zero, then we assume the programmer know what he is doing. Of course this shall be clearly documented in the comment/doxgen.

thx &
rgds,
-qinglai

I don't believe that will work. If a tag value of zero is ever passed in, then it will start matching against cores which are not doing any processing. Then it will get queued up to get sent to those cores, and so never get processed.
We need a bit somewhere inside the tag to permanently set - though it can be configurable.

/Bruce

Quoted 6 lines · jigsaw

Quoted 4 lines

Hi Bruce,

In my use case, unfortunately the tag is not hash. And the tag can be

on

Quoted 8 lines

Quoted 8 lines

either low or high bits, depending on configuration.
I wonder if it is possible to let the user to decide which bit to mask, i.e. to add another param to rte_distributor_create to define the mask.

thx &
rgds,
-qinglai

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines · Bruce Richardson

Quoted 3 lines · Qinglai Xiao

Hi Bruce,

There is a subtle case in which tag values are 2 and 3,

respectively.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Then these two tags cannot be distinguished. There should be a better

way

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags

differ

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

by
only a single bit. I've been assuming that the tag is likely to be a

hash

Quoted 1 line

Quoted 1 line

Quoted 1 line

value in most cases - given that it's only 32-bit - in which case it

just

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

doesn't
matter which bit we chose to permanently set to 1, but if there are scenarios
where it's likely that the low bits are used but the high ones not

so, we

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

can
look to change which bit is set to 1. Either way, the distributor just uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 11 lines

thx &
rgds
-qinglai

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an actual

value of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can use

zero

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

as an

Quoted 4 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs

more

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

explanations in the code or doxygen.

Thanks
--
Thomas

wrote:

Quoted 8 lines · jigsaw

Hi Bruce,

Quoted 1 line

Quoted 1 line

If a tag value of zero is ever passed in, then it will start matching

against cores which are not doing any processing.

Yes, this is true according to current bookkeeping of inflight tags.

But if the slot in in_flight_tags is not a uint32_t but a struct which

has

Quoted 2 lines

a filed as indication of "on/off", and also with corresponding changes in looking for a matched tag, then the need for 1 bit mask can be

eliminated.

Quoted 2 lines

Of course this change requires a little bit more, O(n), memory space and costs O(n) more branch misses. But the benefit is a more free interface

to

Quoted 4 lines

user app.

This is just another trade-off. Since I am in need of such freedom, I am more interested in the free use of 32bits.

If you do implement such a change, I would suggest you simply add a bitmask to the distributor indicating valid workers. Then when we do the check for tag matches, we just need an extra "and" instruction to eliminate invalid
workers from the match.

/Bruce

Quoted 13 lines

thx &
rgds,
-qinglai

Quoted 4 lines · Bruce Richardson

Quoted 3 lines · jigsaw

Hi Bruce,

Actually IMHO it is good to leave the freedom to user to decide how

to

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

interpret the tag value, i.e. remove the OR 1 bit.
If the tag value is zero, then we assume the programmer know what he

is

Quoted 1 line

Quoted 1 line

Quoted 1 line

doing. Of course this shall be clearly documented in the

comment/doxgen.

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

thx &
rgds,
-qinglai

I don't believe that will work. If a tag value of zero is ever passed in, then it will start matching against cores which are not doing any processing. Then it will get queued up to get sent to those cores, and

so

Quoted 2 lines

Quoted 2 lines

never get processed.
We need a bit somewhere inside the tag to permanently set - though it

can

Quoted 10 lines

Quoted 10 lines

be configurable.

/Bruce

Quoted 6 lines · jigsaw

Quoted 4 lines

Hi Bruce,

In my use case, unfortunately the tag is not hash. And the tag can

be

Quoted 3 lines

Quoted 3 lines

on

Quoted 2 lines

Quoted 2 lines

either low or high bits, depending on configuration.
I wonder if it is possible to let the user to decide which bit to

mask,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

i.e. to add another param to rte_distributor_create to define the

mask.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

thx &
rgds,
-qinglai

On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson < [email protected]> wrote:

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines · Qinglai Xiao

Quoted 3 lines

Hi Bruce,

There is a subtle case in which tag values are 2 and 3,

respectively.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Then these two tags cannot be distinguished. There should be a

better

Quoted 7 lines

Quoted 7 lines

way

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

so as to handle this situation.

It's not just in that, case, it's in any case where a pair of tags

differ

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

by
only a single bit. I've been assuming that the tag is likely to

be a

Quoted 2 lines

Quoted 2 lines

hash

Quoted 1 line

Quoted 1 line

Quoted 1 line

value in most cases - given that it's only 32-bit - in which case

it

Quoted 3 lines

Quoted 3 lines

just

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

doesn't
matter which bit we chose to permanently set to 1, but if there

are

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

scenarios
where it's likely that the low bits are used but the high ones not

so, we

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

can
look to change which bit is set to 1. Either way, the distributor

just

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

uses a
31-bit tag rather than a 32-bit one.

/Bruce

Quoted 5 lines

thx &
rgds
-qinglai

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

-----原始邮件-----
发件人: "Thomas Monjalon" <[email protected]>
发送时间: ‎2014/‎11/‎6 12:36
收件人: "Bruce Richardson" <[email protected]>
抄送: "[email protected]" <[email protected]>; "jigsaw" <[email protected]> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation

callback

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

tolibrte_distributor.

Quoted 4 lines

2014-11-06 09:22, Bruce Richardson:

Quoted 2 lines · jigsaw

Quoted 1 line

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

That's needed to ensure that we never track a tag with an

actual

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

value of zero.

Quoted 1 line

Quoted 1 line

We instead always force the low bit to be 1, so that we can

use

Quoted 5 lines

Quoted 5 lines

zero

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

as an

Quoted 3 lines

Quoted 1 line

"empty" value.

Bruce, could you check how this code may be better commented

please?

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

This discussion shows that the distributor library probably

needs

Quoted 10 lines

Quoted 10 lines

more

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

explanations in the code or doxygen.

Thanks
--
Thomas

Hi Bruce,

Pls have a quick look at the diff to see if this is exactly what you mean about the bitmask.
I just wrote it without even compiling, just to express the idea. So it may leave some places unpatched.
If this is agreed, I will make a decent test to verify it before sending the patch for RFC.

lib/librte_distributor/rte_distributor.c +0 -0

index 585ff88..d606bcf 100644
@@ -92,6 +92,8 @@ struct rte_distributor {
        unsigned num_workers;                 /**< Number of workers

polling */

        uint32_t in_flight_tags[RTE_MAX_LCORE];

lib/librte_distributor/rte_distributor.c +6 -2

+       uint32_t in_flight_bitmask;
+
        struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
 
        union rte_distributor_buffer bufs[RTE_MAX_LCORE];

@@ -188,6 +190,7 @@ static inline void
 handle_worker_shutdown(struct rte_distributor *d, unsigned wkr)
 {
        d->in_flight_tags[wkr] = 0;
+       d->in_flight_mask &= ~(1 << wkr);
        d->bufs[wkr].bufptr64 = 0;
        if (unlikely(d->backlog[wkr].count != 0)) {
                /* On return of a packet, we need to move the

@@ -241,6 +244,7 @@ process_returns(struct rte_distributor *d)
                        else {
                                d->bufs[wkr].bufptr64 = RTE_DISTRIB_GET_BUF;
                                d->in_flight_tags[wkr] = 0;
+                               d->in_flight_mask &= ~(1 << wkr);
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
                } else if (data & RTE_DISTRIB_RETURN_BUF) {

@@ -282,12 +286,13 @@ rte_distributor_process(struct rte_distributor *d,
                        next_mb = mbufs[next_idx++];
                        next_value = (((int64_t)(uintptr_t)next_mb)
                                        << RTE_DISTRIB_FLAG_BITS);
-                       new_tag = (next_mb->hash.rss | 1);
+                       new_tag = next_mb->hash.rss;
 
                        uint32_t match = 0;
                        unsigned i;
                        for (i = 0; i < d->num_workers; i++)
-                               match |= (!(d->in_flight_tags[i] ^ new_tag)
+                               match |= (((!(d->in_flight_tags[i] ^

new_tag)) &
+ (d->in_flight_bitmask >> i))

                                        << i);

                        if (match) {

lib/librte_distributor/rte_distributor.c +1 -0

@@ -309,6 +314,7 @@ rte_distributor_process(struct rte_distributor *d,
                        else {
                                d->bufs[wkr].bufptr64 = next_value;
                                d->in_flight_tags[wkr] = new_tag;
+                               d->in_flight_bitmask |= 1 << wkr;
                                next_mb = NULL;
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;

wrote:

Quoted 5 lines · jigsaw

Hi Bruce,

Pls have a quick look at the diff to see if this is exactly what you mean about the bitmask.
I just wrote it without even compiling, just to express the idea. So it

may

Quoted 28 lines

leave some places unpatched.
If this is agreed, I will make a decent test to verify it before sending the patch for RFC.

lib/librte_distributor/rte_distributor.c +0 -0

index 585ff88..d606bcf 100644
@@ -92,6 +92,8 @@ struct rte_distributor {
        unsigned num_workers;                 /**< Number of workers

polling */

        uint32_t in_flight_tags[RTE_MAX_LCORE];

lib/librte_distributor/rte_distributor.c +3 -0

+       uint32_t in_flight_bitmask;
+
        struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
 
        union rte_distributor_buffer bufs[RTE_MAX_LCORE];

@@ -188,6 +190,7 @@ static inline void
 handle_worker_shutdown(struct rte_distributor *d, unsigned wkr)
 {
        d->in_flight_tags[wkr] = 0;
+       d->in_flight_mask &= ~(1 << wkr);
        d->bufs[wkr].bufptr64 = 0;
        if (unlikely(d->backlog[wkr].count != 0)) {
                /* On return of a packet, we need to move the

lib/librte_distributor/rte_distributor.c process_returns(struct rte_distributor *d) · line 245

Quoted 2 lines

lib/librte_distributor/rte_distributor.c +0 -0

@@ -241,6 +244,7 @@ process_returns(struct rte_distributor *d)
                        else {
                                d->bufs[wkr].bufptr64 =

RTE_DISTRIB_GET_BUF;

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

                                d->in_flight_tags[wkr] = 0;
+                               d->in_flight_mask &= ~(1 << wkr);
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
                } else if (data & RTE_DISTRIB_RETURN_BUF) {

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor *d, · line 290

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +1 -2

@@ -282,12 +286,13 @@ rte_distributor_process(struct rte_distributor *d,
                        next_mb = mbufs[next_idx++];
                        next_value = (((int64_t)(uintptr_t)next_mb)
                                        << RTE_DISTRIB_FLAG_BITS);
-                       new_tag = (next_mb->hash.rss | 1);
+                       new_tag = next_mb->hash.rss;
 
                        uint32_t match = 0;
                        unsigned i;
                        for (i = 0; i < d->num_workers; i++)
-                               match |= (!(d->in_flight_tags[i] ^

new_tag)

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +1 -0

+                               match |= (((!(d->in_flight_tags[i] ^

new_tag)) &
+ (d->in_flight_bitmask >>

i))

I would not do the bitmask comparison here, as that's extra instruction in the
loop. Instead, because its a bitmask, build up the match variable as it was before, and then just do a single and operation afterwards, outside the loop
body.

/Bruce

Quoted 14 lines

                                        << i);

                        if (match) {

lib/librte_distributor/rte_distributor.c +1 -0

@@ -309,6 +314,7 @@ rte_distributor_process(struct rte_distributor *d,
                        else {
                                d->bufs[wkr].bufptr64 = next_value;
                                d->in_flight_tags[wkr] = new_tag;
+                               d->in_flight_bitmask |= 1 << wkr;
                                next_mb = NULL;
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;

Yeah that's better. As below, right?

(quoted diff) +1 -0

@@ -290,6 +294,7 @@ rte_distributor_process(struct rte_distributor *d,
                                match |= (!(d->in_flight_tags[i] ^ new_tag)
                                        << i);
 
+                       match &= d->in_flight_bitmask;
                        if (match) {
                                next_mb = NULL;
                                unsigned worker = __builtin_ctz(match);

On Fri, Nov 7, 2014 at 4:44 PM, Bruce Richardson <[email protected]

Quoted 86 lines

wrote:

Quoted 5 lines · jigsaw

Hi Bruce,

Pls have a quick look at the diff to see if this is exactly what you mean about the bitmask.
I just wrote it without even compiling, just to express the idea. So it

may

Quoted 28 lines

leave some places unpatched.
If this is agreed, I will make a decent test to verify it before sending the patch for RFC.

lib/librte_distributor/rte_distributor.c +0 -0

index 585ff88..d606bcf 100644
@@ -92,6 +92,8 @@ struct rte_distributor {
        unsigned num_workers;                 /**< Number of workers

polling */

        uint32_t in_flight_tags[RTE_MAX_LCORE];

lib/librte_distributor/rte_distributor.c +3 -0

+       uint32_t in_flight_bitmask;
+
        struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
 
        union rte_distributor_buffer bufs[RTE_MAX_LCORE];

@@ -188,6 +190,7 @@ static inline void
 handle_worker_shutdown(struct rte_distributor *d, unsigned wkr)
 {
        d->in_flight_tags[wkr] = 0;
+       d->in_flight_mask &= ~(1 << wkr);
        d->bufs[wkr].bufptr64 = 0;
        if (unlikely(d->backlog[wkr].count != 0)) {
                /* On return of a packet, we need to move the

lib/librte_distributor/rte_distributor.c process_returns(struct rte_distributor *d) · line 245

Quoted 2 lines

lib/librte_distributor/rte_distributor.c +0 -0

@@ -241,6 +244,7 @@ process_returns(struct rte_distributor *d)
                        else {
                                d->bufs[wkr].bufptr64 =

RTE_DISTRIB_GET_BUF;

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

                                d->in_flight_tags[wkr] = 0;
+                               d->in_flight_mask &= ~(1 << wkr);
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
                } else if (data & RTE_DISTRIB_RETURN_BUF) {

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor *d, · line 290

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +1 -2

@@ -282,12 +286,13 @@ rte_distributor_process(struct rte_distributor *d,
                        next_mb = mbufs[next_idx++];
                        next_value = (((int64_t)(uintptr_t)next_mb)
                                        << RTE_DISTRIB_FLAG_BITS);
-                       new_tag = (next_mb->hash.rss | 1);
+                       new_tag = next_mb->hash.rss;
 
                        uint32_t match = 0;
                        unsigned i;
                        for (i = 0; i < d->num_workers; i++)
-                               match |= (!(d->in_flight_tags[i] ^

new_tag)

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +1 -0

+                               match |= (((!(d->in_flight_tags[i] ^

new_tag)) &
+ (d->in_flight_bitmask >>

i))

I would not do the bitmask comparison here, as that's extra instruction in the
loop. Instead, because its a bitmask, build up the match variable as it was before, and then just do a single and operation afterwards, outside the loop
body.

/Bruce

Quoted 14 lines

                                        << i);

                        if (match) {

lib/librte_distributor/rte_distributor.c +1 -0

@@ -309,6 +314,7 @@ rte_distributor_process(struct rte_distributor *d,
                        else {
                                d->bufs[wkr].bufptr64 = next_value;
                                d->in_flight_tags[wkr] = new_tag;
+                               d->in_flight_bitmask |= 1 << wkr;
                                next_mb = NULL;
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;

wrote:

Quoted 1 line · jigsaw

Yeah that's better. As below, right?

Yep.

Review rte_distributor_process(struct rte_distributor *d, · line 294

Quoted 3 lines

(quoted diff) +0 -0

@@ -290,6 +294,7 @@ rte_distributor_process(struct rte_distributor *d,
                                match |= (!(d->in_flight_tags[i] ^

new_tag)

Quoted 9 lines

(quoted diff) +1 -0

                                        << i);
 
+                       match &= d->in_flight_bitmask;
                        if (match) {
                                next_mb = NULL;
                                unsigned worker = __builtin_ctz(match);

On Fri, Nov 7, 2014 at 4:44 PM, Bruce Richardson <

[email protected]

Quoted 6 lines

Quoted 6 lines

wrote:

Quoted 3 lines · jigsaw

Hi Bruce,

Pls have a quick look at the diff to see if this is exactly what you

mean

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

about the bitmask.
I just wrote it without even compiling, just to express the idea. So

it

Quoted 3 lines

Quoted 3 lines

may

Quoted 2 lines

leave some places unpatched.
If this is agreed, I will make a decent test to verify it before

sending

Quoted 29 lines

Quoted 29 lines

Quoted 26 lines

the patch for RFC.

lib/librte_distributor/rte_distributor.c +0 -0

index 585ff88..d606bcf 100644
@@ -92,6 +92,8 @@ struct rte_distributor {
        unsigned num_workers;                 /**< Number of workers

polling */

        uint32_t in_flight_tags[RTE_MAX_LCORE];

lib/librte_distributor/rte_distributor.c +3 -0

+       uint32_t in_flight_bitmask;
+
        struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
 
        union rte_distributor_buffer bufs[RTE_MAX_LCORE];

@@ -188,6 +190,7 @@ static inline void
 handle_worker_shutdown(struct rte_distributor *d, unsigned wkr)
 {
        d->in_flight_tags[wkr] = 0;
+       d->in_flight_mask &= ~(1 << wkr);
        d->bufs[wkr].bufptr64 = 0;
        if (unlikely(d->backlog[wkr].count != 0)) {
                /* On return of a packet, we need to move the

lib/librte_distributor/rte_distributor.c process_returns(struct rte_distributor *d) · line 245

Quoted 2 lines

lib/librte_distributor/rte_distributor.c +0 -0

@@ -241,6 +244,7 @@ process_returns(struct rte_distributor *d)
                        else {
                                d->bufs[wkr].bufptr64 =

RTE_DISTRIB_GET_BUF;

Quoted 6 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -0

                                d->in_flight_tags[wkr] = 0;
+                               d->in_flight_mask &= ~(1 << wkr);
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
                } else if (data & RTE_DISTRIB_RETURN_BUF) {

@@ -282,12 +286,13 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

lib/librte_distributor/rte_distributor.c +1 -1

                        next_mb = mbufs[next_idx++];
                        next_value = (((int64_t)(uintptr_t)next_mb)
                                        << RTE_DISTRIB_FLAG_BITS);
-                       new_tag = (next_mb->hash.rss | 1);
+                       new_tag = next_mb->hash.rss;
 

Quoted 8 lines

Quoted 8 lines

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor · line 290

Quoted 4 lines

lib/librte_distributor/rte_distributor.c +0 -1

                        uint32_t match = 0;
                        unsigned i;
                        for (i = 0; i < d->num_workers; i++)
-                               match |= (!(d->in_flight_tags[i] ^

new_tag)

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +1 -0

+                               match |= (((!(d->in_flight_tags[i] ^

new_tag)) &
+

(d->in_flight_bitmask >>

Quoted 3 lines

Quoted 3 lines

i))

I would not do the bitmask comparison here, as that's extra

instruction in

Quoted 2 lines

Quoted 2 lines

the
loop. Instead, because its a bitmask, build up the match variable as

it was

Quoted 10 lines

Quoted 10 lines

before, and then just do a single and operation afterwards, outside the loop
body.

/Bruce

lib/librte_distributor/rte_distributor.c rte_distributor_process(struct rte_distributor

Quoted 4 lines

                                        << i);

                        if (match) {

lib/librte_distributor/rte_distributor.c +0 -0

@@ -309,6 +314,7 @@ rte_distributor_process(struct rte_distributor

*d,

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

lib/librte_distributor/rte_distributor.c +1 -0

                        else {
                                d->bufs[wkr].bufptr64 = next_value;
                                d->in_flight_tags[wkr] = new_tag;
+                               d->in_flight_bitmask |= 1 << wkr;
                                next_mb = NULL;
                        }
                        oldbuf = data >> RTE_DISTRIB_FLAG_BITS;

From: "Bruce Richardson" <[email protected]>

Add in some additional comments around more complex areas of the code so as to make the code easier to read and understand.

Signed-off-by Bruce Richardson <[email protected]>

2014-11-06 13:55, Bruce Richardson:

Quoted 6 lines

From: "Bruce Richardson" <[email protected]>

Add in some additional comments around more complex areas of the code so as to make the code easier to read and understand.

Signed-off-by Bruce Richardson <[email protected]>

Applied

Qinglai, if you change the design, you'll now have some comments to update ;)

Thanks
--
Thomas

2014-11-06 09:22, Bruce Richardson:

Quoted 7 lines · jigsaw

Quoted 5 lines

http://dpdk.org/browse/dpdk/tree/lib/librte_distributor/rte_distributor.c#n285

        new_tag = (next_mb->hash.rss | 1);

Why the logical OR is needed?

Quoted 3 lines

That's needed to ensure that we never track a tag with an actual value of zero. We instead always force the low bit to be 1, so that we can use zero as an "empty" value.

Bruce, could you check how this code may be better commented please? This discussion shows that the distributor library probably needs more explanations in the code or doxygen.

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Bruce Richardson
Sent: Wednesday, November 05, 2014 2:28 PM
To: Qinglai Xiao
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.

Quoted 3 lines · Qinglai Xiao

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Interesting idea.
Did you investigate was there any performance improvement or regression comparing whether the callback was called per-packet as packets were dequeued for distribution (i.e. how you have things now in your patch), compared to calling
the callback in a loop to extract the tags for all packets initially? I suspect there probably isn't much performance difference either way, but it may be worth checking.
One other point, is that I think the callback to extract the tag should have additional parameters - at least one, if not two. I would suggest that the distributor pointer be passed in, as well as an arbitrary void * pointer.

Regards,
/Bruce

Quoted 136 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

app/test/test_distributor.c +3 -3

index ce06436..6ea4943 100644
@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
 	char *name = NULL;
 
 	d = rte_distributor_create(name, rte_socket_id(),
-			rte_lcore_count() - 1);
+			rte_lcore_count() - 1, NULL);
 	if (d != NULL || rte_errno != EINVAL) {
 		printf("ERROR: No error on create() with NULL name param\n");
 		return -1;

@@ -467,7 +467,7 @@ int test_error_distributor_create_numworkers(void)
 {
 	struct rte_distributor *d = NULL;
 	d = rte_distributor_create("test_numworkers", rte_socket_id(),
-			RTE_MAX_LCORE + 10);
+			RTE_MAX_LCORE + 10, NULL);
 	if (d != NULL || rte_errno != EINVAL) {
 		printf("ERROR: No error on create() with num_workers > MAX\n");
 		return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
 	if (d == NULL) {
 		d = rte_distributor_create("Test_distributor", rte_socket_id(),
-				rte_lcore_count() - 1);
+				rte_lcore_count() - 1, NULL);
 		if (d == NULL) {
 			printf("Error creating distributor\n");
 			return -1;

app/test/test_distributor_perf.c +1 -1

index b04864c..507e446 100644
@@ -227,7 +227,7 @@ test_distributor_perf(void)
 
 	if (d == NULL) {
 		d = rte_distributor_create("Test_perf", rte_socket_id(),
-				rte_lcore_count() - 1);
+				rte_lcore_count() - 1, NULL);
 		if (d == NULL) {
 			printf("Error creating distributor\n");
 			return -1;

lib/librte_distributor/rte_distributor.c +10 -2

index 585ff88..78c92bd 100644
@@ -97,6 +97,7 @@ struct rte_distributor {
 	union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 
 	struct rte_distributor_returned_pkts returns;
+	rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor *d,
 	struct rte_mbuf *next_mb = NULL;
 	int64_t next_value = 0;
 	uint32_t new_tag = 0;
+	rte_distributor_tag_fn tag_cb = d->tag_cb;
 	unsigned ret_start = d->returns.start,
 			ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor *d,
 			next_mb = mbufs[next_idx++];
 			next_value = (((int64_t)(uintptr_t)next_mb)
 					<< RTE_DISTRIB_FLAG_BITS);
-			new_tag = (next_mb->hash.rss | 1);
+			if (tag_cb) {
+				new_tag = tag_cb(next_mb);
+			} else {
+				new_tag = (next_mb->hash.rss | 1);
+			}
 
 			uint32_t match = 0;
 			unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct rte_distributor *d)
 struct rte_distributor *
 rte_distributor_create(const char *name,
 		unsigned socket_id,
-		unsigned num_workers)
+		unsigned num_workers,
+		rte_distributor_tag_fn tag_cb)
 {
 	struct rte_distributor *d;
 	struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
 	d = mz->addr;
 	snprintf(d->name, sizeof(d->name), "%s", name);
 	d->num_workers = num_workers;
+	d->tag_cb = tag_cb;
 
 	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
 	TAILQ_INSERT_TAIL(distributor_list, d, next);

lib/librte_distributor/rte_distributor.h +6 -2

index ec0d74a..844d325 100644
@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 
+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from this
  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-		unsigned num_workers);
+		unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1

Quoted 8 lines

-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of Bruce Richardson
Sent: Wednesday, November 05, 2014 2:28 PM
To: Qinglai Xiao
Cc: [email protected]
Subject: Re: [dpdk-dev] [PATCH] Add user defined tag calculation

callback to librte_distributor.

Quoted 8 lines · Qinglai Xiao

Quoted 3 lines

User defined tag calculation has access to mbuf.
Default tag is RSS hash result.

Interesting idea.
Did you investigate was there any performance improvement or regression

comparing

Quoted 1 line

whether the callback was called per-packet as packets were dequeued for

distribution

Quoted 2 lines

(i.e. how you have things now in your patch), compared to calling the callback in a loop to extract the tags for all packets initially? I

suspect

Quoted 1 line

there probably isn't much performance difference either way, but it may

be worth

Quoted 2 lines

checking.
One other point, is that I think the callback to extract the tag should

have

Quoted 1 line

additional parameters - at least one, if not two. I would suggest that

the

Quoted 1 line

distributor pointer be passed in, as well as an arbitrary void * pointer.

Just wonder, why do you need a call-back?
Why not just make rte_distributor_process() to accept an extra parameter:
array of tags?

rte_distributor_process(struct rte_distributor *d,
struct rte_mbuf **mbufs, uint32_t *mbuf_tags, unsigned
num_mbufs)

?

Quoted 19 lines

Regards,
/Bruce

Quoted 15 lines

Signed-off-by Qinglai Xiao <[email protected]>

---
 app/test/test_distributor.c              |    6 +++---
 app/test/test_distributor_perf.c         |    2 +-
 lib/librte_distributor/rte_distributor.c |   12 ++++++++++--
 lib/librte_distributor/rte_distributor.h |    7 ++++++-
 4 files changed, 20 insertions(+), 7 deletions(-)

app/test/test_distributor.c +0 -0

index ce06436..6ea4943 100644
@@ -452,7 +452,7 @@ int test_error_distributor_create_name(void)
    char *name = NULL;
 

Quoted 5 lines

app/test/test_distributor.c int test_error_distributor_create_name(void)

Quoted 5 lines

app/test/test_distributor.c +1 -1

    d = rte_distributor_create(name, rte_socket_id(),
-                   rte_lcore_count() - 1);
+                   rte_lcore_count() - 1, NULL);
    if (d != NULL || rte_errno != EINVAL) {
            printf("ERROR: No error on create() with NULL name

param\n");

Quoted 9 lines

app/test/test_distributor.c int test_error_distributor_create_numworkers(void) · line 472

Quoted 9 lines

app/test/test_distributor.c +1 -1

            return -1;

@@ -467,7 +467,7 @@ int test_error_distributor_create_numworkers(void)
 {
    struct rte_distributor *d = NULL;
    d = rte_distributor_create("test_numworkers", rte_socket_id(),
-                   RTE_MAX_LCORE + 10);
+                   RTE_MAX_LCORE + 10, NULL);
    if (d != NULL || rte_errno != EINVAL) {
            printf("ERROR: No error on create() with num_workers >

MAX\n");

Quoted 5 lines

app/test/test_distributor.c test_distributor(void) · line 517

Quoted 5 lines

app/test/test_distributor.c +0 -0

            return -1;

@@ -515,7 +515,7 @@ test_distributor(void)
 
    if (d == NULL) {
            d = rte_distributor_create("Test_distributor",

rte_socket_id(),

Quoted 6 lines

Review

Quoted 6 lines

app/test/test_distributor.c +1 -1

-                           rte_lcore_count() - 1);
+                           rte_lcore_count() - 1, NULL);
            if (d == NULL) {
                    printf("Error creating distributor\n");
                    return -1;

(quoted diff) +0 -0

b/app/test/test_distributor_perf.c

Quoted 5 lines

Quoted 5 lines

index b04864c..507e446 100644

app/test/test_distributor_perf.c +0 -0

@@ -227,7 +227,7 @@ test_distributor_perf(void)
 

Quoted 8 lines

Review

Quoted 8 lines

app/test/test_distributor_perf.c +1 -1

    if (d == NULL) {
            d = rte_distributor_create("Test_perf", rte_socket_id(),
-                           rte_lcore_count() - 1);
+                           rte_lcore_count() - 1, NULL);
            if (d == NULL) {
                    printf("Error creating distributor\n");
                    return -1;

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.c

Quoted 30 lines

Quoted 30 lines

index 585ff88..78c92bd 100644

lib/librte_distributor/rte_distributor.c +7 -1

@@ -97,6 +97,7 @@ struct rte_distributor {
    union rte_distributor_buffer bufs[RTE_MAX_LCORE];
 
    struct rte_distributor_returned_pkts returns;
+   rte_distributor_tag_fn tag_cb;
 };
 
 TAILQ_HEAD(rte_distributor_list, rte_distributor);

@@ -267,6 +268,7 @@ rte_distributor_process(struct rte_distributor *d,
    struct rte_mbuf *next_mb = NULL;
    int64_t next_value = 0;
    uint32_t new_tag = 0;
+   rte_distributor_tag_fn tag_cb = d->tag_cb;
    unsigned ret_start = d->returns.start,
                    ret_count = d->returns.count;
 

@@ -282,7 +284,11 @@ rte_distributor_process(struct rte_distributor *d,
                    next_mb = mbufs[next_idx++];
                    next_value = (((int64_t)(uintptr_t)next_mb)
                                    << RTE_DISTRIB_FLAG_BITS);
-                   new_tag = (next_mb->hash.rss | 1);
+                   if (tag_cb) {
+                           new_tag = tag_cb(next_mb);
+                   } else {
+                           new_tag = (next_mb->hash.rss | 1);
+                   }
 

Quoted 3 lines

lib/librte_distributor/rte_distributor.c rte_distributor_clear_returns(struct

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +0 -0

                    uint32_t match = 0;
                    unsigned i;

@@ -401,7 +407,8 @@ rte_distributor_clear_returns(struct

rte_distributor *d)

Quoted 15 lines

Quoted 15 lines

lib/librte_distributor/rte_distributor.c +3 -1

 struct rte_distributor *
 rte_distributor_create(const char *name,
            unsigned socket_id,
-           unsigned num_workers)
+           unsigned num_workers,
+           rte_distributor_tag_fn tag_cb)
 {
    struct rte_distributor *d;
    struct rte_distributor_list *distributor_list;

@@ -435,6 +442,7 @@ rte_distributor_create(const char *name,
    d = mz->addr;
    snprintf(d->name, sizeof(d->name), "%s", name);
    d->num_workers = num_workers;
+   d->tag_cb = tag_cb;
 

Quoted 3 lines

Review

Quoted 3 lines

lib/librte_distributor/rte_distributor.c +0 -0

    rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
    TAILQ_INSERT_TAIL(distributor_list, d, next);

(quoted diff) +0 -0

b/lib/librte_distributor/rte_distributor.h

Quoted 32 lines

Quoted 32 lines

index ec0d74a..844d325 100644

lib/librte_distributor/rte_distributor.h +6 -2

@@ -52,6 +52,9 @@ extern "C" {
 
 struct rte_distributor;
 
+typedef uint32_t (*rte_distributor_tag_fn)(struct rte_mbuf *);
+/**< User defined tag calculation function */
+
 /**
  * Function to create a new distributor instance
  *

@@ -65,12 +68,14 @@ struct rte_distributor;
  * @param num_workers
  *   The maximum number of workers that will request packets from this
  *   distributor
+ * @param tag_cb
+ *   The callback function for calculation of user defined tag.
  * @return
  *   The newly created distributor instance
  */
 struct rte_distributor *
 rte_distributor_create(const char *name, unsigned socket_id,
-           unsigned num_workers);
+           unsigned num_workers, rte_distributor_tag_fn tag_cb);
 
 /*  *** APIS to be called on the distributor lcore ***  */
 /*
--

1.7.1