diff mbox

[dpdk-dev] Add user defined tag calculation callback to librte_distributor.

Message ID CAHVfvh4yAdF-aDnV+sh+PgJpEUW18+aF0=JoZDwJtGHhh=5ZzQ@mail.gmail.com (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Qinglai Xiao Nov. 5, 2014, 3:11 p.m. UTC
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:


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

Comments

Bruce Richardson Nov. 5, 2014, 4:36 p.m. UTC | #1
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
> > >
> >
Qinglai Xiao Nov. 5, 2014, 5:24 p.m. UTC | #2
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
> > > >
> > >
>
Bruce Richardson Nov. 6, 2014, 9:22 a.m. UTC | #3
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
> > > > >
> > > >
> >
Qinglai Xiao Nov. 6, 2014, 10:14 a.m. UTC | #4
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
> > > > > >
> > > > >
> > >
>
Thomas Monjalon Nov. 6, 2014, 10:36 a.m. UTC | #5
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
Qinglai Xiao Nov. 6, 2014, 12:36 p.m. UTC | #6
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
Bruce Richardson Nov. 6, 2014, 1:57 p.m. UTC | #7
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
Bruce Richardson Nov. 6, 2014, 1:59 p.m. UTC | #8
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
Qinglai Xiao Nov. 6, 2014, 6:01 p.m. UTC | #9
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
>
Qinglai Xiao Nov. 6, 2014, 7:52 p.m. UTC | #10
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
>>
>
>
Bruce Richardson Nov. 7, 2014, 9:45 a.m. UTC | #11
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
> >>
> >
> >
Qinglai Xiao Nov. 7, 2014, 12:38 p.m. UTC | #12
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
> > >>
> > >
> > >
>
Bruce Richardson Nov. 7, 2014, 1:53 p.m. UTC | #13
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
> > > >>
> > > >
> > > >
> >
diff mbox

Patch

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 */