From owner-freebsd-net@FreeBSD.ORG Fri Jan 11 23:49:04 2013 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id DF53FBE8 for ; Fri, 11 Jan 2013 23:49:04 +0000 (UTC) (envelope-from csjp@sqrt.ca) Received: from sub.sqrt.ca (sub.sqrt.ca [205.200.235.40]) by mx1.freebsd.org (Postfix) with ESMTP id B946DA0D for ; Fri, 11 Jan 2013 23:49:03 +0000 (UTC) Received: by sub.sqrt.ca (Postfix, from userid 58) id 5D2F9B82B; Fri, 11 Jan 2013 17:48:57 -0600 (CST) X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sub X-Spam-Level: X-Spam-Status: No, score=-2.9 required=2.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham version=3.3.2 Received: from [IPv6:::1] (localhost [127.0.0.1]) by sub.sqrt.ca (Postfix) with ESMTP id 69F42B819; Fri, 11 Jan 2013 17:48:53 -0600 (CST) Subject: Re: bpf hold buffer in-use flag Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=us-ascii From: Christian Peron In-Reply-To: <9C928117-2230-4F01-9B95-B6D945AF4416@gmail.com> Date: Fri, 11 Jan 2013 17:48:53 -0600 Content-Transfer-Encoding: quoted-printable Message-Id: <0AAF6A81-2594-449E-A38D-28DE5B055AFF@sqrt.ca> References: <9C928117-2230-4F01-9B95-B6D945AF4416@gmail.com> To: Guy Helmer X-Mailer: Apple Mail (2.1283) Cc: "freebsd-net@freebsd.org" X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jan 2013 23:49:04 -0000 Guy, Can you please describe the race and how to reproduce it? When we = introduced zerocopy-bpf, we also introduced bpf_bufheld() and = bpf_bufreclaimed() which are effectively hops for the regular buffer = mode. Maybe we could maintain state there as well? In any case, I would = like to understand the race/and reproduce it Thanks! On 2012-11-13, at 3:40 PM, Guy Helmer wrote: > To try to completely resolve the race in bpfread(), I have put = together these changes to add a flag to indicate when the hold buffer = cannot be modified because it is in use. Since it's my first time using = mtx_sleep() and wakeup(), I wanted to run these past the list to see if = I can get any feedback on the approach. >=20 >=20 > Index: bpf.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bpf.c (revision 242997) > +++ bpf.c (working copy) > @@ -819,6 +819,7 @@ bpfopen(struct cdev *dev, int flags, int fmt, stru > * particular buffer method. > */ > bpf_buffer_init(d); > + d->bd_hbuf_in_use =3D 0; > d->bd_bufmode =3D BPF_BUFMODE_BUFFER; > d->bd_sig =3D SIGIO; > d->bd_direction =3D BPF_D_INOUT; > @@ -872,6 +873,9 @@ bpfread(struct cdev *dev, struct uio *uio, int iof > callout_stop(&d->bd_callout); > timed_out =3D (d->bd_state =3D=3D BPF_TIMED_OUT); > d->bd_state =3D BPF_IDLE; > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, > + PRINET|PCATCH, "bd_hbuf", 0); > /* > * If the hold buffer is empty, then do a timed sleep, which > * ends when the timeout expires or when enough packets > @@ -940,27 +944,27 @@ bpfread(struct cdev *dev, struct uio *uio, int = iof > /* > * At this point, we know we have something in the hold slot. > */ > + d->bd_hbuf_in_use =3D 1; > BPFD_UNLOCK(d); >=20 > /* > * Move data from hold buffer into user space. > * We know the entire buffer is transferred since > * we checked above that the read buffer is bpf_bufsize bytes. > - * > - * XXXRW: More synchronization needed here: what if a second = thread > - * issues a read on the same fd at the same time? Don't want = this > - * getting invalidated. > + * > + * We do not have to worry about simultaneous reads because > + * we waited for sole access to the hold buffer above. > */ > error =3D bpf_uiomove(d, d->bd_hbuf, d->bd_hlen, uio); >=20 > BPFD_LOCK(d); > - if (d->bd_hbuf !=3D NULL) { > - /* Free the hold buffer only if it is still valid. */ > - d->bd_fbuf =3D d->bd_hbuf; > - d->bd_hbuf =3D NULL; > - d->bd_hlen =3D 0; > - bpf_buf_reclaimed(d); > - } > + KASSERT(d->bd_hbuf !=3D NULL, ("bpfread: lost bd_hbuf")); > + d->bd_fbuf =3D d->bd_hbuf; > + d->bd_hbuf =3D NULL; > + d->bd_hlen =3D 0; > + bpf_buf_reclaimed(d); > + d->bd_hbuf_in_use =3D 0; > + wakeup(&d->bd_hbuf_in_use); > BPFD_UNLOCK(d); >=20 > return (error); > @@ -1114,6 +1118,9 @@ reset_d(struct bpf_d *d) >=20 > BPFD_LOCK_ASSERT(d); >=20 > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, PRINET, > + "bd_hbuf", 0); > if ((d->bd_hbuf !=3D NULL) && > (d->bd_bufmode !=3D BPF_BUFMODE_ZBUF || bpf_canfreebuf(d))) = { > /* Free the hold buffer. */ > @@ -1254,6 +1261,9 @@ bpfioctl(struct cdev *dev, u_long cmd, caddr_t = add >=20 > BPFD_LOCK(d); > n =3D d->bd_slen; > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, = &d->bd_lock, > + PRINET, "bd_hbuf", 0); > if (d->bd_hbuf) > n +=3D d->bd_hlen; > BPFD_UNLOCK(d); > @@ -1967,6 +1977,9 @@ filt_bpfread(struct knote *kn, long hint) > ready =3D bpf_ready(d); > if (ready) { > kn->kn_data =3D d->bd_slen; > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, > + PRINET, "bd_hbuf", 0); > if (d->bd_hbuf) > kn->kn_data +=3D d->bd_hlen; > } else if (d->bd_rtout > 0 && d->bd_state =3D=3D BPF_IDLE) { > @@ -2299,6 +2312,9 @@ catchpacket(struct bpf_d *d, u_char *pkt, u_int = pk > * spot to do it. > */ > if (d->bd_fbuf =3D=3D NULL && bpf_canfreebuf(d)) { > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, > + PRINET, "bd_hbuf", 0); > d->bd_fbuf =3D d->bd_hbuf; > d->bd_hbuf =3D NULL; > d->bd_hlen =3D 0; > @@ -2341,6 +2357,9 @@ catchpacket(struct bpf_d *d, u_char *pkt, u_int = pk > ++d->bd_dcount; > return; > } > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, > + PRINET, "bd_hbuf", 0); > ROTATE_BUFFERS(d); > do_wakeup =3D 1; > curlen =3D 0; > Index: bpf.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bpf.h (revision 242997) > +++ bpf.h (working copy) > @@ -1235,7 +1235,8 @@ SYSCTL_DECL(_net_bpf); > /* > * Rotate the packet buffers in descriptor d. Move the store buffer = into the > * hold slot, and the free buffer ino the store slot. Zero the length = of the > - * new store buffer. Descriptor lock should be held. > + * new store buffer. Descriptor lock should be held. Hold buffer = must > + * not be marked "in use". > */ > #define ROTATE_BUFFERS(d) do { = \ > (d)->bd_hbuf =3D (d)->bd_sbuf; = \ > Index: bpf_buffer.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bpf_buffer.c (revision 242997) > +++ bpf_buffer.c (working copy) > @@ -189,6 +189,9 @@ bpf_buffer_ioctl_sblen(struct bpf_d *d, u_int *i) > return (EINVAL); > } >=20 > + while (d->bd_hbuf_in_use) > + mtx_sleep(&d->bd_hbuf_in_use, &d->bd_lock, > + PRINET, "bd_hbuf", 0); > /* Free old buffers if set */ > if (d->bd_fbuf !=3D NULL) > free(d->bd_fbuf, M_BPF); > Index: bpfdesc.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bpfdesc.h (revision 242997) > +++ bpfdesc.h (working copy) > @@ -63,6 +63,7 @@ struct bpf_d { > caddr_t bd_sbuf; /* store slot */ > caddr_t bd_hbuf; /* hold slot */ > caddr_t bd_fbuf; /* free slot */ > + int bd_hbuf_in_use; /* don't rotate buffers */ > int bd_slen; /* current length of store = buffer */ > int bd_hlen; /* current length of hold buffer = */ >=20 > _______________________________________________ > freebsd-net@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-net > To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"