Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Logan Gunthorpe]

On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
> Hi Ben and Logan,
> 
> It looks like my git send-email has been not working correctly since I
> started trying to get these patches accepted. I may have remedied this
> now, but I have seen that Logan tried to find these patches and failed.
> So as a courtesy until I post PATCH v7 (hopefully correctly, this time),
> I am forwarding you the patches. I hope you like them. I would love to 
> know of any concerns or questions you may have, and / or what happens if 
> you test them. Thanks and all the best!
> 
> ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
> 
> Date: Thu, 23 May 2019 06:29:28 +0800
> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> To: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
> X-Mailer: git-send-email 2.19.1
> 
> Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF size
> for PCI hotplug bridges.

Makes sense.

> Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF size if
> hpmempref has been specified, rather than controlling both MMIO and
> MMIO_PREF sizes unconditionally.

I don't think I like that fact that hpmemsize behaves differently if
hpmempref size is specfied before it. I'd probably suggest having three
parameters: hpmemsize which sets both as it always has, a pref one and a
regular one which each set one of parameters.

Logan

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Nicholas Johnson]

On Wed, Jun 19, 2019 at 10:45:38AM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
> > Hi Ben and Logan,
> > 
> > It looks like my git send-email has been not working correctly since I
> > started trying to get these patches accepted. I may have remedied this
> > now, but I have seen that Logan tried to find these patches and failed.
> > So as a courtesy until I post PATCH v7 (hopefully correctly, this time),
> > I am forwarding you the patches. I hope you like them. I would love to 
> > know of any concerns or questions you may have, and / or what happens if 
> > you test them. Thanks and all the best!
> > 
> > ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
> > 
> > Date: Thu, 23 May 2019 06:29:28 +0800
> > From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > To: linux-kernel@vger.kernel.org
> > Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
> > X-Mailer: git-send-email 2.19.1
> > 
> > Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF size
> > for PCI hotplug bridges.
> 
> Makes sense.
> 
> > Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF size if
> > hpmempref has been specified, rather than controlling both MMIO and
> > MMIO_PREF sizes unconditionally.
> 
> I don't think I like that fact that hpmemsize behaves differently if
> hpmempref size is specfied before it. I'd probably suggest having three
> parameters: hpmemsize which sets both as it always has, a pref one and a
> regular one which each set one of parameters.

It does not matter if hpmempref is specified before or after hpmemsize. 
I made sure of that.

Originally, I proposed to depreciate hpiosize, hpmemsize, and introduce: 
hp_io_size, hp_mmio_size, hp_mmio_pref_size, each controlling its own 
window exclusively.

The patch had the old parameters work with a warning, and if the new 
ones were specified, they would override the old ones. Then, after a few 
kernel releases, the old ones could be removed.

Bjorn insisted that there be nil changes which break the existing 
parameters, and the solution he requested was to leave hpmemsize to work 
exactly the same (controlling both MMIO and MMIO_PREF), unless 
hpmemprefsize is given, which will take control of MMIO_PREF from 
hpmemsize.

From a maintainer's perspective, I see where he is coming from, 
particularly if the changes were to backfire and cause disruptions in 
existing setups in the event of a kernel upgrade. What we are left with 
is not optimal, but it will work.

(Assuming this is accepted): In the future, if somebody were to come up 
with a convincing argument to alter this, the patch to make each 
parameter solely control its own window would be miniscule and easy to 
sign off.

Cheers

> 
> Logan

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Logan Gunthorpe]

