On Mon, 15 Aug 2016, Martin Pieuchot wrote:
> I'd like to make sure that bpf_tap(9) does not grab the KERNEL_LOCK().
> The reason is to reduce the potential lock ordering problems within PF.
>
> I'm currently using a mutex to serialize buffer changes, but since
> bpf_wakeup() will still need the KERNEL_LOCK(), I'm using a task for
> that.
Yes, this is a good direction to unblock the network stack work.
However, I suspect there's a bug below.
> @@ -515,12 +519,28 @@ bpfread(dev_t dev, struct uio *uio, int
> void
> bpf_wakeup(struct bpf_d *d)
> {
> - wakeup((caddr_t)d);
> + /*
> + * As long as csignal() and selwakeup() need to be protected
> + * by the KERNEL_LOCK() we have to delay the wakeup to
> + * another context to keep the hot path KERNEL_LOCK()-free.
> + */
> + bpf_get(d);
> + task_add(systq, &d->bd_wake_task);
We increment the reference count here and decrement it in bpf_wakeup_cb().
However, if the task is already queued here in bpf_wakeup() then won't it
get bumped again and only released once? I suspect this should be:
bpf_get(d);
if (!task_add(systq, &d->bd_wake_task))
bpf_put(d);
dlg@, am I misremembering when that check is necessary?
Philip Guenther
