[Click] Possible bug in userlevel ToDevice run_task function in Click 2.0

[Eddie Kohler]

Hi Jaeyong,

Very nice find!!  Thank you so much!

I applied a different fix, however -- namely, setting the PCAP file 
descriptor so that the add_select() call succeeds.  See here:

https://github.com/kohler/click/commit/0d19b2033c06dbc326536eb3005337caf9c0261c

Does this version work for you?

Best,
Eddie


On 9/15/11 2:06 AM, jaeyong yoo wrote:
> Hello Eddie,
>
> While I was using Click ToDevice userlevel element, ToDevice suddenly stops
> pulling packets from a notifierqueue even though the ActiveNotifier signals
> "wake".
> After some debugging, I think the problem is the following.
>
> First of all, I'm using PCAP as capture method.
>
> In ToDevice::run_task(Task *) function, when "send_packet(p)" function
> returns -ENOBUFS or -EAGAIN, it backs off.
> But, when the backoff is just starting (means when _backoff is 0), it uses
> add_select(_fd, SELECT_WRITE) function to register a callback and no other
> callbacks (fast_reschedule, etc) are registered (it looks reasonable).
> But the problem arises when we use PCAP, because _fd is -1 and the
> add_select does not properly register callback and eventually run_task never
> called again.
> Even ActiveNotifier can not wake this run_task up.
>
> I think the problematic code is the following.
>
>      if (r == -ENOBUFS || r == -EAGAIN) {
>          assert(!_q);
>          _q = p;
>
>          if (!_backoff) {
>              _backoff = 1;
>              add_select(_fd, SELECT_WRITE);
>          } else {
>              _timer.schedule_after(Timestamp::make_usec(_backoff));
>              if (_backoff<  32768)
>                  _backoff *= 2;
>              if (_debug) {
>                  Timestamp now = Timestamp::now();
>                  click_chatter("%{element} backing off for %d at
> %{timestamp}\n", this, _backoff,&now);
>              }
>          }
>          return count>  0;
>
> and should be changed something like the following.
>
>      if (r == -ENOBUFS || r == -EAGAIN) {
>          assert(!_q);
>          _q = p;
>
>          if (!_backoff) {
>              _backoff = 1;
> #if TODEVICE_ALLOW_PCAP
>              _timer.schedule_after(Timestamp::make_usec(_backoff));
> #endif
>
> #if TODEVICE_ALLOW_LINUX
>              add_select(_fd, SELECT_WRITE);
> #endif
>          } else {
>              _timer.schedule_after(Timestamp::make_usec(_backoff));
>              if (_backoff<  32768)
>                  _backoff *= 2;
>              if (_debug) {
>                  Timestamp now = Timestamp::now();
>                  click_chatter("%{element} backing off for %d at
> %{timestamp}\n", this, _backoff,&now);
>              }
>          }
>          return count>  0;
>
>
> Best regards,
> Jaeyong
> _______________________________________________
> click mailing list
> click at amsterdam.lcs.mit.edu
> https://amsterdam.lcs.mit.edu/mailman/listinfo/click