[Click] Possible bug in userlevel ToDevice run_task function in Click 2.0
jaeyong yoo
y.jaeyong at gmail.com
Wed Sep 21 22:20:01 EDT 2011
It works fine :)
Thanks Eddie.
Jaeyong
On Thu, Sep 22, 2011 at 5:11 AM, Eddie Kohler <kohler at cs.ucla.edu> 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
>> click at amsterdam.lcs.mit.edu
>> https://amsterdam.lcs.mit.edu/**mailman/listinfo/click<https://amsterdam.lcs.mit.edu/mailman/listinfo/click>
>>
>
More information about the click
mailing list