Re: [review] sendfile / sendfile_sync refactoring
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. pgp_ieq0LfEKW.pgp Description: PGP signature
Re: [review] sendfile / sendfile_sync refactoring
Hi, It's definitely a work in progress, as you've observed. On 28 November 2013 00:20, Konstantin Belousov kostik...@gmail.com wrote: 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. Yup. I don't like having it in here either. I may create a new header file specifically for this. The sys/event.h change of adding new kevent type is useless now and has no relation to the rest of the patch. Yeah, ignore that bit. I'm working on adding code for that. 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(). I plan on doing that soon. 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. It's just precursor work to add support for other SF_* flags - specifically, a new kqueue notification flag for completion. The sf_sync_* stuff in the compat32 code just duplicates the main syscall. It should be done in the common code. The main motivation for moving the sendfile_sync setup and teardown out of do_sendfile() is so do_sendfile() can keep a very little/light idea of the behaviour of sendfile_sync. I don't mind keeping the code in there. Thanks for your feedback. I'll post an updated diff when I've fleshed out some more of this. -a ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
[review] sendfile / sendfile_sync refactoring
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 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. Initially, I'm going to implement a sendfile knote representing the completion of this particular mbuf transaction. I also have a vague, handwav-y idea to use this kind of mbuf transaction representation for later work with aio_write() (and maybe an aio_writev()) when writing to a socket - wire in the userland memory, create a chain of mbufs to represent those, wrap them up in a sendfile_sync (or whatever it mutates into) and then use that for the kqueue completion notification. It needs the same kind of mbuf transaction and kqueue notification mechanism so I'd like to eventually make the sendfile_sync stuff generic, or use the memory buffer representation from jhb, to implement this in a variety of places. I'm not entirely happy where the sendfile_sync stuff is living but this is all meant to be transition-y and enable further hacking on sendfile / variations thereof without having to duplicate too much code. Thanks, -adrian ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
