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

[Bruce Richardson <bruce.richardson@intel.com>]

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