From owner-freebsd-arch Thu Nov 30 17:24:29 2000 Delivered-To: freebsd-arch@freebsd.org Received: from duke.cs.duke.edu (duke.cs.duke.edu [152.3.140.1]) by hub.freebsd.org (Postfix) with ESMTP id C024437B400 for ; Thu, 30 Nov 2000 17:24:25 -0800 (PST) Received: from grasshopper.cs.duke.edu (grasshopper.cs.duke.edu [152.3.145.30]) by duke.cs.duke.edu (8.9.3/8.9.3) with ESMTP id UAA02260; Thu, 30 Nov 2000 20:24:15 -0500 (EST) Received: (from gallatin@localhost) by grasshopper.cs.duke.edu (8.11.1/8.9.1) id eB11OF003769; Thu, 30 Nov 2000 20:24:15 -0500 (EST) (envelope-from gallatin@cs.duke.edu) From: Andrew Gallatin MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Date: Thu, 30 Nov 2000 20:24:14 -0500 (EST) To: Bosko Milekic Cc: "Kenneth D. Merry" , arch@FreeBSD.ORG Subject: Re: zero copy code review In-Reply-To: References: <20001129231653.A1503@panzer.kdm.org> X-Mailer: VM 6.43 under 20.4 "Emerald" XEmacs Lucid Message-ID: <14886.63486.157224.937225@grasshopper.cs.duke.edu> Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Bosko, Thanks for your comments. I'm a little disconnected from the code these days, as I do most of my development in a 4.0-RELEASE environment. (Ken ported my contributions forward). Bosko Milekic writes: > > In general, I am pro-the zero copy stuff you've been > gathering/merging/updating/writing/etc. over the past several months. > Looking at the sendfile portion of your changes, it's pretty obvious that > they are very minimal, but I'm curious as to why you've bothered removing > the "static" before the sf_buf_free(). I can see why it really has no > significance in the sf_buf_alloc() case, but sf_buf_free() is attached to > the mbuf's m_ext free function pointer (I'm really just curious if the > motivation was strictly stylistic). It was un-staticized because it is called by socow_iodone(), which is the m_ext free for zero-copy transmissions. > Here some other notes, which I came across during a real quick read > of some of the code (I am sort of in a pre-final-exam period, so I can't > dedicate too much time to this for the next 2 weeks, about :-( ): > > in nfs/nfs_serv.c: > In your first "BEGIN SUSPECT REGION" block: > > - You allocate an sf_buf somewhere down the line and then attempt to > allocate an mbuf to which you will hope to attach the sf_buf to. If the > mbuf allocation fails, you don't seem to free the sf_buf anywhere and > consequently, it looks as though you may leak sf_bufs. But the mbuf is allocated using M_WAIT. Can that fail? I haven't kept up with the mbuf changes in -current. > - You only m_freem() on mb (the header mbuf) if mb->m_next != NULL, > but if there is no m_next (m_next == NULL), you don't seem to free the mb > mbuf (header mbuf) at all. Is this meant to be this way? (Note that it > may very well be, I haven't looked at all the other surrounding code, > just making sure). Yes. Like most of the NFS code, it is a little convoluted.. mb is a pre-existing mbuf chain that we're attaching mbufs to. In the failure case (where the mfreem I think you're talking about is), we backout what we've done by freeing the mbufs we've added to mb, return mb->next to null, and continue in the normal (copy) path. <... some helpful comments deleted ....> Many of your comments are directly related to -current, I think I'll let Ken address them... > I would strongly urge you to run some tests under real heavy network > activity (possibly lower NMBCLUSTERS for this) and starve out the mbuf > resources and see if anything strange happens - you may catch a couple of > leaks that may have accidently slipped through. Finally, I'd like to > suggest possibly breaking up some of the diff to smaller chunks, just so > it is easier to track things down if something does break. With -CURRENT > changing relatively dramatically now sometimes several times in a single > day, I think this would be worth it for everybody. FWIW, the client-side nfs changes (in their 4.0-RELEASE form) are in daily use in our lab and have been for months. We run experiments with 8 clients running against our Slice cluster nfs file server. Each client is close to maxed-out (60-70MB/sec per client, typically) for hours... ;) Thank you for your feedback. And thank you for impoving the mbuf system so much. I wasted a whole afternoon yesterday doing something which I could have done in 5 minutes if only I had mext_refcnt in 4.0 ;) Drew To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message