Date: Mon, 20 Aug 2001 22:50:38 -0700 (PDT) From: Matthew Jacob <mjacob@feral.com> To: Bill Paul <wpaul@FreeBSD.ORG> Cc: <hackers@FreeBSD.ORG>, <current@FreeBSD.ORG> Subject: Re: Where to put new bus_dmamap_load_mbuf() code Message-ID: <20010820223007.I39850-100000@wonky.feral.com> In-Reply-To: <20010821000630.92CF937B406@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
> - <insert mysterious DMA operation here>
> - 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010820223007.I39850-100000>