From owner-freebsd-arch@FreeBSD.ORG Thu Feb 16 22:26:07 2012 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BA00A106566C; Thu, 16 Feb 2012 22:26:07 +0000 (UTC) (envelope-from giovanni.trematerra@gmail.com) Received: from mail-qw0-f54.google.com (mail-qw0-f54.google.com [209.85.216.54]) by mx1.freebsd.org (Postfix) with ESMTP id 073778FC13; Thu, 16 Feb 2012 22:26:06 +0000 (UTC) Received: by qaea17 with SMTP id a17so3319464qae.13 for ; Thu, 16 Feb 2012 14:26:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=F43XVx7cE3vZLueyPbiqpK5qghCe6KLrgfH+dKEY8Hw=; b=qXpVkYh7CX2BfZRwPhVnG4tDgT4ULXrm2xZ8pGeqVuh5bI1NF3Cehu7OzwCPyqkR9l RhrSCmkWA3Y86fE8+g/XP7po03gqORWV3+Jzu5psK/CwSQAsMue4IxbghcN5e16JoOJ/ BuvvEsVN6kKWWoRVZ9ubvdZRVGsTPw0mCxxG8= MIME-Version: 1.0 Received: by 10.229.135.85 with SMTP id m21mr2980771qct.26.1329431166247; Thu, 16 Feb 2012 14:26:06 -0800 (PST) Sender: giovanni.trematerra@gmail.com Received: by 10.229.214.139 with HTTP; Thu, 16 Feb 2012 14:26:06 -0800 (PST) In-Reply-To: <20120217015048.H3388@besplex.bde.org> References: <20120208151251.J1853@besplex.bde.org> <20120217015048.H3388@besplex.bde.org> Date: Thu, 16 Feb 2012 23:26:06 +0100 X-Google-Sender-Auth: JOoNqNQbsCPshaB5fWkF73LBH0Y Message-ID: From: Giovanni Trematerra To: freebsd-arch@freebsd.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: Kip Macy , Attilio Rao , Konstantin Belousov , jilles@freebsd.org Subject: Re: [LAST ROUND] fifo/pipe merge code X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Feb 2012 22:26:07 -0000 On Thu, Feb 16, 2012 at 4:59 PM, Bruce Evans wrote: > On Wed, 8 Feb 2012, Giovanni Trematerra wrote: > > Sorry for the delay. No problem, thanks to you for your time. > It looks very good now. =A0I only see very minor style bugs :-). =A0I > didn't think about its correctness again. Hopefully this is the last version that fix the remaining style bugs: http://www.trematerra.net/patches/pipefifo_merge.4.5.patch I would appreciate it if someone were to step up and commit this patch. Thank you. Below some comments of mine. > > diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c > index 94a2713..729e49c 100644 > --- a/sys/fs/fifofs/fifo_vnops.c > +++ b/sys/fs/fifofs/fifo_vnops.c > % ... > % =A0/* > % =A0 * This structure is associated with the FIFO vnode and stores > % =A0 * the state associated with the FIFO. > % =A0 * Notes about locking: > % - * =A0 - fi_readsock and fi_writesock are invariant since init time. > % + * =A0 - fi_pipe is invariant since init time. > % =A0 * =A0 - fi_readers and fi_writers are vnode lock protected. > % - * =A0 - fi_wgen is fif_mtx lock protected. > % + * =A0 - fi_wgen is PIPE_MTX lock protected. > > Can you find a better name than PIPE_MTX here and in a comment later? > PIPE_MTX is the name of the macro that is supposed to hide the details > of the lock, starting with the lock's name. =A0Using it in comments is > like unhiding the details. =A0Mutexes aren't the same as locks, and vnode= .h is > more careful to distinguish them. =A0I adapted the following > better wording from vnode.h. > > =A0* =A0 - fi_readers and fi_writers are protected by the vnode lock. > =A0* =A0 - fi_wgen is protected by the pipe mutex. I used this wording. Thanks for the suggestion. > > 2 places in sys_pipe.c fail to use PIPE_MTX() to hide the details. > I'm going to fix this and other style bugs not related to my patch with a different one. > > % ... > % diff --git a/sys/kern/sys_pipe.c b/sys/kern/sys_pipe.c > % index 9edcb74..4a1d8f3 100644 > > % --- a/sys/kern/sys_pipe.c > % +++ b/sys/kern/sys_pipe.c > % =A0/* > % =A0 * Use this define if you want to disable *fancy* VM things. =A0Expe= ct an > % =A0 * approx 30% decrease in transfer rate. =A0This could be useful for > % @@ -1329,29 +1397,28 @@ pipe_poll(fp, events, active_cred, td) > % =A0 =A0 =A0 struct pipe *rpipe =3D fp->f_data; > % =A0 =A0 =A0 struct pipe *wpipe; > % =A0 =A0 =A0 int revents =3D 0; > % + =A0 =A0 int levents; > > Variables with the same type should be declared on 1 line if possible, > not as for rpipe and wpipe above. =A0Initializations in declarations can > make this hard to read, but are style bugs. > > Variables with the same type should be sorted. fixed. > > Otherwise, this part of the patch is now readable :-). The pipe_poll part of the patch should be readable now. I hope :) > > % ... > % @@ -1625,7 +1708,7 @@ filt_pipedetach(struct knote *kn) > % =A0static int > % =A0filt_piperead(struct knote *kn, long hint) > % =A0{ > % - =A0 =A0 struct pipe *rpipe =3D kn->kn_fp->f_data; > % + =A0 =A0 struct pipe *rpipe =3D (struct pipe *)kn->kn_hook; > > This cast shouldn't be added here or elsewhere. =A0kn_hook has type `void= *', > so this cast is not needed in C, unlike in C++. > > % =A0 =A0 =A0 struct pipe *wpipe =3D rpipe->pipe_peer; > % =A0 =A0 =A0 int ret; fixed. > > This file mostly has the consistently non-KNF style of initializing both > rpipe and wpipe in their declarations. > I'll try to fix with a different patch. -- Gianni