Re: [OpenBSD -current] Change event timer in main loop with kqueue

2021-02-27 Thread Visa Hankala
Moving to tech@.

On Fri, Feb 26, 2021 at 09:42:07PM +0100, martin mag wrote:
> I've been trying to use kqueue for the last couple of day but I keep
> having an issue with EVFILT_TIMER filter. (I'm running Openbsd
> -current)
> 
> Right now, I'm trying to do the following:
> 1) Initilialize a timer event @ 200ms, periodically.
> 2) Inside the main event loop => If this event is retrieved, print
> elapsed time since last one
> 3) After 2 iterations, MODIFY the timer event to 1000ms and continue the loop
> 4) Code stops after 4 iterations as pb arise after the first timer
> change @ iteration 2.
> 
> Reading the manpages kqueue(2), one sees that:
> ** ) An event is uniquely defined by the pair (ident, filter) ==>
> in the example below (TIMER1, EVFILT_TIMER)
> **)  "" Re-adding an existing event will modify the parameters of
> the original event, and not result in a duplicate entry. "" => So
> re-adding the event (TIMER1, EVFILT_TIMER) with a modified field
> 'data' should update the timer from 200ms to 1000ms.
> 
> => Apparently, timer is updated, but not in the way I expected.

The kernel does not reschedule the timer when the user changes the
timeout period. The new period will take effect only after the current
period has expired. This is not explained in the manual page, though.

With the recent kqueue changes, it is straightforward to make the kernel
modify an existing timer. I think the clearest behaviour is to reset the
timer completely when it is modified. If there are pending events, they
should be cancelled because they do not necessarily correspond to the
new settings.

When f_modify and f_process are present in kqread_filtops, filt_timer
is not used. filt_timerexpire() activates timer knotes directly using
knote_activate() instead of KNOTE().

However, the current behaviour has been around so long that one can
argue that it is an actual feature. BSDs are not consistent with this,
though. FreeBSD resets the timer immediately, whereas NetBSD and
DragonFly BSD apply the new period after expiry.

I guess the resetting is harmless in most cases but might wreak havoc
at least with software that keeps poking its timers before expiry.

Index: lib/libc/sys/kqueue.2
===
RCS file: src/lib/libc/sys/kqueue.2,v
retrieving revision 1.43
diff -u -p -r1.43 kqueue.2
--- lib/libc/sys/kqueue.2   14 Nov 2020 10:16:15 -  1.43
+++ lib/libc/sys/kqueue.2   27 Feb 2021 12:54:27 -
@@ -468,6 +468,11 @@ contains the number of times the timeout
 This filter automatically sets the
 .Dv EV_CLEAR
 flag internally.
+.Pp
+If an existing timer is re-added, the existing timer and related pending events
+will be cancelled.
+The timer will be re-started using the timeout period
+.Fa data .
 .It Dv EVFILT_DEVICE
 Takes a descriptor as the identifier and the events to watch for in
 .Fa fflags ,
Index: sys/kern/kern_event.c
===
RCS file: src/sys/kern/kern_event.c,v
retrieving revision 1.161
diff -u -p -r1.161 kern_event.c
--- sys/kern/kern_event.c   24 Feb 2021 14:59:52 -  1.161
+++ sys/kern/kern_event.c   27 Feb 2021 12:54:27 -
@@ -135,7 +135,8 @@ int filt_fileattach(struct knote *kn);
 void   filt_timerexpire(void *knx);
 intfilt_timerattach(struct knote *kn);
 void   filt_timerdetach(struct knote *kn);
-intfilt_timer(struct knote *kn, long hint);
+intfilt_timermodify(struct kevent *kev, struct knote *kn);
+intfilt_timerprocess(struct knote *kn, struct kevent *kev);
 void   filt_seltruedetach(struct knote *kn);
 
 const struct filterops kqread_filtops = {
@@ -163,7 +164,9 @@ const struct filterops timer_filtops = {
.f_flags= 0,
.f_attach   = filt_timerattach,
.f_detach   = filt_timerdetach,
-   .f_event= filt_timer,
+   .f_event= NULL,
+   .f_modify   = filt_timermodify,
+   .f_process  = filt_timerprocess,
 };
 
 struct pool knote_pool;
@@ -444,15 +447,48 @@ filt_timerdetach(struct knote *kn)
struct timeout *to;
 
to = (struct timeout *)kn->kn_hook;
-   timeout_del(to);
+   timeout_del_barrier(to);
free(to, M_KEVENT, sizeof(*to));
kq_ntimeouts--;
 }
 
 int
