From owner-freebsd-current@FreeBSD.ORG Sun Dec 29 23:23:27 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 701CEF8F; Sun, 29 Dec 2013 23:23:27 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 44A471119; Sun, 29 Dec 2013 23:23:26 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id rBTNNPvq042576 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 29 Dec 2013 15:23:26 -0800 (PST) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id rBTNNPEo042575; Sun, 29 Dec 2013 15:23:25 -0800 (PST) (envelope-from jmg) Date: Sun, 29 Dec 2013 15:23:25 -0800 From: John-Mark Gurney To: Adrian Chadd Subject: Re: review request: sendfile kqueue notification Message-ID: <20131229232325.GG99167@funkthat.com> Mail-Followup-To: Adrian Chadd , "freebsd-arch@freebsd.org" , freebsd-current References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Sun, 29 Dec 2013 15:23:26 -0800 (PST) Cc: freebsd-current , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 29 Dec 2013 23:23:27 -0000 Adrian Chadd wrote this message on Thu, Dec 12, 2013 at 12:41 -0800: > I'd like to start committing this to FreeBSD-HEAD: > > http://people.freebsd.org/~adrian/netflix/20131211-sendfile-kqueue-11.diff > > It implements kqueue notifications for sendfile so users can get an > asynchronous notification that the underlying mbufs have been freed. > This allows userland users of sendfile to know that the underlying > memory / file object can be recycled or overwritten. Right now the > only way to do this is to set SF_SYNC and this causes sendfile() to > sleep until the transaction is complete and the mbufs have been freed. > > I've been testing this out locally in my lab environment and it's > running flawlessly at 30gbit/sec of TCP across 32,768 active > transmitting sockets. > > I'd like to start merging this into -HEAD in small pieces to make it > easier to MFC to -10. I have some questions about this. In filt_sfsync_detach, you have: + if (!knlist_empty(knl)) + knlist_remove(knl, kn, 1); Have you had a panic where you were in _detach but the list was empty? a better check instead of knlist_empty is: if (!(kn->kn_status & KN_DETACHED)) You probably copied code from vfs_aio.c, which should probably also use this construct instead. I've noticed that you're manually locking the knotelist, and I should look at this further, but if we really are going to have external people doing lock/unlock, we should make macros for this instead of hard coding it everywhere. Currently only vfs_aio.c is doing this outside of kern_event.c. In all cases, calls to f_event (filt_sfsync) will have the list locked, so turning that into an assert would be correct. We should probably report sendfile errors via the knote. It shouldn't be hard as the kevent data field can be used for this and setting the EV_ERROR. You shouldn't set data to be the sfs pointer in _setup as that can be leaked to userland, and you never use it (maybe you do in your userland side). You also might want to set sfs to NULL after calling sf_sync_free to make sure you don't acidentally use it after you've [possibly] free'd it. I didn't look too closely at the sf logic but what I did see looks fine. Overall I think the patch looks fine, though there are a few XXX that still should be addressed on your side. P.S. When generating a diff, using svn -x -up is useful to show the function code blocks are in. -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."