On Sat, Nov 08, 2008 at 07:13:23PM -0700, Matthew Wilcox wrote:
> 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.
I hadn't considered this case and agree that moving the MMIO readl
"out of the loop" would make it more efficient. But I expect we can
implement that when it becomes a problem.
BTW, I worry the solution won't be as simple as it sounds since
pushing a thousand MMIO writes to a chip will result in the MMIO writes
backing up all the write queues. ie they probably shouldn't be done
while in the interrupt context anyway.
Secondly, if these writes have to go to a thousand different devices,
we would have to flush each one anyway. If we have 2K MSI for one
device and update them all at once, it's going to be a significant
hiccup no matter how we do it. I certainly hope this is not going
to be part of anyone's performance code path (e.g. RX or TX).
> > 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.
Sorry - brainfart. "logical shift left" is the same as "arithmetic 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.
Ok.
...
> 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;
> }
*nod* - the shift is going in the opposite direction in this example.
But I'll agree warning on this would break quite a few compiles.
> > I think the kernel should declare maskbits as 64-bit and
> > use "1ULL" to just keep things obvious.
>
> I suspect that's rather inefficient.
It's slightly less efficient. But it's alot clearer and I don't
have the impression this is performance sensitive code (not like
TLB miss handlers, syscall entry, or DMA mapping code).
> > > 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.
ok
thanks,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
