From owner-freebsd-arch Fri Dec 1 14:51:19 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 7454137B400; Fri, 1 Dec 2000 14:51:16 -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 RAA23194; Fri, 1 Dec 2000 17:51:14 -0500 (EST) Received: (from gallatin@localhost) by grasshopper.cs.duke.edu (8.11.1/8.9.1) id eB1MpEp06117; Fri, 1 Dec 2000 17:51:14 -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: Fri, 1 Dec 2000 17:51:14 -0500 (EST) To: Bosko Milekic Cc: "Kenneth D. Merry" , arch@FreeBSD.ORG, alfred@FreeBSD.ORG Subject: Re: zero copy code review In-Reply-To: References: <14886.63486.157224.937225@grasshopper.cs.duke.edu> X-Mailer: VM 6.43 under 20.4 "Emerald" XEmacs Lucid Message-ID: <14888.9802.415926.434956@grasshopper.cs.duke.edu> Sender: owner-freebsd-arch@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG Bosko Milekic writes: > > It was un-staticized because it is called by socow_iodone(), which > > is the m_ext free for zero-copy transmissions. > > I see. But if the sendfile code still passes it as its own free > routine, then shouldn't it remain staticized, strictly speaking? Although > I may have missed it in the large diff, I did not see any changes to the > actual registering of sf_bufs in the actual sendfile code (i.e. > uipc_syscalls.c). I'm under the impression that in uipc_syscalls.c, the > MEXTADD which sets up an sf_buf with an mbuf still passes sf_buf_free as > its free routine. I'm still not sure I understand your objection. There's some code in socow_cowsetup() which uses sf bufs. Prior to allocating the sf_buf, it does some of its own fiddling with the page and introduces some state the sf_buf_free() wouldn't know how to clear. socow_iodone() undoes that fiddling and then calls sf_buf_free() to free the sfbuf. Isn't it better to call sf_buf_free() than to cut & paste the code? <...> > > But the mbuf is allocated using M_WAIT. Can that fail? I haven't > > kept up with the mbuf changes in -current. > > Yes, it can. M_WAIT just means "if nothing is available, first drain Eeek! I had no idea; I was thinking of it as blocking forever. This will have to be addressed. Thank you for pointing it out! > the stacks and if still nothing is available, then wait > kern.ipc.mbuf_wait ticks (sysctl) and if still nothing is available, fail > and set the passed in pointer to NULL and hope that the caller will deal > with it." Waiting indefinetly can be dangerous in certain situations (for > mbufs) but I won't get into that here. > In your code, you do deal with the possibility of the MGETHDR > returning NULL (you check for it) and you set ENOBUFS in that case and > jump to the "errorpath" label. But, before using MGETHDR, you allocate an > sf_buf (in sf) and it just so happens that the code beyond "errorpath" > does not take care of freeing the sf_buf you allocated before even > trying to allocate the mbuf. I see your point. This was copied, (bug for bug ;-), from sendfile itself. Look at line 1700 or so of kern/uipc_syscalls.c.. This bug should probaby be fixed there too.. > Another thing to note, especially if you are Pre-SMPng: sf_buf_alloc > calls can block, and even indeffinately (until the allocation is > succesfull). In sendfile(2), this doesn't matter as you're not allocating > the sf_buf from an interrupt. It has the potential to be a problem if you > start allocating sf_bufs from interrupt context. Unfortunately, I haven't > yet read+fully visualized all the code in the large diff, but this is > something to take into account when reviewing. The nfs sf_buf_alloc() calls will be made from either a process context (when doing a zero-copy send over a socket) or from the context of an nfsiod for the NFS code, so I think this should be safe. Thanks! Drew To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message