Date: Fri, 1 Dec 2000 17:51: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, alfred@FreeBSD.ORG Subject: Re: zero copy code review Message-ID: <14888.9802.415926.434956@grasshopper.cs.duke.edu> In-Reply-To: <Pine.BSF.4.21.0011302159210.79831-100000@jehovah.technokratis.com> References: <14886.63486.157224.937225@grasshopper.cs.duke.edu> <Pine.BSF.4.21.0011302159210.79831-100000@jehovah.technokratis.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?14888.9802.415926.434956>