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
--H55dy74Id38Dzf32 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 27, 2013 at 11:36:44AM -0800, Adrian Chadd wrote: > Hi, >=20 > Here's part #2 in my sendfile refactoring. This is a little more > intrusive than the first patch. >=20 > http://people.freebsd.org/~adrian/netflix/20131126-sendfile-sync-refactor= -2.diff >=20 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=20 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 !=3D 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. --H55dy74Id38Dzf32 Content-Type: application/pgp-signature -----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----- --H55dy74Id38Dzf32--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131128082000.GK59496>