> Date: Wed, 20 May 2020 14:39:05 +0200
> From: Martin Pieuchot <m...@openbsd.org>
> Cc: tech@openbsd.org
> Content-Type: text/plain; charset=utf-8
> Content-Disposition: inline
>
> On 20/05/20(Wed) 12:18, Mark Kettenis wrote:
> > > Date: Wed, 20 May 2020 11:33:24 +0200
> > > From: Martin Pieuchot <m...@openbsd.org>
> > >
> > > Diff below implements kqfilter for the 3 remaining drivers in the tree,
> > > that I could find, supporting poll(2) but not kqueue(2).
> > >
> > > magma(4) and spif(4) call seltrue() so their diff is trivial.
> > >
> > > This change is required to be able to switch poll(2) and select(2) to
> > > use the *kqfilter() handlers on sparc64. If one values coherency, it
> > > also makes sense on its own.
> > >
> > > ok?
> >
> > Nope. The filt_vldcwrite() implementation is defenitely wrong as it
> > looks at the rx queue instead of the tx queue.
>
> Diff below fixed the cbus_intr_setenabled() line and `avail' calculation.
> Is it what you were pointing?
Yes. But I think it still isn't right. See below.
Are we allowed to make up what these functions return in kn->kn_data?
Since this device only allows reading and writing complete packets,
counting packets instead of bytes probably makes sense.
> > But even then, I'm not sure if this is doing the right thing. Si the
> > way the code enables interrupts compatible with kqueue(2)? In the
> > current implementation, enabling interrupts is closely tied to the
> > selrecord() call and disabling interrupts happens at the same time as
> > the selwakeup().
>
> When a kevent is registered to a kqueue, the corresponding filter
> function, pointed by `f_event', is called. This is similar to calling
> `fo_poll' inside pollscan() before going to sleep.
I guess it has to because if there is something to read/write it
shouldn't go to sleep ;). Does this mean kn->kn_data should be set to
0 where we enable the interrupt?
> When `f_event' returns a non-0 value, an allocated descriptor representing
> the event, `kn' (or knote), is put in the kqueue.
>
> If this didn't happen when the kevent has been registered, either via
> kevent(2) or by calling kqueue_register() inside sys_ppoll(), it might
> happen when selwakeup() occurs. Because selwakeup() calls knote().
>
> To my understanding this does the same, did I miss anything?
You tell me ;).
> > It really is hard to tell for me to tell what is supposed to happen
> > here since there is no kqueue(9) that documents how this is supposed to
> > work.
>
> Agreed, that's why it is important to discuss and build knowledge :)
>
> Index: arch/sparc64/dev/vldcp.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/dev/vldcp.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 vldcp.c
> --- arch/sparc64/dev/vldcp.c 19 Dec 2019 12:04:38 -0000 1.19
> +++ arch/sparc64/dev/vldcp.c 20 May 2020 12:37:27 -0000
> @@ -70,6 +70,11 @@ struct vldcp_softc {
>
> int vldcp_match(struct device *, void *, void *);
> void vldcp_attach(struct device *, struct device *, void *);
> +void filt_vldcprdetach(struct knote *);
> +void filt_vldcpwdetach(struct knote *);
> +int filt_vldcpread(struct knote *, long);
> +int filt_vldcpwrite(struct knote *, long);
> +int vldcpkqfilter(dev_t, struct knote *);
>
> struct cfattach vldcp_ca = {
> sizeof(struct vldcp_softc), vldcp_match, vldcp_attach
> @@ -614,4 +619,122 @@ vldcppoll(dev_t dev, int events, struct
> }
> splx(s);
> return revents;
> +}
> +
> +void
> +filt_vldcprdetach(struct knote *kn)
> +{
> + struct vldcp_softc *sc = (void *)kn->kn_hook;
> + int s;
> +
> + s = spltty();
> + klist_remove(&sc->sc_rsel.si_note, kn);
> + splx(s);
> +}
> +
> +void
> +filt_vldcpwdetach(struct knote *kn)
> +{
> + struct vldcp_softc *sc = (void *)kn->kn_hook;
> + int s;
> +
> + s = spltty();
> + klist_remove(&sc->sc_wsel.si_note, kn);
> + splx(s);
> +}
> +
> +int
> +filt_vldcpread(struct knote *kn, long hint)
> +{
> + struct vldcp_softc *sc = (void *)kn->kn_hook;
> + struct ldc_conn *lc = &sc->sc_lc;
> + uint64_t head, tail, avail, state;
> + int s, err;
> +
> + s = spltty();
> + err = hv_ldc_rx_get_state(lc->lc_id, &head, &tail, &state);
> + if (err == 0 && state == LDC_CHANNEL_UP && head != tail) {
> + avail = (head - tail) / sizeof(struct ldc_pkt) +
> + lc->lc_rxq->lq_nentries - 1;
This needs to be
avail = (tail - head) / sizeof(struct ldc_pkt) +
lc->lc_rxq->lq_nentries;
I think the -1 here would be wrong. If tail is 1 entry ahead of head,
there is 1 packet of data to read.
> + avail %= lc->lc_rxq->lq_nentries;
> + kn->kn_data = avail;
> + } else {
> + cbus_intr_setenabled(sc->sc_bustag, sc->sc_rx_ino,
> + INTR_ENABLED);
kn->kn_data = 0?
> + }
> + splx(s);
> +
> + return (kn->kn_data > 0);
> +}
> +
> +int
> +filt_vldcwrite(struct knote *kn, long hint)
> +{
> + struct vldcp_softc *sc = (void *)kn->kn_hook;
> + struct ldc_conn *lc = &sc->sc_lc;
> + uint64_t head, tail, avail, state;
> + int s, err;
> +
> + s = spltty();
> + err = hv_ldc_tx_get_state(lc->lc_id, &head, &tail, &state);
> + if (err == 0 && state == LDC_CHANNEL_UP && head != tail) {
> + avail = (head - tail) / sizeof(struct ldc_pkt) +
> + lc->lc_txq->lq_nentries - 1;
This one is right I believe. The -1 is needed here since we don't
allow the head pointer to become equal to the tail so effectively we
can't use the "last" entry in the queue.
Yes, I think that means the current poll implementation is broken for
writes. Currently we only poll for reading (POLLIN).
> + avail %= lc->lc_txq->lq_nentries;
> + kn->kn_data = avail;
> + } else {
> + cbus_intr_setenabled(sc->sc_bustag, sc->sc_tx_ino,
> + INTR_ENABLED);
> + }
> + splx(s);
> +
> + return (kn->kn_data > 0);
> +}
> +
> +const struct filterops vldcpread_filtops = {
> + .f_flags = FILTEROP_ISFD,
> + .f_attach = NULL,
> + .f_detach = filt_vldcprdetach,
> + .f_event = filt_vldcpread,
> +};
> +
> +const struct filterops vldcpwrite_filtops = {
> + .f_flags = FILTEROP_ISFD,
> + .f_attach = NULL,
> + .f_detach = filt_vldcpwdetach,
> + .f_event = filt_vldcwrite,
> +};
> +
> +int
> +vldcpkqfilter(dev_t dev, struct knote *kn)
> +{
> + struct vldcp_softc *sc;
> + struct klist *klist;
> + int s;
> +
> + sc = vldcp_lookup(dev);
> + if (sc == NULL)
> + return (ENXIO);
> +
> + switch (kn->kn_filter) {
> + case EVFILT_READ:
> + klist = &sc->sc_rsel.si_note;
> + kn->kn_fop = &vldcpread_filtops;
> + break;
> + case EVFILT_WRITE:
> + klist = &sc->sc_wsel.si_note;
> + kn->kn_fop = &vldcpwrite_filtops;
> + break;
> +
> + default:
> + return (EINVAL);
> + }
> +
> + kn->kn_hook = sc;
> +
> + s = spltty();
> + klist_insert(klist, kn);
> + splx(s);
> +
> + return (0);
> }
> Index: arch/sparc64/include/conf.h
> ===================================================================
> RCS file: /cvs/src/sys/arch/sparc64/include/conf.h,v
> retrieving revision 1.25
> diff -u -p -r1.25 conf.h
> --- arch/sparc64/include/conf.h 13 May 2020 08:10:03 -0000 1.25
> +++ arch/sparc64/include/conf.h 20 May 2020 09:15:55 -0000
> @@ -64,7 +64,8 @@ cdev_decl(vdsp);
> #define cdev_gen_init(c,n) { \
> dev_init(c,n,open), dev_init(c,n,close), dev_init(c,n,read), \
> dev_init(c,n,write), dev_init(c,n,ioctl), (dev_type_stop((*))) nullop, \
> - 0, dev_init(c,n,poll), (dev_type_mmap((*))) enodev }
> + 0, dev_init(c,n,poll), (dev_type_mmap((*))) enodev, \
> + 0, 0, dev_init(c,n,kqfilter) }
>
> cdev_decl(cn);
>
> Index: dev/sbus/magma.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/sbus/magma.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 magma.c
> --- dev/sbus/magma.c 18 Feb 2020 00:12:08 -0000 1.31
> +++ dev/sbus/magma.c 20 May 2020 09:15:55 -0000
> @@ -1340,6 +1340,7 @@ mtty_param(struct tty *tp, struct termio
> * mbppwrite write to mbpp
> * mbppioctl do ioctl on mbpp
> * mbpppoll do poll on mbpp
> + * mbppkqfilter kqueue on mbpp
> * mbpp_rw general rw routine
> * mbpp_timeout rw timeout
> * mbpp_start rw start after delay
> @@ -1513,6 +1514,12 @@ int
> mbpppoll(dev_t dev, int events, struct proc *p)
> {
> return (seltrue(dev, events, p));
> +}
> +
> +int
> +mbppkqfilter(dev_t dev, struct knote *kn)
> +{
> + return (seltrue_kqfilter(dev, kn));
> }
>
> int
> Index: dev/sbus/spif.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/sbus/spif.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 spif.c
> --- dev/sbus/spif.c 19 Jul 2019 00:17:15 -0000 1.23
> +++ dev/sbus/spif.c 20 May 2020 09:15:55 -0000
> @@ -91,6 +91,7 @@ int sbppwrite(dev_t, struct uio *, int);
> int sbpp_rw(dev_t, struct uio *);
> int spifppcintr(void *);
> int sbpppoll(dev_t, int, struct proc *);
> +int sbppkqfilter(dev_t, struct knote *);
> int sbppioctl(dev_t, u_long, caddr_t, int, struct proc *);
>
> struct cfattach spif_ca = {
> @@ -1043,6 +1044,11 @@ int
> sbpppoll(dev_t dev, int events, struct proc *p)
> {
> return (seltrue(dev, events, p));
> +}
> +int
> +sbppkqfilter(dev_t dev, struct knote *kn)
> +{
> + return (seltrue_kqfilter(dev, kn));
> }
>
> int
>
