DPDK patches and discussions
 help / color / mirror / Atom feed
From: jigsaw <jigsaw@gmail.com>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] Add in_flight_bitmask so as to use full 32 bits of tag.
Date: Mon, 10 Nov 2014 14:13:05 +0200	[thread overview]
Message-ID: <CAHVfvh6m2+LDYJuSABd2tbM_vrh3rOy-S6EjkfYJwQpJ11uFLw@mail.gmail.com> (raw)
In-Reply-To: <20141110110922.GB7036@bricha3-MOBL3>

OK thx Bruce, I will make 2 commits with one cover-letter.

On Mon, Nov 10, 2014 at 1:09 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Mon, Nov 10, 2014 at 12:38:32PM +0200, Qinglai Xiao wrote:
> > User application is advocated to set the newly introduced union field
> > mbuf->hash.usr as flow id, which is uint32_t.
> > With introduction of in_flight_bitmask, the whole 32 bits of tag can
> > be used.
> >
> > Further more, this patch fixed the integer overflow when finding the
> > matched tags.
> >
> > Note that currently librte_distributor supports up to 64 worker
> > threads. If more workers are needed, the size of in_flight_bitmask and
> > the algorithm of finding matched tag must be revised.
> >
> > Signed-off-by: Qinglai Xiao <jigsaw@gmail.com>
>
> Hi,
>
> this would probably be better as two patches rather than one. One patch to
> add the hash.usr field, and then use it in the distributor. The change to
> the distributor to add the bit mask to allow all bits of the tag to be used
> should then go as a separate patch.
>
> Regards,
> /Bruce
>
>
> > ---
> >  app/test/test_distributor.c              |   18 ++++++------
> >  app/test/test_distributor_perf.c         |    4 +-
> >  lib/librte_distributor/rte_distributor.c |   45
> +++++++++++++++++++++--------
> >  lib/librte_distributor/rte_distributor.h |    3 ++
> >  lib/librte_mbuf/rte_mbuf.h               |    1 +
> >  5 files changed, 47 insertions(+), 24 deletions(-)
> >
> > diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
> > index ce06436..9e8c06d 100644
> > --- a/app/test/test_distributor.c
> > +++ b/app/test/test_distributor.c
> > @@ -120,7 +120,7 @@ sanity_test(struct rte_distributor *d, struct
> rte_mempool *p)
> >       /* now set all hash values in all buffers to zero, so all pkts go
> to the
> >        * one worker thread */
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = 0;
> > +             bufs[i]->hash.usr = 0;
> >
> >       rte_distributor_process(d, bufs, BURST);
> >       rte_distributor_flush(d);
> > @@ -142,7 +142,7 @@ sanity_test(struct rte_distributor *d, struct
> rte_mempool *p)
> >       if (rte_lcore_count() >= 3) {
> >               clear_packet_count();
> >               for (i = 0; i < BURST; i++)
> > -                     bufs[i]->hash.rss = (i & 1) << 8;
> > +                     bufs[i]->hash.usr = (i & 1) << 8;
> >
> >               rte_distributor_process(d, bufs, BURST);
> >               rte_distributor_flush(d);
> > @@ -167,7 +167,7 @@ sanity_test(struct rte_distributor *d, struct
> rte_mempool *p)
> >        * so load gets distributed */
> >       clear_packet_count();
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = i;
> > +             bufs[i]->hash.usr = i;
> >
> >       rte_distributor_process(d, bufs, BURST);
> >       rte_distributor_flush(d);
> > @@ -199,7 +199,7 @@ sanity_test(struct rte_distributor *d, struct
> rte_mempool *p)
> >               return -1;
> >       }
> >       for (i = 0; i < BIG_BATCH; i++)
> > -             many_bufs[i]->hash.rss = i << 2;
> > +             many_bufs[i]->hash.usr = i << 2;
> >
> >       for (i = 0; i < BIG_BATCH/BURST; i++) {
> >               rte_distributor_process(d, &many_bufs[i*BURST], BURST);
> > @@ -280,7 +280,7 @@ sanity_test_with_mbuf_alloc(struct rte_distributor
> *d, struct rte_mempool *p)
> >               while (rte_mempool_get_bulk(p, (void *)bufs, BURST) < 0)
> >                       rte_distributor_process(d, NULL, 0);
> >               for (j = 0; j < BURST; j++) {
> > -                     bufs[j]->hash.rss = (i+j) << 1;
> > +                     bufs[j]->hash.usr = (i+j) << 1;
> >                       rte_mbuf_refcnt_set(bufs[j], 1);
> >               }
> >
> > @@ -359,7 +359,7 @@ sanity_test_with_worker_shutdown(struct
> rte_distributor *d,
> >       /* now set all hash values in all buffers to zero, so all pkts go
> to the
> >        * one worker thread */
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = 0;
> > +             bufs[i]->hash.usr = 0;
> >
> >       rte_distributor_process(d, bufs, BURST);
> >       /* at this point, we will have processed some packets and have a
> full
> > @@ -372,7 +372,7 @@ sanity_test_with_worker_shutdown(struct
> rte_distributor *d,
> >               return -1;
> >       }
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = 0;
> > +             bufs[i]->hash.usr = 0;
> >
> >       /* get worker zero to quit */
> >       zero_quit = 1;
> > @@ -416,7 +416,7 @@ test_flush_with_worker_shutdown(struct
> rte_distributor *d,
> >       /* now set all hash values in all buffers to zero, so all pkts go
> to the
> >        * one worker thread */
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = 0;
> > +             bufs[i]->hash.usr = 0;
> >
> >       rte_distributor_process(d, bufs, BURST);
> >       /* at this point, we will have processed some packets and have a
> full
> > @@ -488,7 +488,7 @@ quit_workers(struct rte_distributor *d, struct
> rte_mempool *p)
> >       zero_quit = 0;
> >       quit = 1;
> >       for (i = 0; i < num_workers; i++)
> > -             bufs[i]->hash.rss = i << 1;
> > +             bufs[i]->hash.usr = i << 1;
> >       rte_distributor_process(d, bufs, num_workers);
> >
> >       rte_mempool_put_bulk(p, (void *)bufs, num_workers);
> > diff --git a/app/test/test_distributor_perf.c
> b/app/test/test_distributor_perf.c
> > index b04864c..48ee344 100644
> > --- a/app/test/test_distributor_perf.c
> > +++ b/app/test/test_distributor_perf.c
> > @@ -159,7 +159,7 @@ perf_test(struct rte_distributor *d, struct
> rte_mempool *p)
> >       }
> >       /* ensure we have different hash value for each pkt */
> >       for (i = 0; i < BURST; i++)
> > -             bufs[i]->hash.rss = i;
> > +             bufs[i]->hash.usr = i;
> >
> >       start = rte_rdtsc();
> >       for (i = 0; i < (1<<ITER_POWER); i++)
> > @@ -198,7 +198,7 @@ quit_workers(struct rte_distributor *d, struct
> rte_mempool *p)
> >
> >       quit = 1;
> >       for (i = 0; i < num_workers; i++)
> > -             bufs[i]->hash.rss = i << 1;
> > +             bufs[i]->hash.usr = i << 1;
> >       rte_distributor_process(d, bufs, num_workers);
> >
> >       rte_mempool_put_bulk(p, (void *)bufs, num_workers);
> > diff --git a/lib/librte_distributor/rte_distributor.c
> b/lib/librte_distributor/rte_distributor.c
> > index 656ee5c..a84ea97 100644
> > --- a/lib/librte_distributor/rte_distributor.c
> > +++ b/lib/librte_distributor/rte_distributor.c
> > @@ -92,7 +92,13 @@ 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
> */
> > +             /**< Tracks the tag being processed per core */
> > +     uint64_t in_flight_bitmask;
> > +             /**< on/off bits for in-flight tags.
> > +              * Note that if RTE_MAX_LCORE is larger than 64 then
> > +              * the bitmask has to expand.
> > +              */
> > +
> >       struct rte_distributor_backlog backlog[RTE_MAX_LCORE];
> >
> >       union rte_distributor_buffer bufs[RTE_MAX_LCORE];
> > @@ -189,6 +195,7 @@ static inline void
> >  handle_worker_shutdown(struct rte_distributor *d, unsigned wkr)
> >  {
> >       d->in_flight_tags[wkr] = 0;
> > +     d->in_flight_bitmask &= ~(1UL << wkr);
> >       d->bufs[wkr].bufptr64 = 0;
> >       if (unlikely(d->backlog[wkr].count != 0)) {
> >               /* On return of a packet, we need to move the
> > @@ -211,7 +218,10 @@ handle_worker_shutdown(struct rte_distributor *d,
> unsigned wkr)
> >                       pkts[i] = (void *)((uintptr_t)(bl->pkts[idx] >>
> >                                       RTE_DISTRIB_FLAG_BITS));
> >               }
> > -             /* recursive call */
> > +             /* recursive call.
> > +              * Note that the tags were set before first level call
> > +              * to rte_distributor_process.
> > +              */
> >               rte_distributor_process(d, pkts, i);
> >               bl->count = bl->start = 0;
> >       }
> > @@ -242,6 +252,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_bitmask &= ~(1UL << wkr);
> >                       }
> >                       oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
> >               } else if (data & RTE_DISTRIB_RETURN_BUF) {
> > @@ -284,17 +295,21 @@ rte_distributor_process(struct rte_distributor *d,
> >                       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.
> > +                      * User is advocated to set tag vaue for each
> > +                      * mbuf before calling rte_distributor_process.
> > +                      * User defined tags are used to identify flows,
> > +                      * or sessions.
> >                        */
> > -                     new_tag = (next_mb->hash.rss | 1);
> > +                     new_tag = next_mb->hash.usr;
> >
> > -                     uint32_t match = 0;
> > +                     /*
> > +                      * Note that if RTE_MAX_LCORE is larger than 64
> then
> > +                      * the size of match has to be expanded.
> > +                      */
> > +                     uint64_t match = 0;
> >                       unsigned i;
> >                       /*
> > -                      * to scan for a match use "xor" and "not" to get
> a 0/1
> > +                      * 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
> > @@ -303,9 +318,11 @@ rte_distributor_process(struct rte_distributor *d,
> >                               match |= (!(d->in_flight_tags[i] ^ new_tag)
> >                                       << i);
> >
> > +                     /* Only turned-on bits are considered as match */
> > +                     match &= d->in_flight_bitmask;
> >                       if (match) {
> >                               next_mb = NULL;
> > -                             unsigned worker = __builtin_ctz(match);
> > +                             unsigned worker = __builtin_ctzl(match);
> >                               if (add_to_backlog(&d->backlog[worker],
> >                                               next_value) < 0)
> >                                       next_idx--;
> > @@ -322,6 +339,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 |= (1UL << wkr);
> >                               next_mb = NULL;
> >                       }
> >                       oldbuf = data >> RTE_DISTRIB_FLAG_BITS;
> > @@ -379,11 +397,12 @@ rte_distributor_returned_pkts(struct
> rte_distributor *d,
> >  static inline unsigned
> >  total_outstanding(const struct rte_distributor *d)
> >  {
> > -     unsigned wkr, total_outstanding = 0;
> > +     unsigned wkr, total_outstanding;
> >
> > +     total_outstanding = __builtin_popcountl(d->in_flight_bitmask);
> >       for (wkr = 0; wkr < d->num_workers; wkr++)
> > -             total_outstanding += d->backlog[wkr].count +
> > -                             !!(d->in_flight_tags[wkr]);
> > +             total_outstanding += d->backlog[wkr].count;
> > +
> >       return total_outstanding;
> >  }
> >
> > diff --git a/lib/librte_distributor/rte_distributor.h
> b/lib/librte_distributor/rte_distributor.h
> > index ec0d74a..a29c506 100644
> > --- a/lib/librte_distributor/rte_distributor.h
> > +++ b/lib/librte_distributor/rte_distributor.h
> > @@ -87,6 +87,9 @@ rte_distributor_create(const char *name, unsigned
> socket_id,
> >   * Process a set of packets by distributing them among workers that
> request
> >   * packets. The distributor will ensure that no two packets that have
> the
> >   * same flow id, or tag, in the mbuf will be procesed at the same time.
> > + * The user is advocated to set tag for each mbuf. If user doesn't set
> the
> > + * tag, the tag value can be various values depending on driver
> implementation
> > + * and configuration.
> >   *
> >   * This is not multi-thread safe and should only be called on a single
> lcore.
> >   *
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index e8f9bfc..f5f8658 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 usr;     /**< User defined tags. See
> @rte_distributor_process */
> >       } hash;                   /**< hash information */
> >
> >       /* second cache line - fields only used in slow path or on TX */
> > --
> > 1.7.1
> >
>

      reply	other threads:[~2014-11-10 12:03 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-10 10:38 Qinglai Xiao
2014-11-10 11:09 ` Bruce Richardson
2014-11-10 12:13   ` jigsaw [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHVfvh6m2+LDYJuSABd2tbM_vrh3rOy-S6EjkfYJwQpJ11uFLw@mail.gmail.com \
    --to=jigsaw@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).