From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH] rte_delay_us can be replaced with user function Date: Thu, 22 Sep 2016 17:08:42 +0200 Message-ID: <4326235.prI1QteV26@xps13> References: <1469016644-6521-1-git-send-email-jozmarti@cisco.com> <2644416.cGCtye7R7u@xps13> <3d62cdb3b19648d8a4ed7e0ff4847db5@XCH-RCD-019.cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org To: jozmarti@cisco.com Return-path: Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id 008225681 for ; Thu, 22 Sep 2016 17:08:50 +0200 (CEST) Received: by mail-wm0-f47.google.com with SMTP id l132so153411746wmf.1 for ; Thu, 22 Sep 2016 08:08:50 -0700 (PDT) In-Reply-To: <3d62cdb3b19648d8a4ed7e0ff4847db5@XCH-RCD-019.cisco.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 2016-09-22 08:37, Jozef Martiniak -X: > Hello Thomas, > > > First a general comment: please check your patch with scripts/checkpatches.sh. > Done. checkpath.pl is taken from latest linux kernel... I can see these warnings: UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned' LEADING_SPACE: please, no spaces at the start of a line > > What will happen if we need to provide more builtin handlers? > > I still think that rte_delay_us_block can be exported and initialized > The idea was to make this function simple&&fast as possible. If user needs > more builtin handlers - he can implement this functionality within his delay > function. > > Btw there is also another patch with the same functionality (uses weak symbols) > which I would prefer: > https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch I don't like the VPP patch because it is hacky and weak symbols should be avoided for static linking. > Do you have any idea how should rte_delay_us_callback_register look like for > multiple handlers ? Yes, just as simple as an assignment: void rte_delay_us_callback_register(void (*func)(unsigned)) { rte_delay_us = func; } We just need to export rte_delay_us_block and call rte_delay_us_callback_register(rte_delay_us_block) at EAL initialization or in a constructor. > Other comments are accepted. > > Best regards, > Jozef > > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Wednesday, September 21, 2016 3:13 PM > To: Jozef Martiniak -X (jozmarti - PANTHEON TECHNOLOGIES at Cisco) > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] rte_delay_us can be replaced with user function > > Hi, > > I think this feature should enter in the release 16.11. > We just need to make sure it is implemented with the right API. > Do you have any comment about managing several builtin handlers? > > > 2016-09-13 22:04, Thomas Monjalon: > > Hi, > > > > Sorry for late review. > > This patch was in a summer hole :/ > > > > First a general comment: please check your patch with > > scripts/checkpatches.sh. > > In order to ease tracking of this patch, please increment the version > > when sending a new one in the same thread: > > git send-email -1 -v3 --annotate --to dev@dpdk.org \ > > --in-reply-to 1469016644-6521-1-git-send-email-jozmarti@cisco.com > > > > More comments below. > > > > 2016-07-20 14:10, jozmarti@cisco.com: > > > +void rte_delay_us_callback_register(void (*userfunc)(unsigned)) { > > > + if (userfunc == NULL) > > > + rte_delay_us = rte_delay_us_block; > > > > Here you are creating an exception for rte_delay_us_block which is > > mapped as a NULL handler. > > What will happen if we need to provide more builtin handlers? > > I still think that rte_delay_us_block can be exported and initialized > > as the default handler. Other opinions are obviously welcome. > > > > > + else > > > + rte_delay_us = userfunc; > > > +} > > > + > > > +static void __attribute__((constructor)) > > > +rte_timer_init(void) > > > +{ > > > + /* set rte_delay_us_block as a delay function */ > > > + rte_delay_us_callback_register(NULL); > > > +} > > > diff --git a/lib/librte_eal/common/include/generic/rte_cycles.h > > > b/lib/librte_eal/common/include/generic/rte_cycles.h > > > index 8cc21f2..7a45b58 100644 > > > --- a/lib/librte_eal/common/include/generic/rte_cycles.h > > > +++ b/lib/librte_eal/common/include/generic/rte_cycles.h > > > @@ -182,13 +182,16 @@ rte_get_timer_hz(void) } > > > > > > /** > > > + * > > > > useless newline > > > > > * Wait at least us microseconds. > > > + * This function can be replaced with user-defined function using > > > + * rte_delay_us_callback_register > > > > I think you can use @see to point to rte_delay_us_callback_register. > > > > > * > > > * @param us > > > * The number of microseconds to wait. > > > */ > > > void > > > -rte_delay_us(unsigned us); > > > +(*rte_delay_us)(unsigned us); > > > > > > /** > > > * Wait at least ms milliseconds. > > > @@ -202,4 +205,14 @@ rte_delay_ms(unsigned ms) > > > rte_delay_us(ms * 1000); > > > } > > > > > > +/** > > > + * Replace rte_delay_us with user defined function. > > > + * > > > + * @param userfunc > > > + * User function which replaces rte_delay_us. NULL restores > > > + * buildin block delay function. > > > > buildin -> builtin ? > > > > > + */ > > > +void rte_delay_us_callback_register(void(*userfunc)(unsigned)); > > > >