Re: bpf_catchpacket and bpf_wakeup optimisations

2020-12-28 Thread David Gwynne
On Mon, Dec 28, 2020 at 02:45:06PM +1000, David Gwynne wrote:
> now that bpf read timeouts are only handled on the bpfread() side,
> there's a simplification that can be made in bpf_catchpacket. the chunk
> in bpf_catchpacket that rotates the buffers when one gets full already
> does a wakeup, so we don't have to check if we have any waiting readers
> and wake them up when a buffer gets full.
> 
> we can use bd_nreaders to omgoptimise bpf_wakeup though. wakeup(9) is
> mpsafe, so we don't have to defer the call to a task. however, we can
> avoid calling wakeup() and therefore trying to take the sched lock and
> all that stuff when we know there's nothing sleeping.
> 
> this also avoids scheduling the task if there's no async stuff set up.
> it's a bit magical because it knows what's inside selwakeup.
> 
> tests? ok?

visa pointed out that i missed a bit relating to kq.

Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.199
diff -u -p -r1.199 bpf.c
--- bpf.c   26 Dec 2020 16:30:58 -  1.199
+++ bpf.c   29 Dec 2020 06:04:58 -
@@ -554,7 +554,6 @@ out:
return (error);
 }
 
-
 /*
  * If there are processes sleeping on this descriptor, wake them up.
  */
@@ -563,14 +562,20 @@ bpf_wakeup(struct bpf_d *d)
 {
MUTEX_ASSERT_LOCKED(>bd_mtx);
 
+   if (d->bd_nreaders)
+   wakeup(d);
+
/*
 * As long as pgsigio() 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);
-   if (!task_add(systq, >bd_wake_task))
-   bpf_put(d);
+   if ((d->bd_async && d->bd_sig) ||
+   (!klist_empty(>bd_sel.si_note) || d->bd_sel.si_seltid != 0)) {
+   bpf_get(d);
+   if (!task_add(systq, >bd_wake_task))
+   bpf_put(d);
+   }
 }
 
 void
@@ -578,7 +583,6 @@ bpf_wakeup_cb(void *xd)
 {
struct bpf_d *d = xd;
 
-   wakeup(d);
if (d->bd_async && d->bd_sig)
pgsigio(>bd_sigio, d->bd_sig, 0);
 
@@ -1542,17 +1546,6 @@ bpf_catchpacket(struct bpf_d *d, u_char 
 * reads should be woken up.
 */
do_wakeup = 1;
-   }
-
-   if (d->bd_nreaders > 0) {
-   /*
-* We have one or more threads sleeping in bpfread().
-* We got a packet, so wake up all readers.
-*/
-   if (d->bd_fbuf != NULL) {
-   ROTATE_BUFFERS(d);
-   do_wakeup = 1;
-   }
}
 
if (do_wakeup)



bpf_catchpacket and bpf_wakeup optimisations

2020-12-27 Thread David Gwynne
now that bpf read timeouts are only handled on the bpfread() side,
there's a simplification that can be made in bpf_catchpacket. the chunk
in bpf_catchpacket that rotates the buffers when one gets full already
does a wakeup, so we don't have to check if we have any waiting readers
and wake them up when a buffer gets full.

we can use bd_nreaders to omgoptimise bpf_wakeup though. wakeup(9) is
mpsafe, so we don't have to defer the call to a task. however, we can
avoid calling wakeup() and therefore trying to take the sched lock and
all that stuff when we know there's nothing sleeping.

this also avoids scheduling the task if there's no async stuff set up.
it's a bit magical because it knows what's inside selwakeup.

tests? ok?

Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.199
diff -u -p -r1.199 bpf.c
--- bpf.c   26 Dec 2020 16:30:58 -  1.199
+++ bpf.c   28 Dec 2020 03:05:36 -
@@ -563,14 +563,19 @@ bpf_wakeup(struct bpf_d *d)
 {
MUTEX_ASSERT_LOCKED(>bd_mtx);
 
+   if (d->bd_nreaders)
+   wakeup(d);
+
/*
 * As long as pgsigio() 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);
-   if (!task_add(systq, >bd_wake_task))
-   bpf_put(d);
+   if ((d->bd_async && d->bd_sig) || (d->bd_sel.si_seltid != 0)) {
+   bpf_get(d);
+   if (!task_add(systq, >bd_wake_task))
+   bpf_put(d);
+   }
 }
 
 void
@@ -578,7 +583,6 @@ bpf_wakeup_cb(void *xd)
 {
struct bpf_d *d = xd;
 
-   wakeup(d);
if (d->bd_async && d->bd_sig)
pgsigio(>bd_sigio, d->bd_sig, 0);
 
@@ -1542,17 +1546,6 @@ bpf_catchpacket(struct bpf_d *d, u_char 
 * reads should be woken up.
 */
do_wakeup = 1;
-   }
-
-   if (d->bd_nreaders > 0) {
-   /*
-* We have one or more threads sleeping in bpfread().
-* We got a packet, so wake up all readers.
-*/
-   if (d->bd_fbuf != NULL) {
-   ROTATE_BUFFERS(d);
-   do_wakeup = 1;
-   }
}
 
if (do_wakeup)