Date: Thu, 28 Nov 2013 10:20:00 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Adrian Chadd <adrian@freebsd.org> 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: <20131128082000.GK59496@kib.kiev.ua> In-Reply-To: <CAJ-VmomGd_-v7caBV1GxHhOAXG9Mgb3dVPh7vqAbZQWY3iiOYA@mail.gmail.com> References: <CAJ-VmomGd_-v7caBV1GxHhOAXG9Mgb3dVPh7vqAbZQWY3iiOYA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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. [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQIcBAEBAgAGBQJSlvywAAoJEJDCuSvBvK1Bs9gP/iv7zcSWRf51kPbynKkWlM42 XZx4Tg6OnMkykU/G/M5091AyBgtM6NdJcalmtn5suFST5xa42gP0XXhWx7PhRdTQ m+tCqEqs19QNhElDCQ9a/BtemTuEN1ZIg4vYyXpG1qH4mWK+hJr+zJrQNcEMUXDo P3n4t7H5p4z9+20ETIQUGMqyuP0xHlAafvljGTuDzgDfk+y/3LN8Y8bM6VTGDm9B NzCbkP4cvMzuoeGiHOzpv13xd7jiLPJxaDbzUJQNQblWR1j9cWZe/z4AnAeZpm3z n5Pon2SeDxIi3i8p357J3H4UouDEwN52yt7DaXCUTSaRjI1T1ElDmMOnZAkl3YxX f8ZWP+YtJCaS+ZOW3OzV6pIqDVNkLpXuuNrsj2AISNNyz7SxwL7WEB3aRGaksj1f IVEBGs0CDboG0DYO49AFcEJCih1c6FvSvKyzkI6dvyX6L+1di3BnrpIIe7SKxUfb bxl6vxuGuFHSQhGKcb6eT9D6RaDIrQBhHzyvMWaWGFqdITOSH/7uHqk5twYfFBN8 8hd4NVEG8Q3zv6UHjWX+wYf3Vs/NcZ9ayEWjimdSsUqykuerDBUfTWm5t6uuupNp QxiHGI85nEGx2xxiQW22vLn1ESeaP0oY7qWXd3i9GcUP3h0p5uVnpB+ZYPbP+1s9 v991+RluwIIic34q+5I8 =MPo5 -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131128082000.GK59496>