Re: [patch 2/2] backport of sba sg list management to ccio-dma

On Fri, Oct 26, 2007 at 03:20:55PM +0200, Joel Soete wrote:
...
> > > (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]

Understood - ia64 has an instruction to implement FFS/FLS. PA-RISC does not.

> 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;
> }

Prove this code is faster than the __ffs asm in include/asm-parisc/bitops.h
and we can talk about it. Unless gcc has gotten phenomenally better, I'm
not inclined to believe the generic code will be faster.

> 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)

Sure - submit a patch for that. That sounds fine to me.


> 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));
> }

This looks fine to me.
I've not checked if all possible values map correctly.
Can you try (for n in 0-31), and verify (2^n) and (2^n)+1 values are the
same for the above and the original get_order() function?

> or imho even better
> 
> 	return (size ? fls((size - 1) >> (PAGE_SHIFT)): 0);

fls is inlined already and does the same test for size.
Adding another test here doesn't help.
The first one you posted is correct.

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

I don't need a mathematical demonstration...just a short test to demonstrate
(2^n) and (2^n+1) still produce the same results.

> What's your opinion?

In general, sounds good to me! :)

hth,
grant