Re: Possible bug in userlevel ToDevice run_task function in Click 2.0

It works fine :)
Thanks Eddie.


Jaeyong

On Thu, Sep 22, 2011 at 5:11 AM, Eddie Kohler <[email protected]> wrote:

> 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/**0d19b2033c06dbc326536eb3005337*
> *caf9c0261c<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
>> [email protected]
>> https://amsterdam.lcs.mit.edu/**mailman/listinfo/click<https://amsterdam.lcs.mit.edu/mailman/listinfo/click>
>>
>