On 2019-06-19 6:56 p.m., Nicholas Johnson wrote:
> On Wed, Jun 19, 2019 at 10:45:38AM -0600, Logan Gunthorpe wrote:
>>
>>
>> On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
>>> Hi Ben and Logan,
>>>
>>> It looks like my git send-email has been not working correctly since I
>>> started trying to get these patches accepted. I may have remedied this
>>> now, but I have seen that Logan tried to find these patches and failed.
>>> So as a courtesy until I post PATCH v7 (hopefully correctly, this time),
>>> I am forwarding you the patches. I hope you like them. I would love to 
>>> know of any concerns or questions you may have, and / or what happens if 
>>> you test them. Thanks and all the best!
>>>
>>> ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
>>>
>>> Date: Thu, 23 May 2019 06:29:28 +0800
>>> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
>>> To: linux-kernel@vger.kernel.org
>>> Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
>>> Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
>>> X-Mailer: git-send-email 2.19.1
>>>
>>> Add kernel parameter pci=hpmemprefsize=nn[KMG] to control MMIO_PREF size
>>> for PCI hotplug bridges.
>>
>> Makes sense.
>>
>>> Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF size if
>>> hpmempref has been specified, rather than controlling both MMIO and
>>> MMIO_PREF sizes unconditionally.
>>
>> I don't think I like that fact that hpmemsize behaves differently if
>> hpmempref size is specfied before it. I'd probably suggest having three
>> parameters: hpmemsize which sets both as it always has, a pref one and a
>> regular one which each set one of parameters.
> 
> It does not matter if hpmempref is specified before or after hpmemsize. 
> I made sure of that.

> Originally, I proposed to depreciate hpiosize, hpmemsize, and introduce: 
> hp_io_size, hp_mmio_size, hp_mmio_pref_size, each controlling its own 
> window exclusively.
> 
> The patch had the old parameters work with a warning, and if the new 
> ones were specified, they would override the old ones. Then, after a few 
> kernel releases, the old ones could be removed.

Well I don't like that either. No need to depreciate hpmemsize.

> Bjorn insisted that there be nil changes which break the existing 
> parameters, and the solution he requested was to leave hpmemsize to work 
> exactly the same (controlling both MMIO and MMIO_PREF), unless 
> hpmemprefsize is given, which will take control of MMIO_PREF from 
> hpmemsize.

I agree with Bjorn here too but my suggestion is to leave hpmemsize
alone and have it set both values as it has always done. And add two new
parameters to set one or the other. Then there's none of this "sets one
if the other one wasn't set". Also, if I only want to change the
non-preftechable version then your method leaves no way to do so without
setting the preftechable version.

Logan

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Bjorn Helgaas]

On Wed, Jun 19, 2019 at 07:35:21PM -0600, Logan Gunthorpe wrote:
> On 2019-06-19 6:56 p.m., Nicholas Johnson wrote:
> > On Wed, Jun 19, 2019 at 10:45:38AM -0600, Logan Gunthorpe wrote:
> >> On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
> >>> ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
> >>>
> >>> Date: Thu, 23 May 2019 06:29:28 +0800
> >>> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> >>> To: linux-kernel@vger.kernel.org
> >>> Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> >>> Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
> >>> X-Mailer: git-send-email 2.19.1
> >>>
> >>> Add kernel parameter pci=hpmemprefsize=nn[KMG] to control
> >>> MMIO_PREF size for PCI hotplug bridges.
> >>
> >> Makes sense.
> >>
> >>> Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF
> >>> size if hpmempref has been specified, rather than controlling
> >>> both MMIO and MMIO_PREF sizes unconditionally.
> >>
> >> I don't think I like that fact that hpmemsize behaves differently
> >> if hpmempref size is specfied before it. I'd probably suggest
> >> having three parameters: hpmemsize which sets both as it always
> >> has, a pref one and a regular one which each set one of
> >> parameters.
> > 
> > It does not matter if hpmempref is specified before or after
> > hpmemsize.  I made sure of that.
> 
> > Originally, I proposed to depreciate hpiosize, hpmemsize, and
> > introduce: hp_io_size, hp_mmio_size, hp_mmio_pref_size, each
> > controlling its own window exclusively.
> > 
> > The patch had the old parameters work with a warning, and if the
> > new ones were specified, they would override the old ones. Then,
> > after a few kernel releases, the old ones could be removed.
> 
> Well I don't like that either. No need to depreciate hpmemsize.
> 
> > Bjorn insisted that there be nil changes which break the existing
> > parameters, and the solution he requested was to leave hpmemsize
> > to work exactly the same (controlling both MMIO and MMIO_PREF),
> > unless hpmemprefsize is given, which will take control of
> > MMIO_PREF from hpmemsize.
> 
> I agree with Bjorn here too but my suggestion is to leave hpmemsize
> alone and have it set both values as it has always done. And add two
> new parameters to set one or the other. Then there's none of this
> "sets one if the other one wasn't set". Also, if I only want to
> change the non-preftechable version then your method leaves no way
> to do so without setting the preftechable version.