-filt_timer(struct knote *kn, long hint)
+filt_timermodify(struct kevent *kev, struct knote *kn)
+{
+   struct timeout *to = kn->kn_hook;
+   int s;
+
+   /* Reset the timer. Any pending events are discarded. */
+
+   timeout_del_barrier(to);
+
+   s = splhigh();
+   if (kn->kn_status & KN_QUEUED)
+   knote_dequeue(kn);
+   kn->kn_status &= ~KN_ACTIVE;
+   splx(s);
+
+   kn->kn_data = 0;
+   knote_modify(kev, kn);
+   /* Reinit timeout to invoke tick adjustment again. */
+   timeout_set(to, filt_timerexpire, kn);
+   filt_timer_timeout_add(kn);
+
+   return (0);
+}
+
+int
+filt_timerprocess(struct knote *kn, 

[OpenBSD -current] Change event timer in main loop with kqueue

2021-02-26 Thread martin mag
Hello everyone!

I've been trying to use kqueue for the last couple of day but I keep
having an issue with EVFILT_TIMER filter. (I'm running Openbsd
-current)

Right now, I'm trying to do the following:
1) Initilialize a timer event @ 200ms, periodically.
2) Inside the main event loop => If this event is retrieved, print
elapsed time since last one
3) After 2 iterations, MODIFY the timer event to 1000ms and continue the loop
4) Code stops after 4 iterations as pb arise after the first timer
change @ iteration 2.

Reading the manpages kqueue(2), one sees that:
** ) An event is uniquely defined by the pair (ident, filter) ==>
in the example below (TIMER1, EVFILT_TIMER)
**)  "" Re-adding an existing event will modify the parameters of
the original event, and not result in a duplicate entry. "" => So
re-adding the event (TIMER1, EVFILT_TIMER) with a modified field
'data' should update the timer from 200ms to 1000ms.

=> Apparently, timer is updated, but not in the way I expected. See
below an example.

Here is the C program. I removed every 'error-checker' intentionally
as this is just a basic test:

#include 
#include 
#include 
#include 
#define TIMER1 202

int main(){
int kq=0, nev=0;
struct kevent evlist, chlist;
struct timespec start, stop, elapsed;

/* Initialize the queue */
kq = kqueue();
/* Register event to the queue */
EV_SET(&chlist, TIMER1, EVFILT_TIMER, EV_ADD | EV_ENABLE, 0, 200, 0);
kevent(kq, &chlist, 1, NULL, 0, NULL);

for (int i=0; i<4; i++){
clock_gettitme(CLOCK_MONOTONIC, &start);
nev = kevent(kq, NULL, 0, &evlist, 1, NULL);
printf("Iteration %d => nb events=%d\n", i+1, nev);
if (evlist.ident == TIMER1){
clock_gettime(CLOCK_MONOTONIC, &stop);
timespecsub(&stop, &start, &elapsed);
printf("Time elapsed since previous iteration: %lld.%09lds\n",
   (long long) elapsed.tv_sec, (long long)
elapsed.tv_nsec);

/* > MODIFY TIMER <== */
if( (i+1)%2 == 0){
printf("Adjusting timer event ...\n");
EV_SET(&chlist, TIMER1, EVFILT_TIMER, EV_ADD |
EV_ENABLE, 0, 1000, 0);
/* I also tried this:   chlist.data = 1000;  but same
problem arise*/
/* Register modification within the queue */
kevent(kq, &chlist, 1, NULL, 0, NULL);
printf("Next event should happen %dms later", chlist.data);
  } /* End i%4 == 0

 } /* End evlist.ident == TIMER1 */

} /* End for loop */

return EXIT_SUCCESS;
}

*** Compiled with gcc-8.4.0
# egcc -o test_kqueue test_kqueue.c

*** OUTPUT of above program
Iteration 1 => nb events=1
Time elapsed since previous event:0.203417468s
==

Iteration 2 => nb events=1
Time elapsed since previous event:0.199534100s
Adjusting timer event 
Next event in 1000ms<< ===
This is where TIMER is changed

<< ===  and  kqueue is updated
==

Iteration 3 => nb events=1
Time elapsed since previous event:0.199848328<< === Problem here:

   << It should be ~1s not 0.2s (initial timer)
==

Iteration 4 => nb events=1
Time elapsed since previous event:0.999884957s   << === Now it's OK
Adjusting timer event 
Next event in 1000ms
==
*** END OF OUTPUT

So what I expected from my program was that Iteration 3 would be
retrieved 1second after iteration 2. But here, it is retrieved 0.2s
after only. This is AS IF the change wasn't taken into account yet
...? The expected behaviour is seen at iteration 4.

I'm pretty sure I'm not understanding correctly what happens but I
cannot figure out where I'm wrong in my example.

I did another test modifying the event timer (line 31 in program) in loop with:
TEST 1: (Added EV_ONESHOT)
  EV_SET(&chlist, TIMER1, EVFILT_TIMER, EV_ADD | EV_ENABLE |
EV_ONESHOT, 0, 1000, 0);   ===>> ONESHOT does not seem to be taken
into account as the event keeps beeing retrieved 1s apart. (the
expected behavior would that that only one event should be triggered
after this modification)

TEST 2 (Disabling event to see if it happens instantly or if it is
"delayed" as in the previous examples)
The ONLY change that work as expected is EV_DISABLE, which stops
events from being retrieved after iteration 2.

Could any one help me figure out what I'm doing wrong and how I can
manage modifying an existing timer event?

Thanks a lot!

PS: This is not a copy/paste program as I'm not sending the message
from the same PC. I hope I didn't do any typos rewritting
everything...