From owner-freebsd-current@FreeBSD.ORG Thu Nov 28 08:33:09 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id CB40E886; Thu, 28 Nov 2013 08:33:09 +0000 (UTC) Received: from mail-qe0-x229.google.com (mail-qe0-x229.google.com [IPv6:2607:f8b0:400d:c02::229]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 661CF1D8A; Thu, 28 Nov 2013 08:33:09 +0000 (UTC) Received: by mail-qe0-f41.google.com with SMTP id gh4so6251511qeb.0 for ; Thu, 28 Nov 2013 00:33:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=CZB3Yt/dOFXcZK5gTD2hVMe4YLuhFVyzTBRbQlOZMhU=; b=Jx5aAIdGBlW0WlDahoK60Jps8ESnOqZcvqmkxxwFF/ZgSFipgSz58iOpv4JHUgG/3F eZtIdDPqw7W0cH63EOyhqaEFg+OfILDcbZT9JkT2QhOabftHbJVoOi4WOFGSrLYEpUhB X2v04n1NKX9MA+aXzoGQxloTyyNTBWi9XIjk6h9zEr+HVn5sOWd3gC17vl0g67QubKpX dn8w75pUdWDgnA1M0FKc6mUKpshuEOvusMnZ0VAYF4spGC8Rh05snWqGKmNy9DN/1CRE HfDbGSeBtyinWKFNiaOIaOsTQ0H+cfCv9CGOlCc8VI4p50w3MZb2AdOZkEX8/2iMMhVv SVLQ== MIME-Version: 1.0 X-Received: by 10.224.111.197 with SMTP id t5mr74848653qap.49.1385627587884; Thu, 28 Nov 2013 00:33:07 -0800 (PST) Sender: adrian.chadd@gmail.com Received: by 10.224.53.200 with HTTP; Thu, 28 Nov 2013 00:33:07 -0800 (PST) In-Reply-To: <20131128082000.GK59496@kib.kiev.ua> References: <20131128082000.GK59496@kib.kiev.ua> Date: Thu, 28 Nov 2013 00:33:07 -0800 X-Google-Sender-Auth: R-KhTJ2Tg2a8hK0JPoWnR4Q9HCA Message-ID: Subject: Re: [review] sendfile / sendfile_sync refactoring From: Adrian Chadd To: Konstantin Belousov Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-current , Gleb Smirnoff , "freebsd-arch@freebsd.org" X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Nov 2013 08:33:10 -0000 Hi, It's definitely a work in progress, as you've observed. On 28 November 2013 00:20, Konstantin Belousov 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