Date: Fri, 1 Dec 2000 00:16:19 -0700 From: "Kenneth D. Merry" <ken@kdm.org> To: Bosko Milekic <bmilekic@technokratis.com> Cc: arch@FreeBSD.ORG, gallatin@FreeBSD.ORG Subject: Re: zero copy code review Message-ID: <20001201001619.C10772@panzer.kdm.org> In-Reply-To: <Pine.BSF.4.21.0011301657500.78741-100000@jehovah.technokratis.com>; from bmilekic@technokratis.com on Thu, Nov 30, 2000 at 06:00:38PM -0500 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
[ Drew answered some of this, I'll try to answer the rest. ] Thanks for looking at the code! On Thu, Nov 30, 2000 at 18:00:38 -0500, Bosko Milekic wrote: [ ... ] [ Drew answered this part ] > - In the actual MEXTADD(), you don't seem to be passing the M_RDONLY > flag (which is done for sendfile buffer ext mbufs). M_RDONLY is used to > indicate to the rest of the code that the m_data is not to be tampered > with (trimmed, et al) -- in other words, it's read-only. Have you > considered it? That was an oversight, I'll add the flag. (The places where it is used are in uipc_cow.c, if_ti.c and nfs_serv.c.) > - Stylistic suggestion: please try to keep things 25x80. :-) I try, and I think most of the changes are, except for the NFS stuff. I didn't reformat that, although I suppose I could. (It irritates me, too.) > [ skipped all the other NFS + ti driver changes ] > > jumbo.h: > I would like to eventually split the cluster code out of mbuf.h and > uipc_mbuf.c and change jumbo.h/uipc_jumbo.c -> cluster.h/uipc_cluster.c > > mbuf.h: > - Make EXT_DISPOSABLE 3, instead of 300... if you decide to keep it. > The reason I say this is because it seems to me that EXT_DISPOSABLE should > be more of an m_flag than an ext_type, which would probably mean that we > should make m_flag bigger than a short (which it is now). The reason I > argue this is because EXT_DISPOSABLE seems to be more of an indication of > what should be done with the contents of the mbuf. Perhaps what needs to > be done instead is make the EXT_DISPOSABLE flag, have if_ti use the DRV > ext type (like it should be doing) for its external buffers, and make it > set EXT_DISPOSABLE|M_RDONLY during the MEXTADD. Let's not get too strict > with this for now, though, it would be better to make sure everything is > working perfectly until we decide what to do with this - and it can be > changed easily later. In its current incarnation, EXT_DISPOSABLE indicates that the the memory used in the mbuf can be disposed of -- i.e. removed from the kernel's virtual address map. The contents aren't disposed of, they're just moved elsewhere. I don't think most of the rest of the mbuf code is setup to deal with the memory inside a non-external mbuf going away. (Which would be the potential implication of having EXT_DISPOSABLE be a regular m_flag.) > tiio.h: Are you sure tiio.h belongs in src/sys/sys ? Well, it defines the interface for the character device front end for the ti(4) driver. Usually ioctls and supporting structures go in sys/sys. Would you suggest another location? > Also, have you checked whether any locking should be performed here? > Considering that this is all supposed to improve performance, it would be > nice if it didn't all need to run under Giant. I realize that some of this > will have to wait (i.e. VM), but what about the if_ti code? Is that > something that can be looked at RSN? When Bill converted the ti(4) driver from spls to mutexes, I did the same conversion on my modifications to the driver. Is that sufficient? I'm not terribly up-to-date on the mutex stuff. As for the rest of the code, since it was written pre-mutex, it still has the spls in the right places. I suppose that they would just need to be converted to mutexes. (Or is that an overly simplistic way to look at it? :) > 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. Good idea, I'll do it if I have the time. :( > 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. Heh, well, the big chunk is the Tigon firmware. :) Are you suggesting just splitting the diffs out into multiple files, or actually breaking the changes up? The latter would be rather difficult to do, I think. In any case, the changes aren't on by default, so folks can just not turn them on if they run into problems. Thanks for the review, I'll try to incorporate your suggestions. Ken -- Kenneth Merry ken@kdm.org 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?20001201001619.C10772>