[parisc-linux] [patch 2/2] backport of sba sg list management to ccio-dma

[Joel Soete]

> On Thu, Oct 25, 2007 at 09:13:18AM +0200, Joel Soete wrote:
> ...
> > I attached 2 test files (GetOrder-o.c using the original kernel get_iovp_order
> >  routine and GetOrder-f.c using supposed faster release) and here are some
> > results:
> ...
> > I didn't check in more details the difference of results between not-optimized
> > and optimized (-O2) compile but it's clear it's faster ;-)
> 
> Thanks for running those...I didn't expect it to be 40x faster (in the
> first pair of runs).
> 
> > (why not push this in upstream? [I mean ./include/asm-generic/page.h])
> 
> Erm, push what?
Sorry for shortcut, here is more detailed idea:

get_iovp_order came to me when reading the ia64/hp sba implementation where
you can read:
arch/ia64/hp/common/sba_iommu.c:
/**
 * For most cases the normal get_order is sufficient, however it limits us
 * to PAGE_SIZE being the minimum mapping alignment and TC flush granularity.
 * It only incurs about 1 clock cycle to use this one with the static variable
 * and makes the code more intuitive.
 */
static SBA_INLINE int
get_iovp_order (unsigned long size)
[snip]

but it's ia64 and I look for something I could test on parisc and this generic
code looked to me ideal:

include/asm-generic/page.h

/* Pure 2^n version of get_order */
static __inline__ __attribute_const__ int get_order(unsigned long size)
{
        int order;

        size = (size - 1) >> (PAGE_SHIFT - 1);
        order = -1;
        do {
                size >>= 1;
                order++;
        } while (size);
        return order;
}

just want to change PAGE_SHIFT with IOVP_SHIFT to make the drill ;-)
(this was this first idea)
static CCIO_INLINE int
get_iovp_order(unsigned long size)
{ 
	int order;

	size = (size - 1) >> (IOVP_SHIFT - 1);
	order = -1;
	do {
		size >>= 1;
		order++;
	} while (size);
	return order;
}

which make you though to ffs() which in turn make me proposed following:
static CCIO_INLINE int
get_iovp_order(unsigned long size)
{
        return fls((size - 1) >> (IOVP_SHIFT));
} 

(test case comparison loop shows well it's the same even for size=0)

I did some test with different values of IOVP_SHIFT and even with this case
IOVP_SHIFT==PAGE_SHIFT which is to me get_order().

static __inline__ __attribute_const__ int get_order(unsigned long size)
{
        return fls((size - 1) >> (PAGE_SHIFT));
} 

or imho even better

	return (size ? fls((size - 1) >> (PAGE_SHIFT)): 0);

Is it a bit more clear?
(I just hope you will not ask me to make the mathematical demonstration ;-))

What's your opinion?

> The code I was referring to is parisc assembly.
> That doesn't belong in asm-generic.
> Or did you somehow overlook that?
> 
> ...
> > > I'm not sure we have to handle that case...or if we do, make sure it's
> > handled correctly in get_iovp_order().
> > >
> > I just hope so that linux check to never call get_order(0) ;-)
> 
> "hope?" Bad idea. :)
> Please add an ASSERT or verify ccio can never invokes get_order(0).
> 
Yep or apply same idea as above but for get_iovp_order (i.e. return (size ?
fls((size - 1) >> (IOVP_SHIFT)): 0)

(Test doesn't show any lost of perf ;-)

> thanks,
> grant
> 
> 
All additional comment will be welcome.

Tx,
    J.

---
Pack Scarlet One, ADSL 6 Mbps + Telephonie, a partir de EUR 29,95...
http://www.scarlet.be/