Adding two new parameters sounds like a good idea to me.

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Nicholas Johnson]

On Thu, Jun 20, 2019 at 08:47:12AM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 19, 2019 at 07:35:21PM -0600, Logan Gunthorpe wrote:
> > On 2019-06-19 6:56 p.m., Nicholas Johnson wrote:
> > > On Wed, Jun 19, 2019 at 10:45:38AM -0600, Logan Gunthorpe wrote:
> > >> On 2019-06-19 8:01 a.m., Nicholas Johnson wrote:
> > >>> ----- Forwarded message from Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au> -----
> > >>>
> > >>> Date: Thu, 23 May 2019 06:29:28 +0800
> > >>> From: Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > >>> To: linux-kernel@vger.kernel.org
> > >>> Cc: linux-pci@vger.kernel.org, bhelgaas@google.com, mika.westerberg@linux.intel.com, corbet@lwn.net, Nicholas Johnson <nicholas.johnson-opensource@outlook.com.au>
> > >>> Subject: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently
> > >>> X-Mailer: git-send-email 2.19.1
> > >>>
> > >>> Add kernel parameter pci=hpmemprefsize=nn[KMG] to control
> > >>> MMIO_PREF size for PCI hotplug bridges.
> > >>
> > >> Makes sense.
> > >>
> > >>> Change behaviour of pci=hpmemsize=nn[KMG] to not set MMIO_PREF
> > >>> size if hpmempref has been specified, rather than controlling
> > >>> both MMIO and MMIO_PREF sizes unconditionally.
> > >>
> > >> I don't think I like that fact that hpmemsize behaves differently
> > >> if hpmempref size is specfied before it. I'd probably suggest
> > >> having three parameters: hpmemsize which sets both as it always
> > >> has, a pref one and a regular one which each set one of
> > >> parameters.
> > > 
> > > It does not matter if hpmempref is specified before or after
> > > hpmemsize.  I made sure of that.
> > 
> > > Originally, I proposed to depreciate hpiosize, hpmemsize, and
> > > introduce: hp_io_size, hp_mmio_size, hp_mmio_pref_size, each
> > > controlling its own window exclusively.
> > > 
> > > The patch had the old parameters work with a warning, and if the
> > > new ones were specified, they would override the old ones. Then,
> > > after a few kernel releases, the old ones could be removed.
> > 
> > Well I don't like that either. No need to depreciate hpmemsize.
> > 
> > > Bjorn insisted that there be nil changes which break the existing
> > > parameters, and the solution he requested was to leave hpmemsize
> > > to work exactly the same (controlling both MMIO and MMIO_PREF),
> > > unless hpmemprefsize is given, which will take control of
> > > MMIO_PREF from hpmemsize.
> > 
> > I agree with Bjorn here too but my suggestion is to leave hpmemsize
> > alone and have it set both values as it has always done. And add two
> > new parameters to set one or the other. Then there's none of this
> > "sets one if the other one wasn't set". Also, if I only want to
> > change the non-preftechable version then your method leaves no way
> > to do so without setting the preftechable version.
> 
> Adding two new parameters sounds like a good idea to me.
Yeah, that is basically what I did originally (except I depreciated the 
old ones rather than keeping them).

I did it this way on your direct advice in keeping with minimal lines of 
diff, minimal disruption, etc. If I were to do this, the number of lines 
of diff will increase and then I will be fielding complaints that it is 
too large to sign off.

