Date: Mon, 09 Nov 2015 12:19:13 -0700 From: Ian Lepore <ian@freebsd.org> To: Roger Pau =?ISO-8859-1?Q?Monn=E9?= <royger@FreeBSD.org>, Hans Petter Selasky <hps@selasky.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r290610 - head/sys/x86/x86 Message-ID: <1447096753.91534.514.camel@freebsd.org> In-Reply-To: <5640BEBE.3070805@FreeBSD.org> References: <201511091219.tA9CJwe7067036@repo.freebsd.org> <564091FF.8090605@selasky.org> <5640BEBE.3070805@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 2015-11-09 at 16:41 +0100, Roger Pau Monné wrote:
> Hello,
>
> El 09/11/15 a les 13.30, Hans Petter Selasky ha escrit:
> > On 11/09/15 13:19, Roger Pau Monné wrote:
> > > + if (dmat->common.flags & BUS_DMA_KEEP_PG_OFFSET) {
> > > + /*
> > > + * If we have to keep the offset of each page this
> > > function
> > > + * is not suitable, switch back to
> > > bus_dmamap_load_ma_triv
> > > + * which is going to do the right thing in this case.
> > > + */
> > > + error = bus_dmamap_load_ma_triv(dmat, map, ma, buflen,
> > > ma_offs,
> > > + flags, segs, segp);
> > > + return (error);
> > > + }
> >
> > Hi,
> >
> > There has been an update made to the USB stack, which is currently
> > the
> > only client of "BUS_DMA_KEEP_PG_OFFSET", which means this check can
>
> The only in-tree client. We don't know if there are other clients out
> of
> the tree.
>
The BUS_DMA_KEEP_PG_OFFSET flag is not documented anywhere except in a
short coment block near its #define which is less than 100% rigorous in
specifying exactly what the flag even means. For example it is
documented as being a "hint" but also includes the word "must". It
doesn't say whether the flag is to be used to create a tag, to create a
map, or to load a buffer. It says the offset must be kept the same in
the first segment but doesn't really fully explain what the USB
requirements are (and the flag was added specifically for USB).
To me, all of that adds up to freedom to clarify the meaning of the
flag in both code and documentation without a lot of worrying about out
of tree code. And the mips and arm busdma implementations are soon
going to leverage that freedom into much better implementations based
on the new understanding of what that flag really requires.
> > probably be skipped or relaxed a bit. The condition which must be
> > fullfilled is:
>
> So you basically want a contiguous bounce buffer. I don't think we
> can
> just change BUS_DMA_KEEP_PG_OFFSET to mean "use a contiguous bounce
> buffer". Maybe a new flag could be introduced to describe this new
> requirement and the old one deprecated.
>
It's not "I want a contiguous buffer". It is "I want contiguous
offsets when transitioning between (potentially non-continguous)
pages." That grants you the freedom to bounce a couple different ways
depending on what's most efficient for the platform and the situation.
For example, you can maintain the offset-within-page in the first
segment and allow the offset in all subsequent pages (bounced or not)
to be zero. That's what all current implementations are doing, but it
can require two full bounce pages to handle a 2-byte transfer that
happens to be split across two pages (and yes that happens; it's not
even rare in USB, as lots of DMA is done specifying buffers and
variables on kernel stacks or in softc member variables).
It also allows for the possibility of changing the offset in the first
segment if doing so avoids any page crossings at all (which handles
everything from the 2-byte worst case to a 4096-byte buffer).
> > #ifdef USB_DEBUG
> > if (nseg > 1 &&
> > ((segs->ds_addr + segs->ds_len) & (USB_PAGE_SIZE - 1))
> > !=
> > ((segs + 1)->ds_addr & (USB_PAGE_SIZE - 1))) {
> > /*
> > * This check verifies there is no page offset hole
> > * between the first and second segment. See the
> > * BUS_DMA_KEEP_PG_OFFSET flag.
> > */
> > DPRINTFN(0, "Page offset was not preserved\n");
> > error = 1;
> > goto done;
> > }
> > #endif
>
> AFAICT with the current bounce implementation on x86 you would have
> to
> specify an alignment of PAGE_SIZE in order to have this guarantee
> without specifying BUS_DMA_KEEP_PG_OFFSET.
>
> IMHO, we should change all the current bounce buffer code and switch
> to
> use memdescs for everything (ie: bios and mbufs should use a memdesc
> internally). Then each arch should provide functions to copy from the
> different kinds of memdescs (either memdescs containing physical or
> virtual memory), so the bounce code could be unified between all
> arches.
> Of course that's easier said than done...
>
I completely disagree with this, mostly because I'm halfway through a
set of changes that will make arm and mips bounce handling look
completely different than x86 or other platforms. That reflects the
differences in underlying hardware that leads to completely different
reasons for bouncing in the first place.
x86 code bounces DMA when there are exclusion regions in the physical
address space where DMA cannot occur. On ARM and MIPS that situation
just basically never occurs (except maybe on a few ancient arm xscale
systems), and virtually all bouncing is due to DMA buffers not being on
cache line boundaries. Because bio and mbuf DMA typically is cache
-aligned, that means that almost all bouncing is a handful of bytes at
a time, and the page-replacement bounce scheme that was copied from x86
just isn't efficient (in fact it wastes megabytes of memory in
preallocated bounce buffers that small-memory arm and mips systems
can't really afford to waste).
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1447096753.91534.514.camel>