Re: Replace ttkqflush() with klist_invalidate()
On Tue, Feb 04, 2020 at 01:04:24PM +0100, Martin Pieuchot wrote:
> On 02/02/20(Sun) 09:58, Visa Hankala wrote:
> > tty(4) uses custom code for revoking knotes. It should be changed to use
> > klist_invalidate() to handle the revocation in one place. The following
> > diff does that.
>
> Great, your diff is ok mpi@
>
> > In addition, the patch makes the code store the tty context pointer
> > in kn_hook directly. This simplifies the code.
>
> Does that mean we'll need some reference counting when getting kqueue
> out of the KERNEL_LOCK()? Who is the owner of `kn_hook'?
I think the proper way is to invalidate knotes before the associated
event source object or kqueue is destroyed. That way there is no need
for reference counting. klist_invalidate() does the job when the object
is destroyed kind of involuntarily, like when a device is detached.
Typically, knotes are freed by knote_fdclose() when the file descriptor
is closed, which happens before the object is freed. kqueue_close()
takes care of freeing kqueue's registered knotes when the kqueue itself
is being freed.
I would say the main owner of kn_hook and the knote itself is the object
that owns the kn_selnext list. klist_invalidate() dissolves that
ownership (well, any knotes on the list get freed either when the EOF
event is delivered to userspace or when the kqueue is destroyed).
> > Unlike ttkqflush(), klist_invalidate() can sleep. However, that should
> > not be a problem because the callers of ttyfree() call vdevgone() that
> > can sleep as well.
>
> It took me some time to audit com_detach() because ttyfree() isn't the
> last call. Most of the *_detach() functions have similar problems: they
> assume the execution of the code is atomic and free data structures
> before unregistering handlers.
>
> Diff below improves that, ok?
Right, at least the soft interrupt handler dereferences sc_tty. The
code assumes that softintr_disestablish() has barrier-like behaviour,
which should be the case in this context on most architectures because
of the kernel lock.
OK visa@
> Index: dev/ic/com.c
> ===
> RCS file: /cvs/src/sys/dev/ic/com.c,v
> retrieving revision 1.170
> diff -u -p -r1.170 com.c
> --- dev/ic/com.c 19 Jul 2019 00:17:15 - 1.170
> +++ dev/ic/com.c 4 Feb 2020 11:27:06 -
> @@ -186,14 +186,14 @@ com_detach(struct device *self, int flag
> mn |= 0x80;
> vdevgone(maj, mn, mn, VCHR);
>
> + timeout_del(>sc_dtr_tmo);
> + timeout_del(>sc_diag_tmo);
> + softintr_disestablish(sc->sc_si);
> +
> /* Detach and free the tty. */
> if (sc->sc_tty) {
> ttyfree(sc->sc_tty);
> }
> -
> - timeout_del(>sc_dtr_tmo);
> - timeout_del(>sc_diag_tmo);
> - softintr_disestablish(sc->sc_si);
>
> return (0);
> }
>
Re: Replace ttkqflush() with klist_invalidate()
On 02/02/20(Sun) 09:58, Visa Hankala wrote:
> tty(4) uses custom code for revoking knotes. It should be changed to use
> klist_invalidate() to handle the revocation in one place. The following
> diff does that.
Great, your diff is ok mpi@
> In addition, the patch makes the code store the tty context pointer
> in kn_hook directly. This simplifies the code.
Does that mean we'll need some reference counting when getting kqueue
out of the KERNEL_LOCK()? Who is the owner of `kn_hook'?
> Unlike ttkqflush(), klist_invalidate() can sleep. However, that should
> not be a problem because the callers of ttyfree() call vdevgone() that
> can sleep as well.
It took me some time to audit com_detach() because ttyfree() isn't the
last call. Most of the *_detach() functions have similar problems: they
assume the execution of the code is atomic and free data structures
before unregistering handlers.
Diff below improves that, ok?
Index: dev/ic/com.c
===
RCS file: /cvs/src/sys/dev/ic/com.c,v
retrieving revision 1.170
diff -u -p -r1.170 com.c
--- dev/ic/com.c19 Jul 2019 00:17:15 - 1.170
+++ dev/ic/com.c4 Feb 2020 11:27:06 -
@@ -186,14 +186,14 @@ com_detach(struct device *self, int flag
mn |= 0x80;
vdevgone(maj, mn, mn, VCHR);
+ timeout_del(>sc_dtr_tmo);
+ timeout_del(>sc_diag_tmo);
+ softintr_disestablish(sc->sc_si);
+
/* Detach and free the tty. */
if (sc->sc_tty) {
ttyfree(sc->sc_tty);
}
-
- timeout_del(>sc_dtr_tmo);
- timeout_del(>sc_diag_tmo);
- softintr_disestablish(sc->sc_si);
return (0);
}
Replace ttkqflush() with klist_invalidate()
tty(4) uses custom code for revoking knotes. It should be changed to use
klist_invalidate() to handle the revocation in one place. The following
diff does that.
In addition, the patch makes the code store the tty context pointer
in kn_hook directly. This simplifies the code.
Unlike ttkqflush(), klist_invalidate() can sleep. However, that should
not be a problem because the callers of ttyfree() call vdevgone() that
can sleep as well.
It would be good to have this tested in various setups. Originally,
ttkqflush() was added to prevent crashing when a ucom(4) device is
detached while still in use by a kqueue-enabled program such as cu(1).
OK?
Index: kern/tty.c
===
RCS file: src/sys/kern/tty.c,v
retrieving revision 1.151
diff -u -p -r1.151 tty.c
--- kern/tty.c 9 Jan 2020 14:35:20 - 1.151
+++ kern/tty.c 2 Feb 2020 09:38:17 -
@@ -75,7 +75,6 @@ static void ttyblock(struct tty *);
void ttyunblock(struct tty *);
static void ttyecho(int, struct tty *);
static void ttyrubo(struct tty *, int);
-void ttkqflush(struct klist *klist);
intfilt_ttyread(struct knote *kn, long hint);
void filt_ttyrdetach(struct knote *kn);
intfilt_ttywrite(struct knote *kn, long hint);
@@ -1124,7 +1123,7 @@ ttkqfilter(dev_t dev, struct knote *kn)
return (EINVAL);
}
- kn->kn_hook = (caddr_t)((u_long)dev);
+ kn->kn_hook = tp;
s = spltty();
SLIST_INSERT_HEAD(klist, kn, kn_selnext);
@@ -1134,29 +1133,11 @@ ttkqfilter(dev_t dev, struct knote *kn)
}
void
-ttkqflush(struct klist *klist)
-{
- struct knote *kn, *kn1;
-
- SLIST_FOREACH_SAFE(kn, klist, kn_selnext, kn1) {
- SLIST_REMOVE(klist, kn, knote, kn_selnext);
- kn->kn_hook = (caddr_t)((u_long)NODEV);
- kn->kn_flags |= EV_EOF;
- knote_activate(kn);
- }
-}
-
-void
filt_ttyrdetach(struct knote *kn)
{
- dev_t dev = (dev_t)((u_long)kn->kn_hook);
- struct tty *tp;
+ struct tty *tp = kn->kn_hook;
int s;
- if (dev == NODEV)
- return;
- tp = (*cdevsw[major(dev)].d_tty)(dev);
-
s = spltty();
SLIST_REMOVE(>t_rsel.si_note, kn, knote, kn_selnext);
splx(s);
@@ -1165,16 +1146,9 @@ filt_ttyrdetach(struct knote *kn)
int
filt_ttyread(struct knote *kn, long hint)
{
- dev_t dev = (dev_t)((u_long)kn->kn_hook);
- struct tty *tp;
+ struct tty *tp = kn->kn_hook;
int s;
- if (dev == NODEV) {
- kn->kn_flags |= EV_EOF;
- return (1);
- }
- tp = (*cdevsw[major(dev)].d_tty)(dev);
-
s = spltty();
kn->kn_data = ttnread(tp);
splx(s);
@@ -1188,14 +1162,9 @@ filt_ttyread(struct knote *kn, long hint
void
filt_ttywdetach(struct knote *kn)
{
- dev_t dev = (dev_t)((u_long)kn->kn_hook);
- struct tty *tp;
+ struct tty *tp = kn->kn_hook;
int s;
- if (dev == NODEV)
- return;
- tp = (*cdevsw[major(dev)].d_tty)(dev);
-
s = spltty();
SLIST_REMOVE(>t_wsel.si_note, kn, knote, kn_selnext);
splx(s);
@@ -1204,16 +1173,9 @@ filt_ttywdetach(struct knote *kn)
int
filt_ttywrite(struct knote *kn, long hint)
{
- dev_t dev = (dev_t)((u_long)kn->kn_hook);
- struct tty *tp;
+ struct tty *tp = kn->kn_hook;
int canwrite, s;
- if (dev == NODEV) {
- kn->kn_flags |= EV_EOF;
- return (1);
- }
- tp = (*cdevsw[major(dev)].d_tty)(dev);
-
s = spltty();
kn->kn_data = tp->t_outq.c_cn - tp->t_outq.c_cc;
canwrite = (tp->t_outq.c_cc <= tp->t_lowat);
@@ -2378,6 +2340,7 @@ ttymalloc(int baud)
void
ttyfree(struct tty *tp)
{
+ int s;
--tty_count;
#ifdef DIAGNOSTIC
@@ -2386,8 +2349,10 @@ ttyfree(struct tty *tp)
#endif
TAILQ_REMOVE(, tp, tty_link);
- ttkqflush(>t_rsel.si_note);
- ttkqflush(>t_wsel.si_note);
+ s = spltty();
+ klist_invalidate(>t_rsel.si_note);
+ klist_invalidate(>t_wsel.si_note);
+ splx(s);
clfree(>t_rawq);
clfree(>t_canq);
