I think this summary of the regression is appropriate for a top-post.
Details follow below.
commit bdd8b6c98239: introduced what I call a real regression which
persists in 5.17.x
Jan's proposed patch:
https://lore.kernel.org/lkml/[email protected]/
Jan's patch would fix the real regression introduced by bdd8b6c98239 when
the nopat option is not enabled, but when the nopat option is enabled, this
patch would introduce what Jan calls a "perceived regression" that is really
caused by the failure of the i915 driver to handle the case of the nopat
option
being provided on the command line properly.
What I request: commit Jan's proposed patch, and backport it to 5.17. That
would fix the real regression and only cause a perceived regression for the
case when nopat is enabled. In that case, patches to the i915 driver
would be helpful but necessary to fix a regression.
Regard,
Chuck Zmudzinski
On 5/20/2022 11:46 AM, Chuck Zmudzinski wrote:
> On 5/20/2022 10:06 AM, Jan Beulich wrote:
>> On 20.05.2022 15:33, Chuck Zmudzinski wrote:
>>> On 5/20/2022 5:41 AM, Jan Beulich wrote:
>>>> On 20.05.2022 10:30, Chuck Zmudzinski wrote:
>>>>> On 5/20/2022 2:59 AM, Chuck Zmudzinski wrote:
>>>>>> On 5/20/2022 2:05 AM, Jan Beulich wrote:
>>>>>>> On 20.05.2022 06:43, Chuck Zmudzinski wrote:
>>>>>>>> On 5/4/22 5:14 AM, Juergen Gross wrote:
>>>>>>>>> On 04.05.22 10:31, Jan Beulich wrote:
>>>>>>>>>> On 03.05.2022 15:22, Juergen Gross wrote:
>>>>>>>>>>
>>>>>>>>>> ... these uses there are several more. You say nothing on why
>>>>>>>>>> those want
>>>>>>>>>> leaving unaltered. When preparing my earlier patch I did
>>>>>>>>>> inspect them
>>>>>>>>>> and came to the conclusion that these all would also better
>>>>>>>>>> observe the
>>>>>>>>>> adjusted behavior (or else I couldn't have left pat_enabled()
>>>>>>>>>> as the
>>>>>>>>>> only predicate). In fact, as said in the description of my
>>>>>>>>>> earlier
>>>>>>>>>> patch, in
>>>>>>>>>> my debugging I did find the use in i915_gem_object_pin_map()
>>>>>>>>>> to be
>>>>>>>>>> the
>>>>>>>>>> problematic one, which you leave alone.
>>>>>>>>> Oh, I missed that one, sorry.
>>>>>>>> That is why your patch would not fix my Haswell unless
>>>>>>>> it also touches i915_gem_object_pin_map() in
>>>>>>>> drivers/gpu/drm/i915/gem/i915_gem_pages.c
>>>>>>>>
>>>>>>>>> I wanted to be rather defensive in my changes, but I agree at
>>>>>>>>> least
>>>>>>>>> the
>>>>>>>>> case in arch_phys_wc_add() might want to be changed, too.
>>>>>>>> I think your approach needs to be more aggressive so it will fix
>>>>>>>> all the known false negatives introduced by bdd8b6c98239
>>>>>>>> such as the one in i915_gem_object_pin_map().
>>>>>>>>
>>>>>>>> I looked at Jan's approach and I think it would fix the issue
>>>>>>>> with my Haswell as long as I don't use the nopat option. I
>>>>>>>> really don't have a strong opinion on that question, but I
>>>>>>>> think the nopat option as a Linux kernel option, as opposed
>>>>>>>> to a hypervisor option, should only affect the kernel, and
>>>>>>>> if the hypervisor provides the pat feature, then the kernel
>>>>>>>> should not override that,
>>>>>>> Hmm, why would the kernel not be allowed to override that? Such
>>>>>>> an override would affect only the single domain where the
>>>>>>> kernel runs; other domains could take their own decisions.
>>>>>>>
>>>>>>> Also, for the sake of completeness: "nopat" used when running on
>>>>>>> bare metal has the same bad effect on system boot, so there
>>>>>>> pretty clearly is an error cleanup issue in the i915 driver. But
>>>>>>> that's orthogonal, and I expect the maintainers may not even care
>>>>>>> (but tell us "don't do that then").
>>>>> Actually I just did a test with the last official Debian kernel
>>>>> build of Linux 5.16, that is, a kernel before bdd8b6c98239 was
>>>>> applied. In fact, the nopat option does *not* break the i915 driver
>>>>> in 5.16. That is, with the nopat option, the i915 driver loads
>>>>> normally on both the bare metal and on the Xen hypervisor.
>>>>> That means your presumption (and the presumption of
>>>>> the author of bdd8b6c98239) that the "nopat" option was
>>>>> being observed by the i915 driver is incorrect. Setting "nopat"
>>>>> had no effect on my system with Linux 5.16. So after doing these
>>>>> tests, I am against the aggressive approach of breaking the i915
>>>>> driver with the "nopat" option because prior to bdd8b6c98239,
>>>>> nopat did not break the i915 driver. Why break it now?
>>>> Because that's, in my understanding, is the purpose of "nopat"
>>>> (not breaking the driver of course - that's a driver bug -, but
>>>> having an effect on the driver).
>>> I wouldn't call it a driver bug, but an incorrect configuration of the
>>> kernel by the user. I presume X86_FEATURE_PAT is required by the
>>> i915 driver
>> The driver ought to work fine without PAT (and hence without being
>> able to make WC mappings). It would use UC instead and be slow, but
>> it ought to work.
>
> I am not an expert, but I think the reason it failed on my box was
> because of the requirements of CI. Maybe the driver would fall back
> to UC if the add_taint_for_CI function did not halt the entire system
> in response to the failed test for PAT when trying to use WC mappings.
>
>>> and therefore the driver should refuse to disable
>>> it if the user requests to disable it and instead warn the user that
>>> the driver did not disable the feature, contrary to what the user
>>> requested with the nopat option.
>>>
>>> In any case, my test did not verify that when nopat is set in Linux
>>> 5.16,
>>> the thread takes the same code path as when nopat is not set,
>>> so I am not totally sure that the reason nopat does not break the
>>> i915 driver in 5.16 is that static_cpu_has(X86_FEATURE_PAT)
>>> returns true even when nopat is set. I could test it with a custom
>>> log message in 5.16 if that is necessary.
>>>
>>> Are you saying it was wrong for
>>> to return true in 5.16 when the user requests nopat?
>> No, I'm not saying that. It was wrong for this construct to be used
>> in the driver, which was fixed for 5.17 (and which had caused the
>> regression I did observe, leading to the patch as a hopefully least
>> bad option).
>
> Hmm, the patch I used to fix my box with 5.17.6 used
> static_cpu_has(X86_FEATURE_PAT) so the driver could
> continue to configure the hardware using WC. This is the
> relevant part of the patch I used to fix my box, which includes
> extra error logs, (against Debian's official build of 5.17.6):
>
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-09
> 03:16:33.000000000 -0400
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-19
> 15:55:40.339778818 -0400
> ...
> @@ -430,17 +434,23 @@
> err = i915_gem_object_wait_moving_fence(obj, true);
> if (err) {
> ptr = ERR_PTR(err);
> + DRM_ERROR("i915_gem_object_wait_moving_fence error, err =
> %d\n", err);
> goto err_unpin;
> }
>
> - if (GEM_WARN_ON(type == I915_MAP_WC && !pat_enabled()))
> + if (GEM_WARN_ON(type == I915_MAP_WC &&
> + !pat_enabled() && !static_cpu_has(X86_FEATURE_PAT))) {
> + DRM_ERROR("type == I915_MAP_WC && !pat_enabled(), err =
> %d\n", -ENODEV);
> ptr = ERR_PTR(-ENODEV);
> + }
> else if (i915_gem_object_has_struct_page(obj))
> ptr = i915_gem_object_map_page(obj, type);
> else
> ptr = i915_gem_object_map_pfn(obj, type);
> - if (IS_ERR(ptr))
> + if (IS_ERR(ptr)) {
> + DRM_ERROR("IS_ERR(PTR) is true, returning a (ptr) error\n");
> goto err_unpin;
> + }
>
> obj->mm.mapping = page_pack_bits(ptr, type);
> }
>
> As you can see, adding the static_cpu_has(X86_FEATURE_PAT)
> function to the test for PAT restored the behavior of 5.16 on the
> Xen hypervisor to 5.17, and that is how I discovered the solution
> to this problem on 5.17 on my box.
>
>>> I think that is
>>> just permitting a bad configuration to break the driver that a
>>> well-written operating system should not allow. The i915 driver
>>> was, in my opinion, correctly ignoring the nopat option in 5.16
>>> because that option is not compatible with the hardware the
>>> i915 driver is trying to initialize and setup at boot time. At least
>>> that is my understanding now, but I will need to test it on 5.16
>>> to be sure I understand it correctly.
>>>
>>> Also, AFAICT, your patch would break the driver when the nopat
>>> option is set and only fix the regression introduced by bdd8b6c98239
>>> when nopat is not set on my box, so your patch would
>>> introduce a regression relative to Linux 5.16 and earlier for the
>>> case when nopat is set on my box. I think your point would
>>> be that it is not a regression if it is an incorrect user
>>> configuration.
>> Again no - my view is that there's a separate, pre-existing issue
>> in the driver which was uncovered by the change. This may be a
>> perceived regression, but is imo different from a real one.
>
> Maybe it is only a perceived regression if nopat is set, but
> imo bdd8b6c98239 introduced a real regression in 5.17
> relative to 5.16 for the correctly and identically configured
> case when the nopat option is not set. That is why I still think
> it should be reverted and the fix backported to 5.17 until the
> regression for the case when nopat is not set is fixed. As I
> said before, the i915 driver relies on the memory subsyste
> to provide it with an accurate test for the x86 pat feature.
> The test the driver used in bdd8b6c98239 gives the i915 driver
> a false negative, and that caused a real regression when nopat
> is not set. bdd8b6c98239 can be re-applied if we apply your
> patch which corrects the false negative that pat_enabled() is
> currently providing the i915 driver with. That false negative
> from pat_enabled() is not an i915 bug, it is a bug in x86/pat.
>
> Chuck
On 5/20/2022 1:13 PM, Chuck Zmudzinski wrote:
> I think this summary of the regression is appropriate for a top-post.
> Details follow below.
>
> commit bdd8b6c98239: introduced what I call a real regression which
> persists in 5.17.x
>
> Jan's proposed patch:
> https://lore.kernel.org/lkml/[email protected]/
>
> Jan's patch would fix the real regression introduced by bdd8b6c98239 when
> the nopat option is not enabled, but when the nopat option is enabled,
> this
> patch would introduce what Jan calls a "perceived regression" that is
> really
> caused by the failure of the i915 driver to handle the case of the
> nopat option
> being provided on the command line properly.
>
> What I request: commit Jan's proposed patch, and backport it to 5.17.
> That
> would fix the real regression and only cause a perceived regression
> for the
> case when nopat is enabled. In that case, patches to the i915 driver
> would be helpful but necessary to fix a regression.
Sorry again, I mean patches to i915 would be helpful but *not* necessary
to fix a regression.
Regards,
Chuck Zmudzinski
>
> On 5/20/2022 11:46 AM, Chuck Zmudzinski wrote:
>> On 5/20/2022 10:06 AM, Jan Beulich wrote:
>>> On 20.05.2022 15:33, Chuck Zmudzinski wrote:
>>>> On 5/20/2022 5:41 AM, Jan Beulich wrote:
>>>>> On 20.05.2022 10:30, Chuck Zmudzinski wrote:
>>>>>> On 5/20/2022 2:59 AM, Chuck Zmudzinski wrote:
>>>>>>> On 5/20/2022 2:05 AM, Jan Beulich wrote:
>>>>>>>> On 20.05.2022 06:43, Chuck Zmudzinski wrote:
>>>>>>>>> On 5/4/22 5:14 AM, Juergen Gross wrote:
>>>>>>>>>> On 04.05.22 10:31, Jan Beulich wrote:
>>>>>>>>>>> On 03.05.2022 15:22, Juergen Gross wrote:
>>>>>>>>>>>
>>>>>>>>>>> ... these uses there are several more. You say nothing on why
>>>>>>>>>>> those want
>>>>>>>>>>> leaving unaltered. When preparing my earlier patch I did
>>>>>>>>>>> inspect them
>>>>>>>>>>> and came to the conclusion that these all would also better
>>>>>>>>>>> observe the
>>>>>>>>>>> adjusted behavior (or else I couldn't have left
>>>>>>>>>>> pat_enabled() as the
>>>>>>>>>>> only predicate). In fact, as said in the description of my
>>>>>>>>>>> earlier
>>>>>>>>>>> patch, in
>>>>>>>>>>> my debugging I did find the use in i915_gem_object_pin_map()
>>>>>>>>>>> to be
>>>>>>>>>>> the
>>>>>>>>>>> problematic one, which you leave alone.
>>>>>>>>>> Oh, I missed that one, sorry.
>>>>>>>>> That is why your patch would not fix my Haswell unless
>>>>>>>>> it also touches i915_gem_object_pin_map() in
>>>>>>>>> drivers/gpu/drm/i915/gem/i915_gem_pages.c
>>>>>>>>>
>>>>>>>>>> I wanted to be rather defensive in my changes, but I agree at
>>>>>>>>>> least
>>>>>>>>>> the
>>>>>>>>>> case in arch_phys_wc_add() might want to be changed, too.
>>>>>>>>> I think your approach needs to be more aggressive so it will fix
>>>>>>>>> all the known false negatives introduced by bdd8b6c98239
>>>>>>>>> such as the one in i915_gem_object_pin_map().
>>>>>>>>>
>>>>>>>>> I looked at Jan's approach and I think it would fix the issue
>>>>>>>>> with my Haswell as long as I don't use the nopat option. I
>>>>>>>>> really don't have a strong opinion on that question, but I
>>>>>>>>> think the nopat option as a Linux kernel option, as opposed
>>>>>>>>> to a hypervisor option, should only affect the kernel, and
>>>>>>>>> if the hypervisor provides the pat feature, then the kernel
>>>>>>>>> should not override that,
>>>>>>>> Hmm, why would the kernel not be allowed to override that? Such
>>>>>>>> an override would affect only the single domain where the
>>>>>>>> kernel runs; other domains could take their own decisions.
>>>>>>>>
>>>>>>>> Also, for the sake of completeness: "nopat" used when running on
>>>>>>>> bare metal has the same bad effect on system boot, so there
>>>>>>>> pretty clearly is an error cleanup issue in the i915 driver. But
>>>>>>>> that's orthogonal, and I expect the maintainers may not even care
>>>>>>>> (but tell us "don't do that then").
>>>>>> Actually I just did a test with the last official Debian kernel
>>>>>> build of Linux 5.16, that is, a kernel before bdd8b6c98239 was
>>>>>> applied. In fact, the nopat option does *not* break the i915 driver
>>>>>> in 5.16. That is, with the nopat option, the i915 driver loads
>>>>>> normally on both the bare metal and on the Xen hypervisor.
>>>>>> That means your presumption (and the presumption of
>>>>>> the author of bdd8b6c98239) that the "nopat" option was
>>>>>> being observed by the i915 driver is incorrect. Setting "nopat"
>>>>>> had no effect on my system with Linux 5.16. So after doing these
>>>>>> tests, I am against the aggressive approach of breaking the i915
>>>>>> driver with the "nopat" option because prior to bdd8b6c98239,
>>>>>> nopat did not break the i915 driver. Why break it now?
>>>>> Because that's, in my understanding, is the purpose of "nopat"
>>>>> (not breaking the driver of course - that's a driver bug -, but
>>>>> having an effect on the driver).
>>>> I wouldn't call it a driver bug, but an incorrect configuration of the
>>>> kernel by the user. I presume X86_FEATURE_PAT is required by the
>>>> i915 driver
>>> The driver ought to work fine without PAT (and hence without being
>>> able to make WC mappings). It would use UC instead and be slow, but
>>> it ought to work.
>>
>> I am not an expert, but I think the reason it failed on my box was
>> because of the requirements of CI. Maybe the driver would fall back
>> to UC if the add_taint_for_CI function did not halt the entire system
>> in response to the failed test for PAT when trying to use WC mappings.
>>
>>>> and therefore the driver should refuse to disable
>>>> it if the user requests to disable it and instead warn the user that
>>>> the driver did not disable the feature, contrary to what the user
>>>> requested with the nopat option.
>>>>
>>>> In any case, my test did not verify that when nopat is set in Linux
>>>> 5.16,
>>>> the thread takes the same code path as when nopat is not set,
>>>> so I am not totally sure that the reason nopat does not break the
>>>> i915 driver in 5.16 is that static_cpu_has(X86_FEATURE_PAT)
>>>> returns true even when nopat is set. I could test it with a custom
>>>> log message in 5.16 if that is necessary.
>>>>
>>>> Are you saying it was wrong for
>>>> to return true in 5.16 when the user requests nopat?
>>> No, I'm not saying that. It was wrong for this construct to be used
>>> in the driver, which was fixed for 5.17 (and which had caused the
>>> regression I did observe, leading to the patch as a hopefully least
>>> bad option).
>>
>> Hmm, the patch I used to fix my box with 5.17.6 used
>> static_cpu_has(X86_FEATURE_PAT) so the driver could
>> continue to configure the hardware using WC. This is the
>> relevant part of the patch I used to fix my box, which includes
>> extra error logs, (against Debian's official build of 5.17.6):
>>
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-09
>> 03:16:33.000000000 -0400
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c 2022-05-19
>> 15:55:40.339778818 -0400
>> ...
>> @@ -430,17 +434,23 @@
>> err = i915_gem_object_wait_moving_fence(obj, true);
>> if (err) {
>> ptr = ERR_PTR(err);
>> + DRM_ERROR("i915_gem_object_wait_moving_fence error, err
>> = %d\n", err);
>> goto err_unpin;
>> }
>>
>> - if (GEM_WARN_ON(type == I915_MAP_WC && !pat_enabled()))
>> + if (GEM_WARN_ON(type == I915_MAP_WC &&
>> + !pat_enabled() && !static_cpu_has(X86_FEATURE_PAT))) {
>> + DRM_ERROR("type == I915_MAP_WC && !pat_enabled(), err =
>> %d\n", -ENODEV);
>> ptr = ERR_PTR(-ENODEV);
>> + }
>> else if (i915_gem_object_has_struct_page(obj))
>> ptr = i915_gem_object_map_page(obj, type);
>> else
>> ptr = i915_gem_object_map_pfn(obj, type);
>> - if (IS_ERR(ptr))
>> + if (IS_ERR(ptr)) {
>> + DRM_ERROR("IS_ERR(PTR) is true, returning a (ptr)
>> error\n");
>> goto err_unpin;
>> + }
>>
>> obj->mm.mapping = page_pack_bits(ptr, type);
>> }
>>
>> As you can see, adding the static_cpu_has(X86_FEATURE_PAT)
>> function to the test for PAT restored the behavior of 5.16 on the
>> Xen hypervisor to 5.17, and that is how I discovered the solution
>> to this problem on 5.17 on my box.
>>
>>>> I think that is
>>>> just permitting a bad configuration to break the driver that a
>>>> well-written operating system should not allow. The i915 driver
>>>> was, in my opinion, correctly ignoring the nopat option in 5.16
>>>> because that option is not compatible with the hardware the
>>>> i915 driver is trying to initialize and setup at boot time. At least
>>>> that is my understanding now, but I will need to test it on 5.16
>>>> to be sure I understand it correctly.
>>>>
>>>> Also, AFAICT, your patch would break the driver when the nopat
>>>> option is set and only fix the regression introduced by bdd8b6c98239
>>>> when nopat is not set on my box, so your patch would
>>>> introduce a regression relative to Linux 5.16 and earlier for the
>>>> case when nopat is set on my box. I think your point would
>>>> be that it is not a regression if it is an incorrect user
>>>> configuration.
>>> Again no - my view is that there's a separate, pre-existing issue
>>> in the driver which was uncovered by the change. This may be a
>>> perceived regression, but is imo different from a real one.
>>
>> Maybe it is only a perceived regression if nopat is set, but
>> imo bdd8b6c98239 introduced a real regression in 5.17
>> relative to 5.16 for the correctly and identically configured
>> case when the nopat option is not set. That is why I still think
>> it should be reverted and the fix backported to 5.17 until the
>> regression for the case when nopat is not set is fixed. As I
>> said before, the i915 driver relies on the memory subsyste
>> to provide it with an accurate test for the x86 pat feature.
>> The test the driver used in bdd8b6c98239 gives the i915 driver
>> a false negative, and that caused a real regression when nopat
>> is not set. bdd8b6c98239 can be re-applied if we apply your
>> patch which corrects the false negative that pat_enabled() is
>> currently providing the i915 driver with. That false negative
>> from pat_enabled() is not an i915 bug, it is a bug in x86/pat.
>>
>> Chuck
>