Re: Lock operations for knote lists
On Tue, Dec 15, 2020 at 09:58:16AM -0300, Martin Pieuchot wrote:
> On 11/12/20(Fri) 17:37, Visa Hankala wrote:
> > Index: kern/kern_event.c
> > ===
> > RCS file: src/sys/kern/kern_event.c,v
> > retrieving revision 1.147
> > diff -u -p -r1.147 kern_event.c
> > --- kern/kern_event.c 9 Dec 2020 18:58:19 - 1.147
> > +++ kern/kern_event.c 11 Dec 2020 17:05:09 -
> > @@ -1539,9 +1576,14 @@ klist_invalidate(struct klist *list)
> > NET_ASSERT_UNLOCKED();
> >
> > s = splhigh();
> > + ls = klist_lock(list);
>
> Isn't splhigh() redundant with klist_lock() now? If a subsystem
> provides its own lock/unlock routine shouldn't it ensure that the
> necessary SPL is used? Or is this protecting something else? Or is
> it just paranoia and we should try to remove it in a later step?
splhigh() is needed by knote_acquire(). The code will change when
a mutex is added to it.
> > while ((kn = SLIST_FIRST(>kl_list)) != NULL) {
> > - if (!knote_acquire(kn))
> > + if (!knote_acquire(kn, list, ls)) {
> > + /* knote_acquire() has unlocked list. */
> > + ls = klist_lock(list);
> > continue;
> > + }
> > + klist_unlock(list, ls);
> > splx(s);
> > kn->kn_fop->f_detach(kn);
> > if (kn->kn_fop->f_flags & FILTEROP_ISFD) {
>
Re: Lock operations for knote lists
On 11/12/20(Fri) 17:37, Visa Hankala wrote:
> This patch extends struct klist with a callback descriptor and
> an argument. The main purpose of this is to let the kqueue subsystem
> assert when a klist should be locked, and operate the klist lock
> in klist_invalidate().
Lovely!
> Access to a knote list of a kqueue-monitored object has to be
> serialized somehow. Because the object often has a lock for protecting
> its state, and because the object often acquires this lock at the latest
> in its f_event callback functions, I would like to use the same lock
> also for the knote lists. Uses of NOTE_SUBMIT already show a pattern
> arising.
>
> There could be an embedded lock in klist. However, such a lock would be
> redundant in many cases. The code could not rely on a single lock type
> (mutex, rwlock, something else) because the needs of monitored objects
> vary. In addition, an embedded lock would introduce new lock order
> constraints. Note that this patch does not rule out use of dedicated
> klist locks.
Indeed, I'm currently dealing with such different type of lock issue in
UVM :/
> The patch introduces a way to associate lock operations with a klist.
> The caller can provide a custom implementation, or use a ready-made
> interface with a mutex or rwlock.
>
> For compatibility with old code, the new code falls back to using the
> kernel lock if no specific klist initialization has been done. The
> existing code already relies on implicit initialization of klist.
>
> Unfortunately, the size of struct klist will grow threefold.
The growth is unavoidable, it could have been of the size of a lock...
> As the patch gives the code the ability to operate the klist lock,
> the klist API could provide variants of insert and remove actions that
> handle locking internally, for convenience. However, that I would leave
> for another patch because I would prefer to rename the current
> klist_insert() to klist_insert_locked(), and klist_remove() to
> klist_remove_locked().
>
> The patch additionally provides three examples of usage: audio, pipes,
> and sockets. Each of these examples is logically a separate changeset.
One question below.
> Please test and review.
This is in the middle of a build on sparc64, so far so good.
> Index: kern/kern_event.c
> ===
> RCS file: src/sys/kern/kern_event.c,v
> retrieving revision 1.147
> diff -u -p -r1.147 kern_event.c
> --- kern/kern_event.c 9 Dec 2020 18:58:19 - 1.147
> +++ kern/kern_event.c 11 Dec 2020 17:05:09 -
> @@ -1539,9 +1576,14 @@ klist_invalidate(struct klist *list)
> NET_ASSERT_UNLOCKED();
>
> s = splhigh();
> + ls = klist_lock(list);
Isn't splhigh() redundant with klist_lock() now? If a subsystem
provides its own lock/unlock routine shouldn't it ensure that the
necessary SPL is used? Or is this protecting something else? Or is
it just paranoia and we should try to remove it in a later step?
> while ((kn = SLIST_FIRST(>kl_list)) != NULL) {
> - if (!knote_acquire(kn))
> + if (!knote_acquire(kn, list, ls)) {
> + /* knote_acquire() has unlocked list. */
> + ls = klist_lock(list);
> continue;
> + }
> + klist_unlock(list, ls);
> splx(s);
> kn->kn_fop->f_detach(kn);
> if (kn->kn_fop->f_flags & FILTEROP_ISFD) {
Re: Lock operations for knote lists
On Fri, Dec 11, 2020 at 05:37:57PM +, Visa Hankala wrote:
> This patch extends struct klist with a callback descriptor and
> an argument. The main purpose of this is to let the kqueue subsystem
> assert when a klist should be locked, and operate the klist lock
> in klist_invalidate().
i've always felt that the klist has always had an obvious need for
locking, but it's excellent that you've actually gone and done more than
feel things about it.
> Access to a knote list of a kqueue-monitored object has to be
> serialized somehow. Because the object often has a lock for protecting
> its state, and because the object often acquires this lock at the latest
> in its f_event callback functions, I would like to use the same lock
> also for the knote lists. Uses of NOTE_SUBMIT already show a pattern
> arising.
That makes sense. If it helps, even if it just helps justify it,
this is the same approach used in kstat, for much the same reason.
> There could be an embedded lock in klist. However, such a lock would be
> redundant in many cases. The code could not rely on a single lock type
> (mutex, rwlock, something else) because the needs of monitored objects
> vary. In addition, an embedded lock would introduce new lock order
> constraints. Note that this patch does not rule out use of dedicated
> klist locks.
>
> The patch introduces a way to associate lock operations with a klist.
> The caller can provide a custom implementation, or use a ready-made
> interface with a mutex or rwlock.
>
> For compatibility with old code, the new code falls back to using the
> kernel lock if no specific klist initialization has been done. The
> existing code already relies on implicit initialization of klist.
I was going to ask if you could provide a struct klistops around
KERNEL_LOCK as the default, but that would involve a lot more churn to
explicitly init all the klist structs.
> Unfortunately, the size of struct klist will grow threefold.
I think we'll live.
> As the patch gives the code the ability to operate the klist lock,
> the klist API could provide variants of insert and remove actions that
> handle locking internally, for convenience. However, that I would leave
> for another patch because I would prefer to rename the current
> klist_insert() to klist_insert_locked(), and klist_remove() to
> klist_remove_locked().
>
> The patch additionally provides three examples of usage: audio, pipes,
> and sockets. Each of these examples is logically a separate changeset.
>
>
> Please test and review.
I'll try to have a closer look soon.
> Index: dev/audio.c
> ===
> RCS file: src/sys/dev/audio.c,v
> retrieving revision 1.191
> diff -u -p -r1.191 audio.c
> --- dev/audio.c 19 May 2020 06:32:24 - 1.191
> +++ dev/audio.c 11 Dec 2020 17:05:09 -
> @@ -305,11 +305,12 @@ audio_buf_wakeup(void *addr)
> int
> audio_buf_init(struct audio_softc *sc, struct audio_buf *buf, int dir)
> {
> + klist_init_mutex(>sel.si_note, _lock);
> buf->softintr = softintr_establish(IPL_SOFTAUDIO,
> audio_buf_wakeup, buf);
> if (buf->softintr == NULL) {
> printf("%s: can't establish softintr\n", DEVNAME(sc));
> - return ENOMEM;
> + goto bad;
> }
> if (sc->ops->round_buffersize) {
> buf->datalen = sc->ops->round_buffersize(sc->arg,
> @@ -323,9 +324,12 @@ audio_buf_init(struct audio_softc *sc, s
> buf->data = malloc(buf->datalen, M_DEVBUF, M_WAITOK);
> if (buf->data == NULL) {
> softintr_disestablish(buf->softintr);
> - return ENOMEM;
> + goto bad;
> }
> return 0;
> +bad:
> + klist_free(>sel.si_note);
> + return ENOMEM;
> }
>
> void
> @@ -336,6 +340,7 @@ audio_buf_done(struct audio_softc *sc, s
> else
> free(buf->data, M_DEVBUF, buf->datalen);
> softintr_disestablish(buf->softintr);
> + klist_free(>sel.si_note);
> }
>
> /*
> @@ -1256,6 +1261,7 @@ audio_attach(struct device *parent, stru
> return;
> }
>
> + klist_init_mutex(>mix_sel.si_note, _lock);
> sc->mix_softintr = softintr_establish(IPL_SOFTAUDIO,
> audio_mixer_wakeup, sc);
> if (sc->mix_softintr == NULL) {
> @@ -1451,6 +1457,7 @@ audio_detach(struct device *self, int fl
>
> /* free resources */
> softintr_disestablish(sc->mix_softintr);
> + klist_free(>mix_sel.si_note);
> free(sc->mix_evbuf, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ev));
> free(sc->mix_ents, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ctrl));
> audio_buf_done(sc, >play);
> Index: kern/kern_event.c
> ===
> RCS file: src/sys/kern/kern_event.c,v
> retrieving revision 1.147
> diff -u -p -r1.147 kern_event.c
> --- kern/kern_event.c 9 Dec 2020 18:58:19 - 1.147
> +++
Lock operations for knote lists
This patch extends struct klist with a callback descriptor and
an argument. The main purpose of this is to let the kqueue subsystem
assert when a klist should be locked, and operate the klist lock
in klist_invalidate().
Access to a knote list of a kqueue-monitored object has to be
serialized somehow. Because the object often has a lock for protecting
its state, and because the object often acquires this lock at the latest
in its f_event callback functions, I would like to use the same lock
also for the knote lists. Uses of NOTE_SUBMIT already show a pattern
arising.
There could be an embedded lock in klist. However, such a lock would be
redundant in many cases. The code could not rely on a single lock type
(mutex, rwlock, something else) because the needs of monitored objects
vary. In addition, an embedded lock would introduce new lock order
constraints. Note that this patch does not rule out use of dedicated
klist locks.
The patch introduces a way to associate lock operations with a klist.
The caller can provide a custom implementation, or use a ready-made
interface with a mutex or rwlock.
For compatibility with old code, the new code falls back to using the
kernel lock if no specific klist initialization has been done. The
existing code already relies on implicit initialization of klist.
Unfortunately, the size of struct klist will grow threefold.
As the patch gives the code the ability to operate the klist lock,
the klist API could provide variants of insert and remove actions that
handle locking internally, for convenience. However, that I would leave
for another patch because I would prefer to rename the current
klist_insert() to klist_insert_locked(), and klist_remove() to
klist_remove_locked().
The patch additionally provides three examples of usage: audio, pipes,
and sockets. Each of these examples is logically a separate changeset.
Please test and review.
Index: dev/audio.c
===
RCS file: src/sys/dev/audio.c,v
retrieving revision 1.191
diff -u -p -r1.191 audio.c
--- dev/audio.c 19 May 2020 06:32:24 - 1.191
+++ dev/audio.c 11 Dec 2020 17:05:09 -
@@ -305,11 +305,12 @@ audio_buf_wakeup(void *addr)
int
audio_buf_init(struct audio_softc *sc, struct audio_buf *buf, int dir)
{
+ klist_init_mutex(>sel.si_note, _lock);
buf->softintr = softintr_establish(IPL_SOFTAUDIO,
audio_buf_wakeup, buf);
if (buf->softintr == NULL) {
printf("%s: can't establish softintr\n", DEVNAME(sc));
- return ENOMEM;
+ goto bad;
}
if (sc->ops->round_buffersize) {
buf->datalen = sc->ops->round_buffersize(sc->arg,
@@ -323,9 +324,12 @@ audio_buf_init(struct audio_softc *sc, s
buf->data = malloc(buf->datalen, M_DEVBUF, M_WAITOK);
if (buf->data == NULL) {
softintr_disestablish(buf->softintr);
- return ENOMEM;
+ goto bad;
}
return 0;
+bad:
+ klist_free(>sel.si_note);
+ return ENOMEM;
}
void
@@ -336,6 +340,7 @@ audio_buf_done(struct audio_softc *sc, s
else
free(buf->data, M_DEVBUF, buf->datalen);
softintr_disestablish(buf->softintr);
+ klist_free(>sel.si_note);
}
/*
@@ -1256,6 +1261,7 @@ audio_attach(struct device *parent, stru
return;
}
+ klist_init_mutex(>mix_sel.si_note, _lock);
sc->mix_softintr = softintr_establish(IPL_SOFTAUDIO,
audio_mixer_wakeup, sc);
if (sc->mix_softintr == NULL) {
@@ -1451,6 +1457,7 @@ audio_detach(struct device *self, int fl
/* free resources */
softintr_disestablish(sc->mix_softintr);
+ klist_free(>mix_sel.si_note);
free(sc->mix_evbuf, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ev));
free(sc->mix_ents, M_DEVBUF, sc->mix_nent * sizeof(struct mixer_ctrl));
audio_buf_done(sc, >play);
Index: kern/kern_event.c
===
RCS file: src/sys/kern/kern_event.c,v
retrieving revision 1.147
diff -u -p -r1.147 kern_event.c
--- kern/kern_event.c 9 Dec 2020 18:58:19 - 1.147
+++ kern/kern_event.c 11 Dec 2020 17:05:09 -
@@ -57,6 +57,17 @@
#include
#include
+#ifdef DIAGNOSTIC
+#define KLIST_ASSERT_LOCKED(kl) do { \
+ if ((kl)->kl_ops != NULL) \
+ (kl)->kl_ops->klo_assertlk((kl)->kl_arg); \
+ else\
+ KERNEL_ASSERT_LOCKED(); \
+} while (0)
+#else
+#define KLIST_ASSERT_LOCKED(kl)((void)(kl))
+#endif
+
struct kqueue *kqueue_alloc(struct filedesc *);
void kqueue_terminate(struct proc *p, struct kqueue *);
void kqueue_free(struct kqueue *);
@@ -79,6 +90,8 @@ void
