Re: [PATCH 2/3] pci: Clamp pcie_set_readrq() when using "performance" settings

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2011-10-04 at 09:51 -0700, Linus Torvalds wrote:
> On Tue, Oct 4, 2011 at 9:08 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >>
> >> Well, bcrl argues that patches 1-2 of 3 are actively wrong.
> >
> > This is an argument that isn't finished :-)
> 
> Oh, I absolutely *hope* it isn't finished, but there's no way I'm
> applying a patch for -rc9 that people are still actively arguing
> whether it's at all valid or not.

Well, thing is, you -already- have the whole "performance" option which
is what we are 'arguing' about upstream. Except that the implementation
of it that you have in your tree now has very nasty bugs (ie it doesn't
do what it's supposed to and really doesn't work).

Patches 1 and 2 fix it to do what it's supposed to.

Whether that's a useful scheme or not is what Ben and I are discussing
and frankly the jury is still out as to whether it's beneficial or not
for the generic case. (It's basically an algorithm used today by pHyp on
power).

I wouldn't mind much if we just ripped it out, and left the other
options only at this stage.

> Which is why I'm planning on applying 3/3 just to make the whole issue
> irrelevant for 3.1, and then the people who want to test things out
> can apply whatever patches they want and play with the kernel command
> line options to actually enable whatever behavior they are testing.

As you like but I still think you should apply 1 and 2. As I said above,
the option is there already but the implementation is buggy. Let's at
least make it do what it's supposed to. It's still optional and will
still need testing and benchmarking but heh ...

Or the other option is to rip out all the "performance" case code.

I don't think it makes any sense to keep that option in it's current
non-working form. IE. The code right now does utterly wrong things and
generates something that cannot work if you chose that mode on your
kernel command line.

Cheers,
Ben.