Date: Thu, 28 Nov 2013 00:33:07 -0800 From: Adrian Chadd <adrian@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-current <freebsd-current@freebsd.org>, Gleb Smirnoff <glebius@freebsd.org>, "freebsd-arch@freebsd.org" <freebsd-arch@freebsd.org> Subject: Re: [review] sendfile / sendfile_sync refactoring Message-ID: <CAJ-Vmo=t%2BuBCoNjmnk9ZtpKjLULaBCROnMGXu7N=kSkzPBmO=g@mail.gmail.com> In-Reply-To: <20131128082000.GK59496@kib.kiev.ua> References: <CAJ-VmomGd_-v7caBV1GxHhOAXG9Mgb3dVPh7vqAbZQWY3iiOYA@mail.gmail.com> <20131128082000.GK59496@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi, It's definitely a work in progress, as you've observed. On 28 November 2013 00:20, Konstantin Belousov <kostikbel@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=t%2BuBCoNjmnk9ZtpKjLULaBCROnMGXu7N=kSkzPBmO=g>