Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Matthew Wilcox <matthew@wil.cx>]

On Sat, Nov 08, 2008 at 11:37:45AM -0700, Grant Grundler wrote:
> On Sat, Nov 08, 2008 at 07:10:22AM -0700, Matthew Wilcox wrote:
> > It's certainly true (I'd noticed it too).  What I hadn't decided was
> > whether to take out the readl() or take out the msix_flush_writes().
> > I'm conflicted.
> 
> Good, I'm not. :)
> I'd much rather remove msix_flush_writes(). It's clearer to me to have the
> readl() next to the writel() than to bury the readl() a function call later.

That's certainly a respectable opinion.  It's one I hold myself.  Have
you considered another opinion that if you're trying to disable a
thousand or more interrupts, doing writel/readl() a thousand times is
going to be less efficient than doing writel() a thousand times and
readl() once?  We don't really have a need to do that today, but it's a
possible direction we might want to move in the future.

> Thanks - You are right. I expected (1U<<32) to be zero (arithmetic shift left)
> instead of 1 ("Rotate Left" aka "logical shift left"). Code below produced:

Er, rotate left is not logical shift left.  Also, shifting by >= the number of
bits in the element is undefined by the C spec.  Which is why Jan broke
it into two shifts.

>      5 32 0x1 0x0
> 
> when using "1U << k". Here's the test code:
> 
> grundler@mb500:~$ cat test_shift.c
> #include <stdio.h>
> main()
> {
> 	unsigned int i;
> 	for (i=0;i<6; i++) {
> 		unsigned int k = 1U << i ;
> 		unsigned int j = 1ULL << k ;
> 		printf("%d %d 0x%x 0x%x\n", i, k, j, j - 1);
> 	}
>         return 0;
> }

This will give different results on different processors, and possibly
even with different compilers.  It is entitled to emit nasal daemons.

> Using "1ULL" (64-bit constant) is enough to get the result I was
> expecting.... note I didn't declare j as "unsigned long long j"
> because gcc didn't warn about assigning "1ULL < k" (64-bit) to an
> unsigned int (32-bit). gcc bug?

Not a bug.  That kind of truncation isn't generally warned about.
Consider:

void foo(char *x, int bar)
{
	x[0] = bar >> 24;
	x[1] = bar >> 16;
	x[2] = bar >> 8;
	x[3] = bar;
}

> I think the kernel should declare maskbits as 64-bit and
> use "1ULL" to just keep things obvious.

I suspec that's rather inefficient.

> > unsigned int msi_mask[] = {
> > 	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
> > };
> 
> The code, when written correctly, seems compact and simple enough.

I'm not sure I agree.

This is all rather moot since multiple MSI isn't supported, and my
patchset deletes the function in question anyway.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."