Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 12 Mar 2004 16:05:22 -0500 (EST)
From:      Dave Chapeskie <dchapeskie@sandvine.com>
To:        FreeBSD-gnats-submit@FreeBSD.org
Subject:   kern/64178: kqueue does not work with bpf when using BIOCIMMEDIATE or BIOCSRTIMEOUT [PATCH]
Message-ID:  <20040312210522.86CA78B802@squigy.ddm.wox.org>
Resent-Message-ID: <200403122110.i2CLAFHB090508@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help

>Number:         64178
>Category:       kern
>Synopsis:       kqueue does not work with bpf when using BIOCIMMEDIATE or BIOCSRTIMEOUT [PATCH]
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Fri Mar 12 13:10:15 PST 2004
>Closed-Date:
>Last-Modified:
>Originator:     Dave Chapeskie
>Release:        FreeBSD 4.9-STABLE i386
>Organization:
Sandvine Inc.
>Environment:
FreeBSD 4.9R and FreeBSD 5 since Aug 2003

>Description:
When bpf is used with BIOCIMMEDIATE the first packet after a read fails
to active attached knotes and the returned read count does not account
for most recent packet.  If the received packet needs a response before
additional packets will arrive this would cause the application to fail.

When bpf is used with BIOCSRTIMEOUT set to a non-zero value, knotes are
rarely activated at the correct time.

If the attached interface should become detached knotes are not
activated and the application will wait forever (even if there are some
packets available in the slot buffer).
>How-To-Repeat:
Open a bpf node, issue a BIOCIMMEDIATE and/or a BIOCSRTIMEOUT ioctl,
register a kevent on the bpf descriptor, wait for activity with kqueue.
While waiting send a single packet that matches the filter.

>Fix:
Unlike select/poll, bpf does not know when kevent call has blocked so the
exact same behaviour with respect to BIOCSRTIMEOUT can not be
achieved.  Instead it is assumed that any attached knote indicates the
process is waiting (or will shortly) and therefore the read timeout is
immediatly started when the knote is first registered and after every
read or ioctl call.

My tests indicate that with this change a kqueue enabled application
using BIOCSRTIMEOUT sees nearly identical results to the select(2)
version of the application.


The following patches (against RELENG_4 and HEAD) fixes all the issues
with kqueue on a bpf node that I found.

- Deffer the call to bpf_wakeup() in catchpacket() since filt_bpfread needs
  to see the new value of slen.
- Keep the read timeout callout active while any knotes are attached.
- Activate knotes when the interface is not attached (bf_bif==0), a
  following read call will return ENXIO in this case.
(RELENG_4 only):
- Add a couple of SPLASSERT() calls.


For RELENG_4:

Index: bpf.c
===================================================================
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.59.2.13
diff -u -u -r1.59.2.13 bpf.c
--- bpf.c	21 Aug 2003 23:50:54 -0000	1.59.2.13
+++ bpf.c	12 Mar 2004 20:46:32 -0000
@@ -163,6 +163,11 @@
 static struct filterops bpfread_filtops =
 	{ 1, NULL, filt_bpfdetach, filt_bpfread };
 
+/* Test if we need to start the read timeout when we have knotes. */
+#define BPF_KNOTE_NEEDCALLOUT(bd)					\
+		(!SLIST_EMPTY(&(bd)->bd_sel.si_note)			\
+		&& (bd)->bd_rtout > 0 && (bd)->bd_state == BPF_IDLE)
+
 static int
 bpf_movein(uio, linktype, mp, sockp, datlen)
 	register struct uio *uio;
@@ -557,6 +562,12 @@
 	d->bd_fbuf = d->bd_hbuf;
 	d->bd_hbuf = 0;
 	d->bd_hlen = 0;
+
+	/* Start the read timeout if there are any knotes attached. */
+	if (BPF_KNOTE_NEEDCALLOUT(d)) {
+		callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+		d->bd_state = BPF_WAITING;
+	}
 	splx(s);
 
 	return (error);
