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 at 6wind.com] > Sent: Wednesday, September 21, 2016 3:13 PM > To: Jozef Martiniak -X (jozmarti - PANTHEON TECHNOLOGIES at Cisco) <jozmarti > at cisco.com> > Cc: dev at 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 at dpdk.org \ > > --in-reply-to 1469016644-6521-1-git-send-email-jozmarti at cisco.com > > > > More comments below. > > > > 2016-07-20 14:10, jozmarti at 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)); > > > >
