Date: Sun, 29 Dec 2013 15:23:25 -0800 From: John-Mark Gurney <jmg@funkthat.com> To: Adrian Chadd <adrian@freebsd.org> Cc: freebsd-current <freebsd-current@freebsd.org>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: review request: sendfile kqueue notification Message-ID: <20131229232325.GG99167@funkthat.com> In-Reply-To: <CAJ-VmomtUYE8O1XGy6a8OSbZwnxRzC1zfU9cx=V1=tBOOZywQg@mail.gmail.com> References: <CAJ-VmomtUYE8O1XGy6a8OSbZwnxRzC1zfU9cx=V1=tBOOZywQg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131229232325.GG99167>