@@ -570,6 +581,7 @@
 bpf_wakeup(d)
 	register struct bpf_d *d;
 {
+	SPLASSERT(imp, "bpf_wakeup()");
 	if (d->bd_state == BPF_WAITING) {
 		callout_stop(&d->bd_callout);
 		d->bd_state = BPF_IDLE;
@@ -660,6 +672,7 @@
 reset_d(d)
 	struct bpf_d *d;
 {
+	SPLASSERT(imp, "bpf reset_d()");
 	if (d->bd_hbuf) {
 		/* Free the hold buffer. */
 		d->bd_fbuf = d->bd_hbuf;
@@ -966,6 +979,13 @@
 		*(u_int *)addr = d->bd_sig;
 		break;
 	}
+	s = splimp();
+	/* Start the read timeout if there are any knotes attached. */
+	if (BPF_KNOTE_NEEDCALLOUT(d)) {
+		callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+		d->bd_state = BPF_WAITING;
+	}
+	splx(s);
 	return (error);
 }
 
@@ -1131,6 +1151,11 @@
 	kn->kn_hook = (caddr_t)d;	
 	s = splimp();
 	SLIST_INSERT_HEAD(&d->bd_sel.si_note, kn, kn_selnext);
+	/* Start the read timeout if required. */
+	if (d->bd_rtout > 0 && d->bd_state == BPF_IDLE) {
+	    callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+	    d->bd_state = BPF_WAITING;
+	}
 	splx(s);
 
 	return (0);
@@ -1157,7 +1182,7 @@
 	int s, ready;
 
 	s = splimp();
-	ready = bpf_ready(d);
+	ready = (d->bd_bif == NULL || bpf_ready(d));
 	if (ready) {
 		kn->kn_data = d->bd_slen;
 		if (d->bd_hbuf)
@@ -1274,6 +1299,7 @@
 	register struct bpf_hdr *hp;
 	register int totlen, curlen;
 	register int hdrlen = d->bd_bif->bif_hdrlen;
+	int need_wakeup = 0;
 	/*
 	 * Figure out how many bytes to move.  If the packet is
 	 * greater or equal to the snapshot length, transfer that
@@ -1303,7 +1329,7 @@
 			return;
 		}
 		ROTATE_BUFFERS(d);
-		bpf_wakeup(d);
+		++need_wakeup;
 		curlen = 0;
 	}
 	else if (d->bd_immediate || d->bd_state == BPF_TIMED_OUT)
@@ -1312,7 +1338,7 @@
 		 * already expired during a select call.  A packet
 		 * arrived, so the reader should be woken up.
 		 */
-		bpf_wakeup(d);
+		++need_wakeup;
 
 	/*
 	 * Append the bpf header.
@@ -1332,6 +1358,8 @@
 	 */
 	(*cpfn)(pkt, (u_char *)hp + hdrlen, (hp->bh_caplen = totlen - hdrlen));
 	d->bd_slen = curlen + totlen;
+	if (need_wakeup)
+		bpf_wakeup(d);
 }
 
 /*



For -CURRENT:

Index: bpf.c
===================================================================
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.124
diff -u -u -r1.124 bpf.c
--- bpf.c	29 Feb 2004 15:32:33 -0000	1.124
+++ bpf.c	12 Mar 2004 20:49:54 -0000
@@ -142,6 +142,11 @@
 static struct filterops bpfread_filtops =
 	{ 1, NULL, filt_bpfdetach, filt_bpfread };
 
+/* Test if we need to start the read timeout when we have knotes. */
+#define BPF_KNOTE_NEEDCALLOUT(bd)					\
+		(!SLIST_EMPTY(&(bd)->bd_sel.si_note)			\
+		 && (bd)->bd_rtout > 0 && (bd)->bd_state == BPF_IDLE)
+
 static int
 bpf_movein(uio, linktype, mp, sockp, datlen)
 	struct uio *uio;
@@ -507,6 +512,12 @@
 	d->bd_fbuf = d->bd_hbuf;
 	d->bd_hbuf = 0;
 	d->bd_hlen = 0;
+
+	/* Start the read timeout if there are any knotes attached. */
+	if (BPF_KNOTE_NEEDCALLOUT(d)) {
+	    callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+	    d->bd_state = BPF_WAITING;
+	}
 	BPFD_UNLOCK(d);
 
 	return (error);
@@ -924,6 +935,13 @@
 		*(u_int *)addr = d->bd_sig;
 		break;
 	}
+	BPFD_LOCK(d);
+	/* Start the read timeout if there are any knotes attached. */
+	if (BPF_KNOTE_NEEDCALLOUT(d)) {
+	    callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+	    d->bd_state = BPF_WAITING;
+	}
+	BPFD_UNLOCK(d);
 	return (error);
 }
 
@@ -1094,6 +1112,11 @@
 	kn->kn_hook = d;
 	BPFD_LOCK(d);
 	SLIST_INSERT_HEAD(&d->bd_sel.si_note, kn, kn_selnext);
+	/* Start the read timeout if required. */
+	if (d->bd_rtout > 0 && d->bd_state == BPF_IDLE) {
+	    callout_reset(&d->bd_callout, d->bd_rtout, bpf_timed_out, d);
+	    d->bd_state = BPF_WAITING;
+	}
 	BPFD_UNLOCK(d);
 
 	return (0);
@@ -1119,7 +1142,7 @@
 	int ready;
 
 	BPFD_LOCK(d);
-	ready = bpf_ready(d);
+	ready = (d->bd_bif == NULL || bpf_ready(d));
 	if (ready) {
 		kn->kn_data = d->bd_slen;
 		if (d->bd_hbuf)
@@ -1289,6 +1312,7 @@
 	struct bpf_hdr *hp;
 	int totlen, curlen;
 	int hdrlen = d->bd_bif->bif_hdrlen;
+	int need_wakeup = 0;
 
 	/*
 	 * Figure out how many bytes to move.  If the packet is
@@ -1319,7 +1343,7 @@
 			return;
 		}
 		ROTATE_BUFFERS(d);
-		bpf_wakeup(d);
+		++need_wakeup;
 		curlen = 0;
 	}
 	else if (d->bd_immediate || d->bd_state == BPF_TIMED_OUT)
@@ -1328,7 +1352,7 @@
 		 * already expired during a select call.  A packet
 		 * arrived, so the reader should be woken up.
 		 */
-		bpf_wakeup(d);
+		++need_wakeup;
 
 	/*
 	 * Append the bpf header.
@@ -1342,6 +1366,8 @@
 	 */
 	(*cpfn)(pkt, (u_char *)hp + hdrlen, (hp->bh_caplen = totlen - hdrlen));
 	d->bd_slen = curlen + totlen;
+	if (need_wakeup)
+		bpf_wakeup(d);
 }
 
 /*
>Release-Note:
>Audit-Trail:
>Unformatted:



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040312210522.86CA78B802>