RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

gnrc_netif: make use of counting event

Open benpicco opened this issue 3 years ago • 1 comments

Contribution description

Can't we have a counting event type?

This adds a counting event type. If multiple events of the same type occur, the callback function is called for each of them instead of being called only once.

This solves the issue where if a network driver has a queue and generates an event for each queued packet, those packets will be stuck in the queue if gnrc_netif is not fast enough to process them in time before the next event occurs.

Testing procedure

Issues/PRs references

alternative to #18204, #18201

benpicco avatar Jul 22 '22 16:07 benpicco

I changed my mind. Considering that we spend quite some effort on having the event handler thread(s) being merged to safe stack space, it is counter intuitive to now double the number of worker threads needed to handle both counting and non-counting events.

Could we instead just enqueue both counting and non-counting events into the same queue and use pointer tagging to mark those which are counting? In case counting events are not used (the module is not included), no pointer tagging is needed and no overhead is introduced. By using pointer tagger this would not blow up the size of event_t.

maribu avatar Aug 05 '22 11:08 maribu

I definitely like the idea of having a "counter" event. However, after the discussed use case in gnrc_netif, I think we cannot guarantee a deterministic behavior for all devices when there are multiple RX Done events without some cooperation from the device drivers.

As it is now, the NETDEV_EVENT_ISR event is used to offload the _isr function to thread context. Therefore, this follows a semantic of "please offload asap" more than "process interrupts in the order they arrive". as the information for the latter case is discarded when calling _isr. Therefore, if a device is able to queue several RX frames before ISR offloading, the device will need to keep state of this anyway.

Therefore, I would be in favour of:

  1. Merging this PR anyway, as it enables some use cool cases
  2. Simply take https://github.com/RIOT-OS/RIOT/pull/18201 for the multiple RX frames before ISR offloading.

Opinions?

jia200x avatar Aug 22 '22 15:08 jia200x

#18201 would certainly be the least invasive change. My concern for creating this PR was that I imagined many other places, where event users might be surprised by the singleton nature of posting events and having a solution ready for those subtle bugs.

For SLIP (and ethos) it would certainly be an argument to move forward with #18066 then as those would then also benefit from the common infrastructure in the chunked ringbuffer for queued frames.

benpicco avatar Aug 22 '22 18:08 benpicco

My concern for creating this PR was that I imagined many other places, where event users might be surprised by the singleton nature of posting events and having a solution ready for those subtle bugs

Yup. As mentioned early, I still think this is a good idea as a module. We should definitely indicate the singleton behavior of normal events though.

jia200x avatar Aug 23 '22 09:08 jia200x

Merging this PR anyway, as it enables some use cool cases

Sorry, I confused this PR. I meant, merging this mechanism (#18204 ), not the GNRC netif mechanism

jia200x avatar Aug 23 '22 09:08 jia200x

Now I'm confused. #18204 makes all events counting and thus changes the API which may have undesired consequences.

Instead this PR introduces a separate counting event type (and tries to share most code with normal event) and makes use of that in gnrc_netif.

benpicco avatar Aug 23 '22 09:08 benpicco

Ah sorry, you are right. What I meant is merging the counting mechanism, but not the usage on GNRC, as it's mainly used for IRQ offloading.

jia200x avatar Aug 23 '22 09:08 jia200x

Now that it's the responsibility of the driver to handle this, we can close this PR.

benpicco avatar Nov 10 '22 14:11 benpicco