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 -0000 1.147
> +++ kern/kern_event.c 11 Dec 2020 17:05:09 -0000
> @@ -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(&list->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) {
