Re: review request: sendfile kqueue notification

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."