* [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
@ 2014-11-05 13:30 Qinglai Xiao
2014-11-05 14:27 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: Qinglai Xiao @ 2014-11-05 13:30 UTC (permalink / raw)
To: dev; +Cc: Qinglai Xiao
User defined tag calculation has access to mbuf.
Default tag is RSS hash result.
Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
---
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(-)
diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index ce06436..6ea4943 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -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;
diff --git a/app/test/test_distributor_perf.c b/app/test/test_distributor_perf.c
index b04864c..507e446 100644
--- a/app/test/test_distributor_perf.c
+++ b/app/test/test_distributor_perf.c
@@ -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;
diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 585ff88..78c92bd 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -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);
diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
index ec0d74a..844d325 100644
--- a/lib/librte_distributor/rte_distributor.h
+++ b/lib/librte_distributor/rte_distributor.h
@@ -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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 13:30 [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor Qinglai Xiao
@ 2014-11-05 14:27 ` Bruce Richardson
2014-11-05 15:11 ` jigsaw
2014-11-05 15:13 ` Ananyev, Konstantin
0 siblings, 2 replies; 25+ messages in thread
From: Bruce Richardson @ 2014-11-05 14:27 UTC (permalink / raw)
To: Qinglai Xiao; +Cc: dev
On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> 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
> Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> ---
> 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(-)
>
> diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> index ce06436..6ea4943 100644
> --- a/app/test/test_distributor.c
> +++ b/app/test/test_distributor.c
> @@ -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;
> diff --git a/app/test/test_distributor_perf.c b/app/test/test_distributor_perf.c
> index b04864c..507e446 100644
> --- a/app/test/test_distributor_perf.c
> +++ b/app/test/test_distributor_perf.c
> @@ -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;
> diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> index 585ff88..78c92bd 100644
> --- a/lib/librte_distributor/rte_distributor.c
> +++ b/lib/librte_distributor/rte_distributor.c
> @@ -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);
> diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
> index ec0d74a..844d325 100644
> --- a/lib/librte_distributor/rte_distributor.h
> +++ b/lib/librte_distributor/rte_distributor.h
> @@ -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
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 14:27 ` Bruce Richardson
@ 2014-11-05 15:11 ` jigsaw
2014-11-05 16:36 ` Bruce Richardson
2014-11-05 15:13 ` Ananyev, Konstantin
1 sibling, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-05 15:11 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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:
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index e8f9bfc..5b13c0b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -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 <bruce.richardson@intel.com
> wrote:
> On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > 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
>
> > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > ---
> > 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(-)
> >
> > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> > index ce06436..6ea4943 100644
> > --- a/app/test/test_distributor.c
> > +++ b/app/test/test_distributor.c
> > @@ -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;
> > diff --git a/app/test/test_distributor_perf.c
> b/app/test/test_distributor_perf.c
> > index b04864c..507e446 100644
> > --- a/app/test/test_distributor_perf.c
> > +++ b/app/test/test_distributor_perf.c
> > @@ -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;
> > diff --git a/lib/librte_distributor/rte_distributor.c
> b/lib/librte_distributor/rte_distributor.c
> > index 585ff88..78c92bd 100644
> > --- a/lib/librte_distributor/rte_distributor.c
> > +++ b/lib/librte_distributor/rte_distributor.c
> > @@ -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);
> > diff --git a/lib/librte_distributor/rte_distributor.h
> b/lib/librte_distributor/rte_distributor.h
> > index ec0d74a..844d325 100644
> > --- a/lib/librte_distributor/rte_distributor.h
> > +++ b/lib/librte_distributor/rte_distributor.h
> > @@ -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
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 14:27 ` Bruce Richardson
2014-11-05 15:11 ` jigsaw
@ 2014-11-05 15:13 ` Ananyev, Konstantin
2014-11-05 15:24 ` jigsaw
1 sibling, 1 reply; 25+ messages in thread
From: Ananyev, Konstantin @ 2014-11-05 15:13 UTC (permalink / raw)
To: Richardson, Bruce, Qinglai Xiao; +Cc: dev
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Wednesday, November 05, 2014 2:28 PM
> To: Qinglai Xiao
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
>
> On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > 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.
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)
?
>
> Regards,
> /Bruce
>
> > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > ---
> > 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(-)
> >
> > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> > index ce06436..6ea4943 100644
> > --- a/app/test/test_distributor.c
> > +++ b/app/test/test_distributor.c
> > @@ -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;
> > diff --git a/app/test/test_distributor_perf.c b/app/test/test_distributor_perf.c
> > index b04864c..507e446 100644
> > --- a/app/test/test_distributor_perf.c
> > +++ b/app/test/test_distributor_perf.c
> > @@ -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;
> > diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
> > index 585ff88..78c92bd 100644
> > --- a/lib/librte_distributor/rte_distributor.c
> > +++ b/lib/librte_distributor/rte_distributor.c
> > @@ -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);
> > diff --git a/lib/librte_distributor/rte_distributor.h b/lib/librte_distributor/rte_distributor.h
> > index ec0d74a..844d325 100644
> > --- a/lib/librte_distributor/rte_distributor.h
> > +++ b/lib/librte_distributor/rte_distributor.h
> > @@ -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
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 15:13 ` Ananyev, Konstantin
@ 2014-11-05 15:24 ` jigsaw
0 siblings, 0 replies; 25+ messages in thread
From: jigsaw @ 2014-11-05 15:24 UTC (permalink / raw)
To: Ananyev, Konstantin; +Cc: dev
Hi Konstantin,
I agree with you. A callback is not necessary. Pls refer to my previous
mail, which proposed a not-at-all-intrusive patch.
Pls let me know your concern.
thx &
rgds,
-qinglai
On Wed, Nov 5, 2014 at 5:13 PM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> > Sent: Wednesday, November 05, 2014 2:28 PM
> > To: Qinglai Xiao
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] Add user defined tag calculation
> callback to librte_distributor.
> >
> > On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > > 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.
>
>
> 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)
>
> ?
>
>
> >
> > Regards,
> > /Bruce
> >
> > > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > > ---
> > > 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(-)
> > >
> > > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> > > index ce06436..6ea4943 100644
> > > --- a/app/test/test_distributor.c
> > > +++ b/app/test/test_distributor.c
> > > @@ -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;
> > > diff --git a/app/test/test_distributor_perf.c
> b/app/test/test_distributor_perf.c
> > > index b04864c..507e446 100644
> > > --- a/app/test/test_distributor_perf.c
> > > +++ b/app/test/test_distributor_perf.c
> > > @@ -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;
> > > diff --git a/lib/librte_distributor/rte_distributor.c
> b/lib/librte_distributor/rte_distributor.c
> > > index 585ff88..78c92bd 100644
> > > --- a/lib/librte_distributor/rte_distributor.c
> > > +++ b/lib/librte_distributor/rte_distributor.c
> > > @@ -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);
> > > diff --git a/lib/librte_distributor/rte_distributor.h
> b/lib/librte_distributor/rte_distributor.h
> > > index ec0d74a..844d325 100644
> > > --- a/lib/librte_distributor/rte_distributor.h
> > > +++ b/lib/librte_distributor/rte_distributor.h
> > > @@ -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
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 15:11 ` jigsaw
@ 2014-11-05 16:36 ` Bruce Richardson
2014-11-05 17:24 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-05 16:36 UTC (permalink / raw)
To: jigsaw; +Cc: dev
On Wed, Nov 05, 2014 at 05:11:51PM +0200, jigsaw wrote:
> 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:
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index e8f9bfc..5b13c0b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -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?
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
>
> 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 <bruce.richardson@intel.com
> > wrote:
>
> > On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > > 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
> >
> > > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > > ---
> > > 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(-)
> > >
> > > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> > > index ce06436..6ea4943 100644
> > > --- a/app/test/test_distributor.c
> > > +++ b/app/test/test_distributor.c
> > > @@ -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;
> > > diff --git a/app/test/test_distributor_perf.c
> > b/app/test/test_distributor_perf.c
> > > index b04864c..507e446 100644
> > > --- a/app/test/test_distributor_perf.c
> > > +++ b/app/test/test_distributor_perf.c
> > > @@ -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;
> > > diff --git a/lib/librte_distributor/rte_distributor.c
> > b/lib/librte_distributor/rte_distributor.c
> > > index 585ff88..78c92bd 100644
> > > --- a/lib/librte_distributor/rte_distributor.c
> > > +++ b/lib/librte_distributor/rte_distributor.c
> > > @@ -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);
> > > diff --git a/lib/librte_distributor/rte_distributor.h
> > b/lib/librte_distributor/rte_distributor.h
> > > index ec0d74a..844d325 100644
> > > --- a/lib/librte_distributor/rte_distributor.h
> > > +++ b/lib/librte_distributor/rte_distributor.h
> > > @@ -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
> > >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 16:36 ` Bruce Richardson
@ 2014-11-05 17:24 ` jigsaw
2014-11-06 9:22 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-05 17:24 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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 <bruce.richardson@intel.com
> wrote:
> On Wed, Nov 05, 2014 at 05:11:51PM +0200, jigsaw wrote:
> > 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:
> >
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index e8f9bfc..5b13c0b 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -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?
>
> 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
>
> >
> > 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 <
> bruce.richardson@intel.com
> > > wrote:
> >
> > > On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > > > 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
> > >
> > > > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > > > ---
> > > > 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(-)
> > > >
> > > > diff --git a/app/test/test_distributor.c
> b/app/test/test_distributor.c
> > > > index ce06436..6ea4943 100644
> > > > --- a/app/test/test_distributor.c
> > > > +++ b/app/test/test_distributor.c
> > > > @@ -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;
> > > > diff --git a/app/test/test_distributor_perf.c
> > > b/app/test/test_distributor_perf.c
> > > > index b04864c..507e446 100644
> > > > --- a/app/test/test_distributor_perf.c
> > > > +++ b/app/test/test_distributor_perf.c
> > > > @@ -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;
> > > > diff --git a/lib/librte_distributor/rte_distributor.c
> > > b/lib/librte_distributor/rte_distributor.c
> > > > index 585ff88..78c92bd 100644
> > > > --- a/lib/librte_distributor/rte_distributor.c
> > > > +++ b/lib/librte_distributor/rte_distributor.c
> > > > @@ -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);
> > > > diff --git a/lib/librte_distributor/rte_distributor.h
> > > b/lib/librte_distributor/rte_distributor.h
> > > > index ec0d74a..844d325 100644
> > > > --- a/lib/librte_distributor/rte_distributor.h
> > > > +++ b/lib/librte_distributor/rte_distributor.h
> > > > @@ -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
> > > >
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-05 17:24 ` jigsaw
@ 2014-11-06 9:22 ` Bruce Richardson
2014-11-06 10:14 ` jigsaw
2014-11-06 10:36 ` Thomas Monjalon
0 siblings, 2 replies; 25+ messages in thread
From: Bruce Richardson @ 2014-11-06 9:22 UTC (permalink / raw)
To: jigsaw; +Cc: dev
On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> 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?
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
>
> thx &
> rgds,
>
> -qinglai
>
> On Wed, Nov 5, 2014 at 6:36 PM, Bruce Richardson <bruce.richardson@intel.com
> > wrote:
>
> > On Wed, Nov 05, 2014 at 05:11:51PM +0200, jigsaw wrote:
> > > 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:
> > >
> > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > index e8f9bfc..5b13c0b 100644
> > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > @@ -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?
> >
> > 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
> >
> > >
> > > 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 <
> > bruce.richardson@intel.com
> > > > wrote:
> > >
> > > > On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > > > > 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
> > > >
> > > > > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > > > > ---
> > > > > 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(-)
> > > > >
> > > > > diff --git a/app/test/test_distributor.c
> > b/app/test/test_distributor.c
> > > > > index ce06436..6ea4943 100644
> > > > > --- a/app/test/test_distributor.c
> > > > > +++ b/app/test/test_distributor.c
> > > > > @@ -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;
> > > > > diff --git a/app/test/test_distributor_perf.c
> > > > b/app/test/test_distributor_perf.c
> > > > > index b04864c..507e446 100644
> > > > > --- a/app/test/test_distributor_perf.c
> > > > > +++ b/app/test/test_distributor_perf.c
> > > > > @@ -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;
> > > > > diff --git a/lib/librte_distributor/rte_distributor.c
> > > > b/lib/librte_distributor/rte_distributor.c
> > > > > index 585ff88..78c92bd 100644
> > > > > --- a/lib/librte_distributor/rte_distributor.c
> > > > > +++ b/lib/librte_distributor/rte_distributor.c
> > > > > @@ -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);
> > > > > diff --git a/lib/librte_distributor/rte_distributor.h
> > > > b/lib/librte_distributor/rte_distributor.h
> > > > > index ec0d74a..844d325 100644
> > > > > --- a/lib/librte_distributor/rte_distributor.h
> > > > > +++ b/lib/librte_distributor/rte_distributor.h
> > > > > @@ -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
> > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-06 9:22 ` Bruce Richardson
@ 2014-11-06 10:14 ` jigsaw
2014-11-06 10:36 ` Thomas Monjalon
1 sibling, 0 replies; 25+ messages in thread
From: jigsaw @ 2014-11-06 10:14 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
OK understood. Thanks. -qinglai
On Thu, Nov 6, 2014 at 11:22 AM, Bruce Richardson <
bruce.richardson@intel.com> wrote:
> On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > 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?
>
> 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
>
> >
> > thx &
> > rgds,
> >
> > -qinglai
> >
> > On Wed, Nov 5, 2014 at 6:36 PM, Bruce Richardson <
> bruce.richardson@intel.com
> > > wrote:
> >
> > > On Wed, Nov 05, 2014 at 05:11:51PM +0200, jigsaw wrote:
> > > > 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:
> > > >
> > > > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > > > index e8f9bfc..5b13c0b 100644
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -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?
> > >
> > > 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
> > >
> > > >
> > > > 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 <
> > > bruce.richardson@intel.com
> > > > > wrote:
> > > >
> > > > > On Wed, Nov 05, 2014 at 03:30:37PM +0200, Qinglai Xiao wrote:
> > > > > > 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
> > > > >
> > > > > > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
> > > > > > ---
> > > > > > 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(-)
> > > > > >
> > > > > > diff --git a/app/test/test_distributor.c
> > > b/app/test/test_distributor.c
> > > > > > index ce06436..6ea4943 100644
> > > > > > --- a/app/test/test_distributor.c
> > > > > > +++ b/app/test/test_distributor.c
> > > > > > @@ -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;
> > > > > > diff --git a/app/test/test_distributor_perf.c
> > > > > b/app/test/test_distributor_perf.c
> > > > > > index b04864c..507e446 100644
> > > > > > --- a/app/test/test_distributor_perf.c
> > > > > > +++ b/app/test/test_distributor_perf.c
> > > > > > @@ -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;
> > > > > > diff --git a/lib/librte_distributor/rte_distributor.c
> > > > > b/lib/librte_distributor/rte_distributor.c
> > > > > > index 585ff88..78c92bd 100644
> > > > > > --- a/lib/librte_distributor/rte_distributor.c
> > > > > > +++ b/lib/librte_distributor/rte_distributor.c
> > > > > > @@ -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);
> > > > > > diff --git a/lib/librte_distributor/rte_distributor.h
> > > > > b/lib/librte_distributor/rte_distributor.h
> > > > > > index ec0d74a..844d325 100644
> > > > > > --- a/lib/librte_distributor/rte_distributor.h
> > > > > > +++ b/lib/librte_distributor/rte_distributor.h
> > > > > > @@ -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
> > > > > >
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-06 9:22 ` Bruce Richardson
2014-11-06 10:14 ` jigsaw
@ 2014-11-06 10:36 ` Thomas Monjalon
2014-11-06 12:36 ` [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor Qinglai Xiao
` (2 more replies)
1 sibling, 3 replies; 25+ messages in thread
From: Thomas Monjalon @ 2014-11-06 10:36 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, jigsaw
2014-11-06 09:22, Bruce Richardson:
> On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > 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
--
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 10:36 ` Thomas Monjalon
@ 2014-11-06 12:36 ` Qinglai Xiao
2014-11-06 13:59 ` Bruce Richardson
2014-11-06 13:55 ` [dpdk-dev] [PATCH] distributor: add comments to make code more readable Bruce Richardson
2014-11-06 13:57 ` [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor Bruce Richardson
2 siblings, 1 reply; 25+ messages in thread
From: Qinglai Xiao @ 2014-11-06 12:36 UTC (permalink / raw)
To: Thomas Monjalon, Bruce Richardson; +Cc: dev
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" <thomas.monjalon@6wind.com>
发送时间: 2014/11/6 12:36
收件人: "Bruce Richardson" <bruce.richardson@intel.com>
抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 09:22, Bruce Richardson:
> On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > 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
--
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* [dpdk-dev] [PATCH] distributor: add comments to make code more readable
2014-11-06 10:36 ` Thomas Monjalon
2014-11-06 12:36 ` [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor Qinglai Xiao
@ 2014-11-06 13:55 ` Bruce Richardson
2014-11-07 14:08 ` Thomas Monjalon
2014-11-06 13:57 ` [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor Bruce Richardson
2 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-06 13:55 UTC (permalink / raw)
To: dev
From: "Bruce Richardson" <bruce.richardson@intel.com>
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 <bruce.richardson@intel.com>
---
lib/librte_distributor/rte_distributor.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/lib/librte_distributor/rte_distributor.c b/lib/librte_distributor/rte_distributor.c
index 585ff88..656ee5c 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -92,6 +92,7 @@ struct rte_distributor {
unsigned num_workers; /**< Number of workers polling */
uint32_t in_flight_tags[RTE_MAX_LCORE];
+ /**< Tracks the tag being processed per core, 0 == no pkt */
struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
union rte_distributor_buffer bufs[RTE_MAX_LCORE];
@@ -282,10 +283,22 @@ rte_distributor_process(struct rte_distributor *d,
next_mb = mbufs[next_idx++];
next_value = (((int64_t)(uintptr_t)next_mb)
<< RTE_DISTRIB_FLAG_BITS);
+ /*
+ * Set the low bit on the tag, so we can guarantee that
+ * we never store a tag value of zero. That means we can
+ * use the zero-value to indicate that no packet is
+ * being processed by a worker.
+ */
new_tag = (next_mb->hash.rss | 1);
uint32_t match = 0;
unsigned i;
+ /*
+ * to scan for a match use "xor" and "not" to get a 0/1
+ * value, then use shifting to merge to single "match"
+ * variable, where a one-bit indicates a match for the
+ * worker given by the bit-position
+ */
for (i = 0; i < d->num_workers; i++)
match |= (!(d->in_flight_tags[i] ^ new_tag)
<< i);
--
2.1.1
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor.
2014-11-06 10:36 ` Thomas Monjalon
2014-11-06 12:36 ` [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor Qinglai Xiao
2014-11-06 13:55 ` [dpdk-dev] [PATCH] distributor: add comments to make code more readable Bruce Richardson
@ 2014-11-06 13:57 ` Bruce Richardson
2 siblings, 0 replies; 25+ messages in thread
From: Bruce Richardson @ 2014-11-06 13:57 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, jigsaw
On Thu, Nov 06, 2014 at 11:36:09AM +0100, Thomas Monjalon wrote:
> 2014-11-06 09:22, Bruce Richardson:
> > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > > 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.
>
I've sent a patch adding in a couple of comments where I thought some additional
clarification might been needed. Any other places where more info is needed, just
let me know, and I'll be happy to patch that extra info in too.
/Bruce
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 12:36 ` [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor Qinglai Xiao
@ 2014-11-06 13:59 ` Bruce Richardson
2014-11-06 18:01 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-06 13:59 UTC (permalink / raw)
To: Qinglai Xiao; +Cc: dev
On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> 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
>
> thx &
> rgds
> -qinglai
>
> -----原始邮件-----
> 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> 发送时间: 2014/11/6 12:36
> 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback tolibrte_distributor.
>
> 2014-11-06 09:22, Bruce Richardson:
> > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > > 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
> --
> Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 13:59 ` Bruce Richardson
@ 2014-11-06 18:01 ` jigsaw
2014-11-06 19:52 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-06 18:01 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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
On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <bruce.richardson@intel.com
> wrote:
> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> > 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
>
> >
> > thx &
> > rgds
> > -qinglai
> >
> > -----原始邮件-----
> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> > 发送时间: 2014/11/6 12:36
> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback
> tolibrte_distributor.
> >
> > 2014-11-06 09:22, Bruce Richardson:
> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > > >
> 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
> > --
> > Thomas
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 18:01 ` jigsaw
@ 2014-11-06 19:52 ` jigsaw
2014-11-07 9:45 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-06 19:52 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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
On Thu, Nov 6, 2014 at 8:01 PM, jigsaw <jigsaw@gmail.com> wrote:
> 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
>
> On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
>
>> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
>> > 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
>>
>> >
>> > thx &
>> > rgds
>> > -qinglai
>> >
>> > -----原始邮件-----
>> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
>> > 发送时间: 2014/11/6 12:36
>> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
>> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
>> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback
>> tolibrte_distributor.
>> >
>> > 2014-11-06 09:22, Bruce Richardson:
>> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
>> > > >
>> 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
>> > --
>> > Thomas
>>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-06 19:52 ` jigsaw
@ 2014-11-07 9:45 ` Bruce Richardson
2014-11-07 12:38 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-07 9:45 UTC (permalink / raw)
To: jigsaw; +Cc: dev
On Thu, Nov 06, 2014 at 09:52:25PM +0200, jigsaw wrote:
> 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
>
> On Thu, Nov 6, 2014 at 8:01 PM, jigsaw <jigsaw@gmail.com> wrote:
>
> > 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
> >
> > On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <
> > bruce.richardson@intel.com> wrote:
> >
> >> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> >> > 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
> >>
> >> >
> >> > thx &
> >> > rgds
> >> > -qinglai
> >> >
> >> > -----原始邮件-----
> >> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> >> > 发送时间: 2014/11/6 12:36
> >> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> >> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> >> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback
> >> tolibrte_distributor.
> >> >
> >> > 2014-11-06 09:22, Bruce Richardson:
> >> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> >> > > >
> >> 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
> >> > --
> >> > Thomas
> >>
> >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-07 9:45 ` Bruce Richardson
@ 2014-11-07 12:38 ` jigsaw
2014-11-07 13:53 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-07 12:38 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
Hi Bruce,
>> 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
On Fri, Nov 7, 2014 at 11:45 AM, Bruce Richardson <
bruce.richardson@intel.com> wrote:
> On Thu, Nov 06, 2014 at 09:52:25PM +0200, jigsaw wrote:
> > 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
>
> >
> > On Thu, Nov 6, 2014 at 8:01 PM, jigsaw <jigsaw@gmail.com> wrote:
> >
> > > 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
> > >
> > > On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <
> > > bruce.richardson@intel.com> wrote:
> > >
> > >> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> > >> > 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
> > >>
> > >> >
> > >> > thx &
> > >> > rgds
> > >> > -qinglai
> > >> >
> > >> > -----原始邮件-----
> > >> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> > >> > 发送时间: 2014/11/6 12:36
> > >> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> > >> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> > >> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback
> > >> tolibrte_distributor.
> > >> >
> > >> > 2014-11-06 09:22, Bruce Richardson:
> > >> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > >> > > >
> > >>
> 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
> > >> > --
> > >> > Thomas
> > >>
> > >
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-07 12:38 ` jigsaw
@ 2014-11-07 13:53 ` Bruce Richardson
2014-11-07 14:31 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-07 13:53 UTC (permalink / raw)
To: jigsaw; +Cc: dev
On Fri, Nov 07, 2014 at 02:38:13PM +0200, jigsaw wrote:
> Hi Bruce,
>
> >> 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.
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
>
> thx &
> rgds,
> -qinglai
>
>
> On Fri, Nov 7, 2014 at 11:45 AM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
>
> > On Thu, Nov 06, 2014 at 09:52:25PM +0200, jigsaw wrote:
> > > 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
> >
> > >
> > > On Thu, Nov 6, 2014 at 8:01 PM, jigsaw <jigsaw@gmail.com> wrote:
> > >
> > > > 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
> > > >
> > > > On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <
> > > > bruce.richardson@intel.com> wrote:
> > > >
> > > >> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> > > >> > 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
> > > >>
> > > >> >
> > > >> > thx &
> > > >> > rgds
> > > >> > -qinglai
> > > >> >
> > > >> > -----原始邮件-----
> > > >> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> > > >> > 发送时间: 2014/11/6 12:36
> > > >> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> > > >> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> > > >> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback
> > > >> tolibrte_distributor.
> > > >> >
> > > >> > 2014-11-06 09:22, Bruce Richardson:
> > > >> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > > >> > > >
> > > >>
> > 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
> > > >> > --
> > > >> > Thomas
> > > >>
> > > >
> > > >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] distributor: add comments to make code more readable
2014-11-06 13:55 ` [dpdk-dev] [PATCH] distributor: add comments to make code more readable Bruce Richardson
@ 2014-11-07 14:08 ` Thomas Monjalon
2014-11-07 14:31 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Thomas Monjalon @ 2014-11-07 14:08 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev, Qinglai Xiao
2014-11-06 13:55, Bruce Richardson:
> From: "Bruce Richardson" <bruce.richardson@intel.com>
>
> 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 <bruce.richardson@intel.com>
Applied
Qinglai, if you change the design, you'll now have some comments to update ;)
Thanks
--
Thomas
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-07 13:53 ` Bruce Richardson
@ 2014-11-07 14:31 ` jigsaw
[not found] ` <20141107144410.GC12092@bricha3-MOBL3>
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-07 14:31 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
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.
diff --git a/lib/librte_distributor/rte_distributor.c
b/lib/librte_distributor/rte_di
index 585ff88..d606bcf 100644
--- a/lib/librte_distributor/rte_distributor.c
+++ b/lib/librte_distributor/rte_distributor.c
@@ -92,6 +92,8 @@ struct rte_distributor {
unsigned num_workers; /**< Number of workers
polling */
uint32_t in_flight_tags[RTE_MAX_LCORE];
+ 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) {
@@ -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;
On Fri, Nov 7, 2014 at 3:53 PM, Bruce Richardson <bruce.richardson@intel.com
> wrote:
> On Fri, Nov 07, 2014 at 02:38:13PM +0200, jigsaw wrote:
> > Hi Bruce,
> >
> > >> 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.
>
> 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
>
> >
> > thx &
> > rgds,
> > -qinglai
> >
> >
> > On Fri, Nov 7, 2014 at 11:45 AM, Bruce Richardson <
> > bruce.richardson@intel.com> wrote:
> >
> > > On Thu, Nov 06, 2014 at 09:52:25PM +0200, jigsaw wrote:
> > > > 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
> > >
> > > >
> > > > On Thu, Nov 6, 2014 at 8:01 PM, jigsaw <jigsaw@gmail.com> wrote:
> > > >
> > > > > 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
> > > > >
> > > > > On Thu, Nov 6, 2014 at 3:59 PM, Bruce Richardson <
> > > > > bruce.richardson@intel.com> wrote:
> > > > >
> > > > >> On Thu, Nov 06, 2014 at 02:36:09PM +0200, Qinglai Xiao wrote:
> > > > >> > 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
> > > > >>
> > > > >> >
> > > > >> > thx &
> > > > >> > rgds
> > > > >> > -qinglai
> > > > >> >
> > > > >> > -----原始邮件-----
> > > > >> > 发件人: "Thomas Monjalon" <thomas.monjalon@6wind.com>
> > > > >> > 发送时间: 2014/11/6 12:36
> > > > >> > 收件人: "Bruce Richardson" <bruce.richardson@intel.com>
> > > > >> > 抄送: "dev@dpdk.org" <dev@dpdk.org>; "jigsaw" <jigsaw@gmail.com>
> > > > >> > 主题: Re: [dpdk-dev] [PATCH] Add user defined tag calculation
> callback
> > > > >> tolibrte_distributor.
> > > > >> >
> > > > >> > 2014-11-06 09:22, Bruce Richardson:
> > > > >> > > On Wed, Nov 05, 2014 at 07:24:13PM +0200, jigsaw wrote:
> > > > >> > > >
> > > > >>
> > >
> 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
> > > > >> > --
> > > > >> > Thomas
> > > > >>
> > > > >
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] [PATCH] distributor: add comments to make code more readable
2014-11-07 14:08 ` Thomas Monjalon
@ 2014-11-07 14:31 ` jigsaw
0 siblings, 0 replies; 25+ messages in thread
From: jigsaw @ 2014-11-07 14:31 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev
Hi Thomas,
Thanks for the reminder. I didn't notice the change in comments yet.
thx &
rgds,
-qinglai
On Fri, Nov 7, 2014 at 4:08 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:
> 2014-11-06 13:55, Bruce Richardson:
> > From: "Bruce Richardson" <bruce.richardson@intel.com>
> >
> > 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 <bruce.richardson@intel.com>
>
> Applied
>
> Qinglai, if you change the design, you'll now have some comments to update
> ;)
>
> Thanks
> --
> Thomas
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
[not found] ` <20141107144410.GC12092@bricha3-MOBL3>
@ 2014-11-07 14:52 ` jigsaw
2014-11-07 15:04 ` Bruce Richardson
0 siblings, 1 reply; 25+ messages in thread
From: jigsaw @ 2014-11-07 14:52 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
Yeah that's better. As below, right?
@@ -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 <bruce.richardson@intel.com
> wrote:
> On Fri, Nov 07, 2014 at 04:31:18PM +0200, jigsaw wrote:
> > 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.
> >
> > diff --git a/lib/librte_distributor/rte_distributor.c
> > b/lib/librte_distributor/rte_di
> > index 585ff88..d606bcf 100644
> > --- a/lib/librte_distributor/rte_distributor.c
> > +++ b/lib/librte_distributor/rte_distributor.c
> > @@ -92,6 +92,8 @@ struct rte_distributor {
> > unsigned num_workers; /**< Number of workers
> > polling */
> >
> > uint32_t in_flight_tags[RTE_MAX_LCORE];
> > + 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 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
>
> > << i);
> >
> > if (match) {
> > @@ -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;
> >
> >
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-07 14:52 ` jigsaw
@ 2014-11-07 15:04 ` Bruce Richardson
2014-11-07 15:18 ` jigsaw
0 siblings, 1 reply; 25+ messages in thread
From: Bruce Richardson @ 2014-11-07 15:04 UTC (permalink / raw)
To: jigsaw; +Cc: dev
On Fri, Nov 07, 2014 at 04:52:46PM +0200, jigsaw wrote:
> Yeah that's better. As below, right?
Yep.
>
> @@ -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 <bruce.richardson@intel.com
> > wrote:
>
> > On Fri, Nov 07, 2014 at 04:31:18PM +0200, jigsaw wrote:
> > > 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.
> > >
> > > diff --git a/lib/librte_distributor/rte_distributor.c
> > > b/lib/librte_distributor/rte_di
> > > index 585ff88..d606bcf 100644
> > > --- a/lib/librte_distributor/rte_distributor.c
> > > +++ b/lib/librte_distributor/rte_distributor.c
> > > @@ -92,6 +92,8 @@ struct rte_distributor {
> > > unsigned num_workers; /**< Number of workers
> > > polling */
> > >
> > > uint32_t in_flight_tags[RTE_MAX_LCORE];
> > > + 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 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
> >
> > > << i);
> > >
> > > if (match) {
> > > @@ -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;
> > >
> > >
> > >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor.
2014-11-07 15:04 ` Bruce Richardson
@ 2014-11-07 15:18 ` jigsaw
0 siblings, 0 replies; 25+ messages in thread
From: jigsaw @ 2014-11-07 15:18 UTC (permalink / raw)
To: Bruce Richardson; +Cc: dev
OK thanks Bruce. I will get the patch done in coming week. -qinglai
On Fri, Nov 7, 2014 at 5:04 PM, Bruce Richardson <bruce.richardson@intel.com
> wrote:
> On Fri, Nov 07, 2014 at 04:52:46PM +0200, jigsaw wrote:
> > Yeah that's better. As below, right?
>
> Yep.
>
> >
> > @@ -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 <
> bruce.richardson@intel.com
> > > wrote:
> >
> > > On Fri, Nov 07, 2014 at 04:31:18PM +0200, jigsaw wrote:
> > > > 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.
> > > >
> > > > diff --git a/lib/librte_distributor/rte_distributor.c
> > > > b/lib/librte_distributor/rte_di
> > > > index 585ff88..d606bcf 100644
> > > > --- a/lib/librte_distributor/rte_distributor.c
> > > > +++ b/lib/librte_distributor/rte_distributor.c
> > > > @@ -92,6 +92,8 @@ struct rte_distributor {
> > > > unsigned num_workers; /**< Number of workers
> > > > polling */
> > > >
> > > > uint32_t in_flight_tags[RTE_MAX_LCORE];
> > > > + 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 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
> > >
> > > > << i);
> > > >
> > > > if (match) {
> > > > @@ -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;
> > > >
> > > >
> > > >
> > >
>
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-11-07 15:08 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-05 13:30 [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor Qinglai Xiao
2014-11-05 14:27 ` Bruce Richardson
2014-11-05 15:11 ` jigsaw
2014-11-05 16:36 ` Bruce Richardson
2014-11-05 17:24 ` jigsaw
2014-11-06 9:22 ` Bruce Richardson
2014-11-06 10:14 ` jigsaw
2014-11-06 10:36 ` Thomas Monjalon
2014-11-06 12:36 ` [dpdk-dev] 答复: [PATCH] Add user defined tag calculation callback tolibrte_distributor Qinglai Xiao
2014-11-06 13:59 ` Bruce Richardson
2014-11-06 18:01 ` jigsaw
2014-11-06 19:52 ` jigsaw
2014-11-07 9:45 ` Bruce Richardson
2014-11-07 12:38 ` jigsaw
2014-11-07 13:53 ` Bruce Richardson
2014-11-07 14:31 ` jigsaw
[not found] ` <20141107144410.GC12092@bricha3-MOBL3>
2014-11-07 14:52 ` jigsaw
2014-11-07 15:04 ` Bruce Richardson
2014-11-07 15:18 ` jigsaw
2014-11-06 13:55 ` [dpdk-dev] [PATCH] distributor: add comments to make code more readable Bruce Richardson
2014-11-07 14:08 ` Thomas Monjalon
2014-11-07 14:31 ` jigsaw
2014-11-06 13:57 ` [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor Bruce Richardson
2014-11-05 15:13 ` Ananyev, Konstantin
2014-11-05 15:24 ` jigsaw
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).