> Date: Sat, 23 May 2020 12:42:04 +0200
> From: Martin Pieuchot <[email protected]>
>
> On 21/05/20(Thu) 14:44, Mark Kettenis wrote:
> > > Date: Wed, 20 May 2020 14:39:05 +0200
> > > From: Martin Pieuchot <[email protected]>
> > > Cc: [email protected]
> > > [...]
> > > 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.
>
> What is written in `kn_data' will be available to userland in the `data'
> field of the corresponding "struct kevent" exchanged via kevent(2). It
> is described as a "Filter-specific data value".
Which for EVFILT_READ and EVFILT_WRITE is dependent on the actual
device so it can be packets instead of bytes.
>
> To implement poll(2)/select(2) on top of kqfilter* routines the value
> doesn't matter, these interfaces only need to know if something can be
> read or written, not how much.
>
> So if you believe that counting packets make more sense, could you tell
> which calculation I should use or maybe it's easier for you to modify the
> routines once in tree?
You're already counting packets.
> > > 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?
>
> The first time a filter is called with a newly allocated descriptor,
> `kn_data' is already cleared. The existing kqfilters are inconsistent
> when it comes to set kn_data to 0 and which value to test in the return
> statement.
ok
> These 2 behaviors matter for events that stay on the queue after having
> being triggered.
>
> I haven't investigated this part of the handlers yet. This question is
> also related to the EV_CLEAR flag that one can set in events in userland.
>
> For the proposed poll(2)/select(2) diff it doesn't matter because events
> are always allocated & freed for every syscall. However this question
> should be answered if we want to implement lazy removal & reuse of
> descriptors.
In general I think we shouldn't add any new filters in this process
that are "wrong". As this will come back to bite us later. That
said, this device is special and not expected to be used by random
ports so fixing things later is much easier.
> Diff below removes the "-1" as suggested. Is it ok to commit it and
> address your other questions, which apply to all kq filters, later?
There is one fix you missed, see below.
With that change, ok kettenis@
> 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 23 May 2020 10:38:08 -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;
avail = (tail - head) / sizeof(struct ldc_pkt) +
lc->lc_rxq->lq_nentries;
> + avail %= lc->lc_rxq->lq_nentries;
> + kn->kn_data = avail;
> + } else {
> + cbus_intr_setenabled(sc->sc_bustag, sc->sc_rx_ino,
> + INTR_ENABLED);
> + }
> + 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;
> + 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);
>
>
