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".
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?
> > 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.
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.
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?
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 %= 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);
