Download raw body.
kqueue: EVFILT_USER fixes
On Tue, May 20, 2025 at 06:28:58PM +0200, Volker Schlecht wrote:
> As already stated to visa@ off-list: This solves the problem I saw with
> an experimental update to the latest version of devel/libinotify and while I
> don't think I should be ok'ing kernel patches, I can confirm that it solves
> a current problem, and hope that the fix gets in :-)
>
Ah, understood. I have the only negative report with 'filewatch' test
from lua-language-server, but it seems to be not related to this
implementation.
ok mvs to the latest diff
> On 5/19/25 4:55 PM, Visa Hankala wrote:
> > On Sun, May 18, 2025 at 09:27:12PM +0300, Vitaliy Makkoveev wrote:
> > > On Sun, May 18, 2025 at 05:21:14PM +0000, Visa Hankala wrote:
> > > > The EVFILT_USER implementation does not quite align with FreeBSD.
> > > > One difference is that FreeBSD updates the `data' and `udata' fields
> > > > when a user event is triggered. FreeBSD also preserves `fflags' when
> > > > the event is "untriggered". The diff below fixes these.
> > > >
> > > > In addition, use kn_sfflags and kn_sdata instead of kn_fflags and
> > > > kn_data to make the implementation a bit more similar to FreeBSD.
> > > >
> > > > OK?
> > > >
> > >
> > > FreeBSD and NetBSD clean `kn_data' and `kn_fflag' if EV_CLEAR flag is
> > > set. Shouldn't you do the same?
> > >
> > > case EVENT_REGISTER:
> > > kn->kn_sdata = kev->data;
> > > if (kev->flags & EV_CLEAR) {
> > > kn->kn_hookid = 0;
> > > kn->kn_data = 0;
> > > kn->kn_fflags = 0;
> > > }
> > > break;
> > >
> > > case EVENT_PROCESS:
> > > *kev = kn->kn_kevent;
> > > kev->fflags = kn->kn_sfflags;
> > > kev->data = kn->kn_sdata;
> > > if (kn->kn_flags & EV_CLEAR) {
> > > kn->kn_hookid = 0;
> > > kn->kn_data = 0;
> > > kn->kn_fflags = 0;
> > > }
> >
> > I think clearing kn_data and kn_fflags has no effect visible
> > to userland. They are aliases to kn->kn_kevent.data and
> > kn->kn_kevent.fflags. In the EVENT_PROCESS case above, where the user
> > event is prepared for copyout to userland, the values of these fields
> > in *kev are overridden using kn->kn_sfflags and kn->kn_sdata.
> >
> > As for EV_CLEAR, the knote should preserve the userland-visible value
> > of `fflags'. Zeroing `data' is not necessary because it gets set again
> > anyway when the user event is re-activated, and userland cannot read
> > `data' before the event is active.
> >
> > After mulling over the details some more, I think it is clearer to keep
> > using kn_data and kn_flags in filt_user{attach,modify,process}()
> > after all. This possibly avoids the confusion with kn_{fflags,data}
> > versus kn_s{fflags,data}. The diff becomes shorter too.
> >
> > Index: sys/kern/kern_event.c
> > ===================================================================
> > RCS file: src/sys/kern/kern_event.c,v
> > retrieving revision 1.204
> > diff -u -p -r1.204 kern_event.c
> > --- sys/kern/kern_event.c 15 May 2025 19:56:08 -0000 1.204
> > +++ sys/kern/kern_event.c 19 May 2025 14:23:10 -0000
> > @@ -850,16 +850,12 @@ filt_usermodify(struct kevent *kev, stru
> > break;
> > }
> > - if (kev->flags & EV_ADD) {
> > - kn->kn_data = kev->data;
> > - kn->kn_udata = kev->udata;
> > - }
> > + kn->kn_data = kev->data;
> > + kn->kn_udata = kev->udata;
> > /* Allow clearing of an activated event. */
> > - if (kev->flags & EV_CLEAR) {
> > + if (kev->flags & EV_CLEAR)
> > kn->kn_ptr.p_useract = 0;
> > - kn->kn_data = 0;
> > - }
> > return (kn->kn_ptr.p_useract);
> > }
> > @@ -872,11 +868,8 @@ filt_userprocess(struct knote *kn, struc
> > active = kn->kn_ptr.p_useract;
> > if (active && kev != NULL) {
> > *kev = kn->kn_kevent;
> > - if (kn->kn_flags & EV_CLEAR) {
> > + if (kn->kn_flags & EV_CLEAR)
> > kn->kn_ptr.p_useract = 0;
> > - kn->kn_fflags = 0;
> > - kn->kn_data = 0;
> > - }
> > }
> > return (active);
> > Index: regress/sys/kern/kqueue/kqueue-user.c
> > ===================================================================
> > RCS file: src/regress/sys/kern/kqueue/kqueue-user.c,v
> > retrieving revision 1.1
> > diff -u -p -r1.1 kqueue-user.c
> > --- regress/sys/kern/kqueue/kqueue-user.c 10 May 2025 09:44:39 -0000 1.1
> > +++ regress/sys/kern/kqueue/kqueue-user.c 19 May 2025 14:23:10 -0000
> > @@ -44,10 +44,7 @@ do_user(void)
> > n = kevent(kq, NULL, 0, kev, 2, &ts);
> > ASSX(n == 0);
> > - /*
> > - * Activate the event.
> > - * Fields `data' and `udata' do not get updated without EV_ADD.
> > - */
> > + /* Activate the event. This updates `data' and `udata'. */
> > EV_SET(&kev[0], 1, EVFILT_USER, 0, NOTE_TRIGGER | NOTE_FFNOP,
> > 123, &dummy);
> > n = kevent(kq, kev, 1, NULL, 0, NULL);
> > @@ -58,20 +55,6 @@ do_user(void)
> > ASSX(n == 1);
> > ASSX(kev[0].ident == 1);
> > ASSX(kev[0].fflags == NOTE_FFLAGSMASK);
> > - ASSX(kev[0].data == 0);
> > - ASSX(kev[0].udata == NULL);
> > -
> > - /* Activate the event. Update `data' and `udata'. */
> > - EV_SET(&kev[0], 1, EVFILT_USER, EV_ADD, NOTE_TRIGGER | NOTE_FFNOP,
> > - 123, &dummy);
> > - n = kevent(kq, kev, 1, NULL, 0, NULL);
> > - ASSX(n == 0);
> > -
> > - /* Check active events. */
> > - n = kevent(kq, NULL, 0, kev, 2, &ts);
> > - ASSX(n == 1);
> > - ASSX(kev[0].ident == 1);
> > - ASSX(kev[0].fflags == NOTE_FFLAGSMASK);
> > ASSX(kev[0].data == 123);
> > ASSX(kev[0].udata == &dummy);
> > @@ -122,8 +105,8 @@ do_user(void)
> > ASSX(n == 1);
> > ASSX(kev[0].ident == 2);
> > ASSX(kev[0].fflags == 0x11);
> > - ASSX(kev[0].data == 42);
> > - ASSX(kev[0].udata == &dummy);
> > + ASSX(kev[0].data == 24);
> > + ASSX(kev[0].udata == &dummy2);
> > n = kevent(kq, NULL, 0, kev, 2, &ts);
> > ASSX(n == 0);
> > @@ -132,9 +115,9 @@ do_user(void)
> > n = kevent(kq, kev, 1, kev, 2, &ts);
> > ASSX(n == 1);
> > ASSX(kev[0].ident == 2);
> > - ASSX(kev[0].fflags == 0);
> > - ASSX(kev[0].data == 0);
> > - ASSX(kev[0].udata == &dummy);
> > + ASSX(kev[0].fflags == 0x11);
> > + ASSX(kev[0].data == 9);
> > + ASSX(kev[0].udata == &dummy2);
> > EV_SET(&kev[0], 2, EVFILT_USER, EV_DELETE, 0, 0, NULL);
> > n = kevent(kq, kev, 1, kev, 2, &ts);
> > @@ -154,7 +137,7 @@ do_user(void)
> > ASSX(kev[0].ident == 1);
> > ASSX(kev[0].fflags == 0x0faaf0);
> > ASSX(kev[0].data == 0);
> > - ASSX(kev[0].udata == &dummy);
> > + ASSX(kev[0].udata == NULL);
> > /* Test event limit. */
> > for (i = 0;; i++) {
> >
>
kqueue: EVFILT_USER fixes