Date: Tue, 10 Nov 2015 13:27:46 +0100 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= <royger@FreeBSD.org> To: Ian Lepore <ian@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: <5641E2C2.8020009@FreeBSD.org> In-Reply-To: <1447096753.91534.514.camel@freebsd.org> References: <201511091219.tA9CJwe7067036@repo.freebsd.org> <564091FF.8090605@selasky.org> <5640BEBE.3070805@FreeBSD.org> <1447096753.91534.514.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
El 09/11/15 a les 20.19, Ian Lepore ha escrit:
> 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.
Maybe rename the flag to BUS_DMA_CONTIGUOUS_PG_OFFSET?
>>> 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).
Yes, that's what I've now tried to do in x86:
https://reviews.freebsd.org/D4119
Change the offset in the first segment and then making sure everything
is contiguous in terms of offsets.
>>> #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).
I see, I guess you are right that other arches have very different
requirements in terms of bouncing, and that unifying them wouldn't work
that well. I got that conclusion after looking at some of the busdma
implementations for other arches which right now look quite similar,
didn't know there was a major rework planned.
Roger.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5641E2C2.8020009>