Hi Petri, Could you let me know which platform are you using and what's the command line of your l2_fwd test?
Kevin 2017-06-26 20:34 GMT+08:00 Maxim Uvarov <[email protected]>: > On 06/26/17 13:50, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > When init params are set to defaults... > > > > odp_init_param_init(&init); > > > > odp_init_global(&instance, &init, NULL); > > > > > > ... this patch causes -15% (about 1Mpps per cpu) packet rate degradation > with l2fwd. The timer poll function shows up as the second highest CPU > cycle consumer on perf. The defaults needs to be less aggressive: a > tradeoff between somewhat improved timer accuracy for some timer using > applications vs. performance degradation for all application using the > scheduler (also with the default inits)... > > > > The patch actually disables the polling when passing NULL as init param. > API defines that NULL and odp_init_param_init(&init) result the same config > ("all defaults"). This implementation mismatch helps now when most > applications pass NULL, and thus timer polling is kept disabled by default. > > > > When contributing to a central piece of data plane code, it would be > really important to measure packet rate before and after the change... > > > > > > -Petri > > > > Thank Petri, > > I opened bug to track this: > https://bugs.linaro.org/show_bug.cgi?id=3078 > > Maxim. > > > > > > > > > > > > From: Maxim Uvarov [mailto:[email protected]] > > Sent: Friday, June 23, 2017 5:23 PM > > To: Honnappa Nagarahalli <[email protected]> > > Cc: Brian Brooks <[email protected]>; Savolainen, Petri (Nokia - > FI/Espoo) <[email protected]>; lng-odp-forward < > [email protected]> > > Subject: Re: [lng-odp] [API-NEXT PATCH v4] timer: allow timer processing > to run on worker cores > > > > Merged to api-next. > > I think this patch is clean and we can do further improvements / tuning > later. > > Regards, > > Maxim. > > > > On 22 June 2017 at 21:48, Honnappa Nagarahalli <mailto: > [email protected]> wrote: > > On 22 June 2017 at 10:30, Maxim Uvarov <mailto:[email protected]> > wrote: > >> On 06/22/17 17:55, Brian Brooks wrote: > >>> On 06/22 10:27:01, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >>>> I was asking to make sure that performance impact has been checked > also when timers are not used, e.g. l2fwd performance before and after the > change. It would be also appropriate to test impact in the worst case: > l2fwd type application + a periodic 1sec timeout. Timer is on, but timeouts > come very unfrequently (compared to packets). > >>>> > >>>> It seems that no performance tests were run, although the change > affects performance of many applications (e.g. OFP has high packet rate > with timers). Configuration options should be set with defaults that are > acceptable trade-off between packet processing performance and timeout > accuracy. > >>> > >>> If timers are not used, the overhead is just checking a RO variable > >>> (post global init). If timers are used, CONFIG_ parameters have been > >>> provided. The defaults for these parameters came from the work to > >>> drastically reduce jitter of timer processing which is documented > >>> here [1] and presented at Linaro Connect here [2]. > >>> > >>> If you speculate that these defaults might need to be changed, e.g. > >>> l2fwd, we welcome collaboration and data. But, this is not a blocking > >>> issue for this patch right now. > >>> > >>> [1] https://docs.google.com/document/d/1sY7rOxqCNu- > bMqjBiT5_keAIohrX1ZW-eL0oGLAQ4OM/edit?usp=sharing > >>> [2] http://connect.linaro.org/resource/bud17/bud17-320/ > >>> > >> > >> 1) we have all adjustable configs here > >> ./platform/linux-generic/include/odp_config_internal.h > >> that might be also needs to be there. > >> > > That file has all the global config values. These are internal to this > > timer implementation, hence they do not need to be moved. > >> > >> 2) Do we need something special in CI to check different config values? > > > > Nope. > >> > >> 3) Why it's compile time config values and not run time? > > > > These config values are particular to this timer implementation. > > Similar to config values in > > ./platform/linux-generic/include/odp_config_internal.h, these also > > will be compile time constants. > > > >> > >> Maxim. > >> > >> > >>>> -Petri > >>>> > >>>> > >>>> From: Maxim Uvarov [mailto:mailto:[email protected]] > >>>> Sent: Thursday, June 22, 2017 11:22 AM > >>>> To: Honnappa Nagarahalli <mailto:[email protected]> > >>>> Cc: Savolainen, Petri (Nokia - FI/Espoo) <mailto:petri.savolainen@ > nokia.com>; lng-odp-forward <mailto:[email protected]> > >>>> Subject: Re: [lng-odp] [API-NEXT PATCH v4] timer: allow timer > processing to run on worker cores > >>>> > >>>> Petri, do you want to test performance before patch inclusion? > >>>> Maxim. > >>>> > >>>> On 21 June 2017 at 21:52, Honnappa Nagarahalli <mailto:mailto: > [email protected]> wrote: > >>>> We have not run any performance application. In our Linaro connect > >>>> meeting, we presented numbers on how it improves the timer resolution. > >>>> At this point, there is enough configuration options to control the > >>>> effect of calling timer in the scheduler. For applications that do not > >>>> want to use the timer, there should not be any change. For > >>>> applications that use timers non-frequently, the check frequency can > >>>> be controlled via the provided configuration options. > >>>> > >>>> On 20 June 2017 at 02:34, Savolainen, Petri (Nokia - FI/Espoo) > >>>> <mailto:mailto:[email protected]> wrote: > >>>>> Do you have some performance numbers? E.g. how much this slows down > an application which does not use timers (e.g. l2fwd), or an application > that uses only few, non-frequent timeouts? > >>>>> > >>>>> Additionally, init.h/feature.h is not yet in api-next - so this > would not build yet. > >>>>> > >>>>> > >>>>> -Petri > >>>>> > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: lng-odp [mailto:mailto:mailto:mailto:l > [email protected]] On Behalf Of > >>>>>> Honnappa Nagarahalli > >>>>>> Sent: Tuesday, June 20, 2017 7:07 AM > >>>>>> To: Bill Fischofer <mailto:mailto:[email protected]> > >>>>>> Cc: lng-odp-forward <mailto:mailto:[email protected]> > >>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH v4] timer: allow timer > processing > >>>>>> to run on worker cores > >>>>>> > >>>>>> Are you saying we should be good to merge this now? > >>>>>> > >>>>>> On 19 June 2017 at 17:42, Bill Fischofer <mailto:mailto: > [email protected]> > >>>>>> wrote: > >>>>>>> On Mon, Jun 19, 2017 at 4:19 PM, Honnappa Nagarahalli > >>>>>>> <mailto:mailto:[email protected]> wrote: > >>>>>>>> Hi Bill/Maxim, > >>>>>>>> I do not see any further comments, can we merge this to > api-next? > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Honnappa > >>>>> > >>>>> > >>>> > >> > > > > -- Thanks, Kevin
