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 _______________________________________________ firstname.lastname@example.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"