I am already scrambling to make last minute changes before end of 
release to the other patches and I am not even convinced that that stuff 
is going to get accepted based on proximity to deadline and how many 
change requests are flying around.

So I am going to have to respectfully decline to do this for now. I need 
to know earlier in the release cycle if I am going to go back on stuff I 
have already been asked to do.

Cheers.

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Logan Gunthorpe]

On 2019-06-20 8:57 p.m., Nicholas Johnson wrote:
>> Adding two new parameters sounds like a good idea to me.
> Yeah, that is basically what I did originally (except I depreciated the 
> old ones rather than keeping them).
> 
> I did it this way on your direct advice in keeping with minimal lines of 
> diff, minimal disruption, etc. If I were to do this, the number of lines 
> of diff will increase and then I will be fielding complaints that it is 
> too large to sign off.
> 
> I am already scrambling to make last minute changes before end of 
> release to the other patches and I am not even convinced that that stuff 
> is going to get accepted based on proximity to deadline and how many 
> change requests are flying around.

Friendly advice: Linux Kernel development doesn't really work on
deadlines. The patch I linked you to has already been around a couple of
cycles and has missed a couple of merge windows. It's not that big of a
deal. I  try to make it better, if I can, and resubmit it once or twice
a cycle. It will get in when other people understand it and it meets the
community's standards. I had one patch set I submitted for more than a
year and a half, or 25 times, and it eventually got picked up. It's not
always the best experience but patience, persistence and openness to
feedback are what works.

New kernel parameters are important to get right because they are user
facing interfaces and we are stuck with them forever -- breaking
existing users is simply not accepted here. Deprecating features is an
extreme action that the Linux community takes pains to avoid. If we cede
to deadlines to get a new parameter in, and it turns out to be
non-ideal, then we are stuck supporting it forever and it's painful for
everyone.

Logan

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Nicholas Johnson]

On Thu, Jun 20, 2019 at 11:11:05PM -0600, Logan Gunthorpe wrote:
> 
> 
> On 2019-06-20 8:57 p.m., Nicholas Johnson wrote:
> >> Adding two new parameters sounds like a good idea to me.
> > Yeah, that is basically what I did originally (except I depreciated the 
> > old ones rather than keeping them).
> > 
> > I did it this way on your direct advice in keeping with minimal lines of 
> > diff, minimal disruption, etc. If I were to do this, the number of lines 
> > of diff will increase and then I will be fielding complaints that it is 
> > too large to sign off.
> > 
> > I am already scrambling to make last minute changes before end of 
> > release to the other patches and I am not even convinced that that stuff 
> > is going to get accepted based on proximity to deadline and how many 
> > change requests are flying around.
> 
> Friendly advice: Linux Kernel development doesn't really work on
> deadlines. The patch I linked you to has already been around a couple of
> cycles and has missed a couple of merge windows. It's not that big of a
> deal. I  try to make it better, if I can, and resubmit it once or twice
> a cycle. It will get in when other people understand it and it meets the
> community's standards. I had one patch set I submitted for more than a
> year and a half, or 25 times, and it eventually got picked up. It's not
> always the best experience but patience, persistence and openness to
> feedback are what works.
> 
> New kernel parameters are important to get right because they are user
> facing interfaces and we are stuck with them forever -- breaking
> existing users is simply not accepted here. Deprecating features is an
> extreme action that the Linux community takes pains to avoid. If we cede
> to deadlines to get a new parameter in, and it turns out to be
> non-ideal, then we are stuck supporting it forever and it's painful for
> everyone.
> 
> Logan
Thanks for advice. I do believe you are correct. I guess it is my 
inexperience - I have trouble telling what is normal and what to expect 
from the process. But I am not feeling like this is any closer to 
submission than on day one - no sense of progress. It is not reassuring 
and I do wonder if I will ever manage to get it right.

None of this is technically Thunderbolt-specific, but that is the use 
case, and if a single one of these patches does not make it in then the 
use case of Thunderbolt without firmware support will remain broken. The 
only benefit of a partial acception is patch #1 which fixes a bug report 
by Mika Westerberg on its own, so I guess that would be a win. If it has 
to wait another cycle then I will live with it, but this is the cause of 
my anxiety.

