From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id 3C5FC2E89 for ; Wed, 5 Nov 2014 18:14:50 +0100 (CET) Received: by mail-wg0-f50.google.com with SMTP id z12so1460902wgg.37 for ; Wed, 05 Nov 2014 09:24:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=h04zFXRz0c7ap71aOJhd57eC9udz0AsvZuVlUUQUH0k=; b=0m35X2cHUdD8M+CzsKtMK/G3BD+a92EJ9XtHnSl+OOmQhgqawh7R9wfwK+GHj76P5e RSM7pzdvHrgkjM2KKIgxPZfqpDWDC1zj3rTdiwOEtA8lKISRn+dlr7ag8hzQpdQ5TDoM pMzlWH/xxLvWUZTRSjCmyhpp7BCWeaHm5cIHF8d1E50xU8kJWsqh10f4qv166dZ5WCjs A0cUXJ8M7+/LuoMgdO5GVnSSIWTjiJZ9vJKAmbviW+0tGwaSPbzvFtOREeMqnxDj6AbB czwGPl2YDvdCohEE5dzClUEj2zJ4wA/6ooakSNO/Y53tesOimMQDE+71cs8t3h9tXpHa mbVg== MIME-Version: 1.0 X-Received: by 10.180.97.65 with SMTP id dy1mr21663443wib.76.1415208253924; Wed, 05 Nov 2014 09:24:13 -0800 (PST) Received: by 10.27.86.144 with HTTP; Wed, 5 Nov 2014 09:24:13 -0800 (PST) In-Reply-To: <20141105163655.GC9856@bricha3-MOBL3> References: <1415194237-1219-1-git-send-email-jigsaw@gmail.com> <20141105142743.GB9856@bricha3-MOBL3> <20141105163655.GC9856@bricha3-MOBL3> Date: Wed, 5 Nov 2014 19:24:13 +0200 Message-ID: From: jigsaw To: Bruce Richardson Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] Add user defined tag calculation callback to librte_distributor. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Nov 2014 17:14:50 -0000 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 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 > > > > --- > > > > 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 > > > > > > > >