[Xorp-hackers] xorp-ospf performance issues: busy-spins & packet floods.

[Pavlin Radoslavov]

Ben Greear <greearb at candelatech.com> wrote:

> Ben Greear wrote:
> > Pavlin Radoslavov wrote:
> >
> >> See SelectorList::get_ready_priority() inside libxorp/selector.cc
> >> for example.
> >> It is used to find the best priority of a file descriptor that is
> >> ready.
> >>
> The attached patch still lets ospf busy-spin, but at least now it's spinning
> doing reads and
> writes of some SCTP payload instead of just spinning w/out obviously doing
> anything...
> 
> It should get rid of several time calls and some static global variables as
> well.

Without investigating the patch into details, one of the things it
does is commenting-out the priority-related mechanism. This
completely changes the current semantics. As I mentioned in my
previous email, it is better to talk first with Mark on the
subject.

Thanks,
Pavlin

> Thanks,
> Ben
> 
> >>
> >
> > What do you think about having the run() method execute the timer(s),
> > then tasks, then the selector.   I'd run the selector last since it can
> > sleep.
> > We can get rid of the get_ready_priority() all together that way.  If you
> > really wanted to not handle all of the available fds at once (ie, allow low
> > priorities to be skipped), then we could just not process all of them.  I
> > can't
> > think of a good reason to do that, however.
> >
> > Then, we have only one call to select(), as well as making sure that each
> > of the run-queues (timers, tasks, selectors) gets some time to run.  I am
> > suspicious
> > that the loop I see in ospf has to do with an endless timer loop where the
> > ospf
> > code keeps trying to fire timers but never reads/writes the sockets.  I also
> > assume
> > that if it could actually process the sockets, it's timers would be
> > satisfied and quit
> > re-arming themselves.  This is idle conjecture at this time, but it would
> > fit the
> > strace log I saw...
> >
> > I think I can code this up and test it out if it's something you'd
> > consider merging.
> >
> > Thanks,
> > Ben
> >
> >
> 
> 
> -- 
> Ben Greear <greearb at candelatech.com> Candela Technologies Inc
> http://www.candelatech.com
> 
> 
> Index: eventloop.cc
> ===================================================================
> RCS file: /cvs/xorp/libxorp/eventloop.cc,v
> retrieving revision 1.25
> diff -u -r1.25 eventloop.cc
> --- eventloop.cc	28 Sep 2007 06:34:50 -0000	1.25
> +++ eventloop.cc	21 Oct 2007 01:04:37 -0000
> @@ -29,11 +29,6 @@
>  //
>  int eventloop_instance_count;
>  
> -//
> -// Last call to EventLoop::run.  0 is a special value that indicates
> -// EventLoop::run has not been called.
> -//
> -static time_t last_ev_run;
>  
>  EventLoop::EventLoop()
>      : _clock(new SystemClock), _timer_list(_clock),
> @@ -45,7 +40,8 @@
>  {
>      XLOG_ASSERT(eventloop_instance_count == 0);
>      eventloop_instance_count++;
> -    last_ev_run = 0;
> +    last_ev_run = TimeVal::ZERO();
> +    last_warned = 0;
>  }
>  
>  EventLoop::~EventLoop()
> @@ -63,25 +59,47 @@
>  void
>  EventLoop::run()
>  {
> -    const time_t MAX_ALLOWED = 2;
> -    static time_t last_warned;
> +    static const time_t MAX_ALLOWED = 2;
>  
> -    if (last_ev_run == 0)
> -	last_ev_run = time(0);
> +    _timer_list.advance_time();
> +    TimeVal now;
> +    _timer_list.current_time(now);
>  
> -    time_t now = time(0);
> -    time_t diff = now - last_ev_run;
> +    if (last_ev_run.secs() == 0)
> +	last_ev_run = now;
>  
> -    if (now - last_warned > 0 && (diff > MAX_ALLOWED)) {
> +    int32_t diff = now.secs() - last_ev_run.secs();
> +    if (now.secs() - last_warned > 0 && (diff > MAX_ALLOWED)) {
>  	XLOG_WARNING("%d seconds between calls to EventLoop::run", (int)diff);
> -	last_warned = now;
> +	last_warned = now.secs();
>      }
>  
>      TimeVal t;
>  
> -    _timer_list.advance_time();
>      _timer_list.get_next_delay(t);
>  
> +    // Run timers if they need it.
> +    if (t == TimeVal::ZERO()) {
> +	_timer_list.run();
> +    }
> +
> +    if (!_task_list.empty()) {
> +	_task_list.run();
> +	if (!_task_list.empty()) {
> +	    // Run task again as soon as possible.
> +	    t = TimeVal::ZERO();
> +	}
> +    }
> +
> +#ifdef HOST_OS_WINDOWS
> +    _win_dispatcher.wait_and_dispatch(t);
> +#else
> +    _selector_list.wait_and_dispatch(t);
> +#endif
> +
> +    _timer_list.current_time(last_ev_run);
> +
> +#if 0
>      int timer_priority = XorpTask::PRIORITY_INFINITY;
>      int selector_priority = XorpTask::PRIORITY_INFINITY;
>      int task_priority = XorpTask::PRIORITY_INFINITY;
> @@ -139,6 +157,7 @@
>      }
>  
>      last_ev_run = time(0);
> +#endif
>  }
>  
>  bool
> Index: eventloop.hh
> ===================================================================
> RCS file: /cvs/xorp/libxorp/eventloop.hh,v
> retrieving revision 1.28
> diff -u -r1.28 eventloop.hh
> --- eventloop.hh	23 May 2007 12:12:42 -0000	1.28
> +++ eventloop.hh	21 Oct 2007 01:04:37 -0000
> @@ -343,6 +343,8 @@
>  #else
>      SelectorList	_selector_list;
>  #endif
> +    TimeVal last_ev_run;
> +    time_t last_warned;
>  };
>  
>  // ----------------------------------------------------------------------------
> _______________________________________________
> Xorp-hackers mailing list
> Xorp-hackers at icir.org
> http://mailman.ICSI.Berkeley.EDU/mailman/listinfo/xorp-hackers