I can look at this patch again, but my concerns are the following:

- If we avoid depreciation, then that implies this patch is ideal - we 
are not breaking anything for existing users. We can set MMIO on its own 
with pci=hpmemsize and by setting pci=hpmemprefsize to the default size of 
2M (so it remains unchanged).

- If we have two sets of parameters, then we have to support having two 
sets for eternity, and the loss of minimalism and the potential for 
confusion. The description in kernel-parameters.txt will become 
confusing with a lot of "if this" and "if that".

- How do we deal with order if both are specified? It will become 
unclean.

- If we make two new ones, then what should they be called? I propose 
hp_mmio_size and hp_mmio_pref_size from my original submission. Then 
should hpiosize be aliased to hp_io_size to keep the consistency?

- I am quite surprised that when 64-bit window support was added to the 
kernel, that this was not done then, although I am not good enough at 
navigating the mailing list to see what actually happened.

Should I perhaps forward my original submission for this patch again and 
you can see if there is anything to salvage from that that?

Cheers

Re: [nicholas.johnson-opensource@outlook.com.au: [PATCH v6 4/4] PCI: Add pci=hpmemprefsize parameter to set MMIO_PREF size independently]

[Logan Gunthorpe]

On 2019-06-21 12:01 a.m., Nicholas Johnson wrote:
> Thanks for advice. I do believe you are correct. I guess it is my 
> inexperience - I have trouble telling what is normal and what to expect 
> from the process. But I am not feeling like this is any closer to 
> submission than on day one - no sense of progress. It is not reassuring 
> and I do wonder if I will ever manage to get it right.
> 
> None of this is technically Thunderbolt-specific, but that is the use 
> case, and if a single one of these patches does not make it in then the 
> use case of Thunderbolt without firmware support will remain broken. The 
> only benefit of a partial acception is patch #1 which fixes a bug report 
> by Mika Westerberg on its own, so I guess that would be a win. If it has 
> to wait another cycle then I will live with it, but this is the cause of 
> my anxiety.
> 
> I can look at this patch again, but my concerns are the following:
> 
> - If we avoid depreciation, then that implies this patch is ideal - we 
> are not breaking anything for existing users. We can set MMIO on its own 
> with pci=hpmemsize and by setting pci=hpmemprefsize to the default size of 
> 2M (so it remains unchanged).

I disagree. The semantics are weird and the changes to the documentation
show that. When you have to describe things based on what other
parameters are doing, it's weird.

> - If we have two sets of parameters, then we have to support having two 
> sets for eternity, and the loss of minimalism and the potential for 
> confusion. The description in kernel-parameters.txt will become 
> confusing with a lot of "if this" and "if that".

Yes, but they're simple and clear parameters that are easy to support.
There would be no "ifs" at all. One parameter sets one value, another a
parameter sets another, and the original parameter sets both.

> - How do we deal with order if both are specified? It will become 
> unclean.

If a user mixes the "both" with the "single" parameters, the winner goes
to the last that's specified. This is common and predictable. It's not
dissimilar to when a user specifies the same parameter twice.

> - If we make two new ones, then what should they be called? I propose 
> hp_mmio_size and hp_mmio_pref_size from my original submission. Then 
> should hpiosize be aliased to hp_io_size to keep the consistency?

Good question. I'd probably go something along the lines of
'hpmemsize_pref' and 'hpmemsize_nonpref'. Though, it might be good to
get others opinions on this.

> - I am quite surprised that when 64-bit window support was added to the 
> kernel, that this was not done then, although I am not good enough at 
> navigating the mailing list to see what actually happened.

Generally, things won't get done until people have a use case for them
and motivation to make the change. I don't think there's a lot of users
of hpmemsize to begin with.

> Should I perhaps forward my original submission for this patch again and 
> you can see if there is anything to salvage from that that?

Not if you haven't changed anything.

Logan