From owner-freebsd-hackers Mon Aug 20 22:51:11 2001 Delivered-To: freebsd-hackers@freebsd.org Received: from beppo.feral.com (beppo.feral.com [192.67.166.79]) by hub.freebsd.org (Postfix) with ESMTP id 7432437B408; Mon, 20 Aug 2001 22:50:54 -0700 (PDT) (envelope-from mjacob@feral.com) Received: from wonky.feral.com (wonky.feral.com [192.67.166.7]) by beppo.feral.com (8.11.3/8.11.3) with ESMTP id f7L5orI71797; Mon, 20 Aug 2001 22:50:54 -0700 (PDT) (envelope-from mjacob@feral.com) Date: Mon, 20 Aug 2001 22:50:38 -0700 (PDT) From: Matthew Jacob Reply-To: To: Bill Paul Cc: , Subject: Re: Where to put new bus_dmamap_load_mbuf() code In-Reply-To: <20010821000630.92CF937B406@hub.freebsd.org> Message-ID: <20010820223007.I39850-100000@wonky.feral.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-hackers@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Mon, 20 Aug 2001, Bill Paul wrote: > Every hear the phrase "you get what you pay for?" The API isn't all that > clear, and we don't have a man page or document that describes in detail > how to use it properly. Rather than whining about that, I decided to > tinker with it and Use The Source, Luke (tm). This is the result. Well, I'm more familiar with the NetBSD BusDma code, which is similar, and heavily documented. I'm also one of the principle authors of the Solaris DKI/DDI, which is *also* heavily documented, so I have some small notion of how a few of these subsystems are supposed to work, and where documentation exists for these- and similar systems (e.g., UDI). > > My understanding is that you need a dmamap for every buffer that you want > to map into bus space. Each mbuf has a single data buffer associated with > it (either the data area in the mbuf itself, or external storage). We're > not allowed to make assumptions about where these buffers are. Also, a > single ethernet frame can be fragmented across multiple mbufs in a list. > > So unless I'm mistaken, for each mbuf in an mbuf list, what we > have to do is this: > > - create a bus_dmamap_t for the data area in the mbuf using > bus_dmamap_create() > - do the physical to bus mapping with bus_dmamap_load() > - call bus_dmamap_sync() as needed (might handle copying if bounce > buffers are required) > - > - do post-DMA sync as needed (again, might require bounce copying) > - call bus_dmamap_unload() to un-do the bus mapping (which might free > bounce buffers if some were allocated by bus_dmamap_load()) > - destroy the bus_dmamap_t > > One memory region, one DMA map. It seems to me that you can't use a > single dmamap for multiple memory buffers, unless you make certain > assumptions about where in physical memory those buffers reside, and > I thought the idea of busdma was to provide a consistent, opaque API > so that you would not have to make any assumptions. > > Now if I've gotten any of this wrong, please tell me how I should be > doing it. Remember to show all work. I don't give partial credit, nor > do I grade on a curve. This is fine insofar as it goes, but there's nothing, I believe, that requires you to *create* a bus_dmamap_t each time you wish to map something and then destroy it when you unmap something. You might ask why one actually has the separate step from map creation and map load at all then. All the rest of the stuff for load/sync/sync/unload is fine. Using The Code (tm)- you can see that, for example, you can create a tag that describes all of the the addressable space your device can access, e.g.: if (bus_dma_tag_create(pci->parent_dmat, PAGE_SIZE, lim, BUS_SPACE_MAXADDR, BUS_SPACE_MAXADDR, NULL, NULL, len, 1, BUS_SPACE_MAXSIZE_32BIT, 0, &pci->cntrol_dmat) != 0) { isp_prt(isp, ISP_LOGERR, "cannot create a dma tag for control spaces"); free(isp->isp_xflist, M_DEVBUF); free(pci->dmaps, M_DEVBUF); return (1); } Then, for each possible transaction slot- if you have a device that has a fixed number of transactions that are possible (as many do), you can create maps ahead of time: for (i = 0; i < isp->isp_maxcmds; i++) { error = bus_dmamap_create(pci->parent_dmat, 0, &pci->dmaps[i]); if (error) { ... so that for each transaction that needs to be mapped, you can dma load it: bus_dmamap_t *dp; ... dp = &pci->dmaps[isp_handle_index(rq->req_handle)]; ... s = splsoftvm(); error = bus_dmamap_load(pci->parent_dmat, *dp, csio->data_ptr, csio->dxfer_len, eptr, mp, 0); ... which as part of the load process can sync it: dp = &pci->dmaps[isp_handle_index(rq->req_handle)]; if ((csio->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_IN) { bus_dmamap_sync(pci->parent_dmat, *dp, BUS_DMASYNC_PREREAD); } else { bus_dmamap_sync(pci->parent_dmat, *dp, BUS_DMASYNC_PREWRITE); } and when the transaction is done, you can sync and unload: static void isp_pci_dmateardown(struct ispsoftc *isp, XS_T *xs, u_int16_t handle) { struct isp_pcisoftc *pci = (struct isp_pcisoftc *)isp; bus_dmamap_t *dp = &pci->dmaps[isp_handle_index(handle)]; if ((xs->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_IN) { bus_dmamap_sync(pci->parent_dmat, *dp, BUS_DMASYNC_POSTREAD); } else { bus_dmamap_sync(pci->parent_dmat, *dp, BUS_DMASYNC_POSTWRITE); } bus_dmamap_unload(pci->parent_dmat, *dp); } ------------ So- my question still stands- from a performance point of view (Networking people *do* care about performance I believe, yes? :-))- if you don't need to create the map each time, wouldn't you rather not? So, the mbuf mapping code, which is cool to have, really might not need this? > > > The current suggestion is fine except that each platform might have a more > > > efficient, or even required, actual h/w mechanism for mapping mbufs. > > It might, but right now, it doesn't. All I have to work with is the > existing API. I'm not here to stick my fingers in it and change it all > around. I just want to add a bit of code on top of it so that I don't > have to go through quite so many contortions when I use the API in > network adapter drivers. This is not on point. This wasn't an API issue. The existing implementation is in fact maintained as platform specific code. I'm gently suggesting that you consider following suit because there may in fact *be* platform specifics in the future. Or don't. There aren't really any right now- so perhaps I'm overanticipating issues down the line where we have to atticize this file because we had to move all the functions to platform files. > > > > I'd also be a little concerned with the way you're overloading stuff > > > into mbuf.... > > I thought about this. Like it says in the comments, at the device driver > level, you're almost never going to be using some of the pointers in the > mbuf header. On the RX side, *we* (i.e. the driver) are allocating the > mbufs, so we can do whatever the heck we want with them until such time > as we hand them off to ether_input(), and by then we will have put things > back the way they were. For the TX side, by the time we get the mbufs > off the send queue, we always know we're going to have just an mbuf list > (and not an mbuf chain), and we're going to toss the mbufs once we're done > with them, so we can trample on certain things that we know don't matter > to the OS or network stack anymore. > > The alternatives are: > > - Allocate some extra space in the DMA descriptor structures for the > necessary bus_dmamap_t pointers. This is tricky with this particular > NIC, and a little awkward. > - Allocate my own private arrays of bus_dmamap_t that mirror the DMA > rings. This is yet more memory I need to allocate and free at device > attach and detach time. > > I've got space in the mbuf header. It's not being used. It's right > where I need it. Why not take advantage of it? Well- I'll really defer to you on this. Like I said- I'm shakier about it. Why don't we just allocate space in the pkthdr- or while we're at it, have a per-device pool allocator/free/pullup/dma_map vector as part of the pkthdr anyway- that might be of more long term interest anyway, no (e.g., for large MTU allocations so that pkts can be pooled per device- might as well do the DMA goop as part of this while you're at it). > > > Finally- why not make this an inline? > > Er... because that idea offended my delicate sensibilities? :) Stiffen your lip, and think of England. Stout lad! -matt To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-hackers" in the body of the message