On Wed, Nov 27, 2013 at 11:36:44AM -0800, Adrian Chadd wrote: > Hi, > > Here's part #2 in my sendfile refactoring. This is a little more > intrusive than the first patch. > > http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor-2.diff > sf_buf.h is for sf buffers, and not for sendfile stuff. Please do not pollute this header. The changes you have to make to if_ti.c illustrate my point.
The sys/event.h change of adding new kevent type is useless now and has no relation to the rest of the patch. Patch has too many XXX notes for its triviality, some of which are baseless, e.g. the comment for struct sendfile_sync forward declaration in sys/file.h. You extracted all sfs accesses from the sendfile implementation, except the one in sf_buf_mext(). This is weird, please move the code from sf_buf_mext() into static function and put it near sf_sync_ref(). While moving the code into sf_sync_syscall_wait(), please use the opportunity and replace the if (sfs->count != 0) with the while () cv_wait(); loop, to avoid spurious wakeups. I do not see any use for sfs->flags. Also, I do not see any use for passing the flags masked with SF_SYNC, which is always set, to sf_sync_alloc(). If the flags are going to be useful later, it should be added when needed later. The sf_sync_* stuff in the compat32 code just duplicates the main syscall. It should be done in the common code. > My aim here is to move the sendfile_sync stuff out of uipc_syscalls.c > and out of sendfile-only code and into something that can be reused > elsewhere or replaced later on. I've created methods for all the > sendfile_sync stuff, I've moved it out of the do_sendfile() loop so > do_sendfile() doesn't specifically implement the completion behaviour. As is, the patch is sincere nop. Modulo the comments above, I do not object against it.
Description: PGP signature