Date: Thu, 30 Nov 2000 20:24:14 -0500 (EST) From: Andrew Gallatin <gallatin@cs.duke.edu> To: Bosko Milekic <bmilekic@technokratis.com> Cc: "Kenneth D. Merry" <ken@kdm.org>, arch@FreeBSD.ORG Subject: Re: zero copy code review Message-ID: <14886.63486.157224.937225@grasshopper.cs.duke.edu> In-Reply-To: <Pine.BSF.4.21.0011301657500.78741-100000@jehovah.technokratis.com> References: <20001129231653.A1503@panzer.kdm.org> <Pine.BSF.4.21.0011301657500.78741-100000@jehovah.technokratis.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?14886.63486.157224.937225>