Date: Thu, 16 Feb 2012 23:26:06 +0100 From: Giovanni Trematerra <gianni@freebsd.org> To: freebsd-arch@freebsd.org Cc: Kip Macy <kmacy@freebsd.org>, Attilio Rao <attilio@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, jilles@freebsd.org Subject: Re: [LAST ROUND] fifo/pipe merge code Message-ID: <CACfq092qAEF=dtJzwnrv%2BbG13x_KmuZTLBqiZJQb5Kk1KY2FGA@mail.gmail.com> In-Reply-To: <20120217015048.H3388@besplex.bde.org> References: <CACfq090%2BEDVjj_rRNyafiod_BBzGUzmQ9CjApZLcXWR-Ax=0uQ@mail.gmail.com> <20120208151251.J1853@besplex.bde.org> <CACfq091eHpvCbwuU-WOxiyt92YiTbRbWnPmNGGMDidzG=uq4uw@mail.gmail.com> <20120217015048.H3388@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Feb 16, 2012 at 4:59 PM, Bruce Evans <brde@optusnet.com.au> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACfq092qAEF=dtJzwnrv%2BbG13x_KmuZTLBqiZJQb5Kk1KY2FGA>