linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dmytro Maluka <dmy@semihalf.com>
To: Victor Ding <victording@google.com>, Bjorn Helgaas <helgaas@kernel.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Ben Chuang <ben.chuang@genesyslogic.com.tw>,
	linux-pci@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	linux-mmc@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
	Chris Packham <chris.packham@alliedtelesis.co.nz>,
	Kai-Heng Feng <kai.heng.feng@canonical.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	"Saheed O. Bolarinwa" <refactormyself@gmail.com>,
	Vidya Sagar <vidyas@nvidia.com>,
	Xiongfeng Wang <wangxiongfeng2@huawei.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Grzegorz Jaszczyk <jaz@semihalf.com>,
	Tomasz Nowicki <tn@semihalf.com>, Zide Chen <zide.chen@intel.com>
Subject: Re: [PATCH v2] PCI/ASPM: Disable ASPM when save/restore PCI state
Date: Tue, 19 Jul 2022 21:11:44 +0200	[thread overview]
Message-ID: <5e6b3acf-1909-de42-3da7-c591e23ab221@semihalf.com> (raw)
In-Reply-To: <820dff42-67e4-32d2-a72f-9e9bdb70609e@semihalf.com>

On 7/18/22 18:21, Dmytro Maluka wrote:
> While we're at it, I'm also wondering why for the basic PCI config (the
> first 256 bytes) Linux on x86 always uses the legacy 0xCF8/0xCFC method
> instead of MMCFG, even if MMCFG is available. The legacy method is
> inherently non-atomic and does require the global lock, while the MMCFG
> method generally doesn't, so using MMCFG would significantly speed up
> PCI config accesses in high-contention scenarios like the parallel
> suspend/resume.
> 
> I've tried the below change which forces using MMCFG for the first 256
> bytes, and indeed, it makes suspend/resume of individual PCI devices
> with pm_async=1 almost as fast as with pm_async=0. In particular, it
> fixes the problem with slow GL9750 suspend/resume even without Victor's
> patch.
> 
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -40,20 +40,20 @@ const struct pci_raw_ops *__read_mostly raw_pci_ext_ops;
>  int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
>                                                 int reg, int len, u32 *val)
>  {
> -       if (domain == 0 && reg < 256 && raw_pci_ops)
> -               return raw_pci_ops->read(domain, bus, devfn, reg, len, val);
>         if (raw_pci_ext_ops)
>                 return raw_pci_ext_ops->read(domain, bus, devfn, reg, len, val);
> +       if (domain == 0 && reg < 256 && raw_pci_ops)
> +               return raw_pci_ops->read(domain, bus, devfn, reg, len, val);
>         return -EINVAL;
>  }
>  
>  int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
>                                                 int reg, int len, u32 val)
>  {
> -       if (domain == 0 && reg < 256 && raw_pci_ops)
> -               return raw_pci_ops->write(domain, bus, devfn, reg, len, val);
>         if (raw_pci_ext_ops)
>                 return raw_pci_ext_ops->write(domain, bus, devfn, reg, len, val);
> +       if (domain == 0 && reg < 256 && raw_pci_ops)
> +               return raw_pci_ops->write(domain, bus, devfn, reg, len, val);
>         return -EINVAL;
>  }
>  
> 
> Sounds good if I submit a patch like this? (I'm not suggesting it
> instead of Victor's patch, rather as a separate improvement.)

Ok, I found that a similar change was already suggested in the past by
Thomas [1] and got rejected by Linus [2].

Linus' arguments sound reasonable, and I understand that back then the
only known case of an issue with PCI config lock contention was with
Intel PMU counter registers which are in the extended config space
anyway. But now we know another case of such a contention, concerning
the basic config space too, namely: suspending or resuming many PCI
devices in parallel during system suspend/resume.

I've checked that on my box using MMCFG instead of Type 1 (i.e. using my
above patch) reduces the total suspend or resume time by 15-20 ms on
average. (I also had Victor's patch applied all the time, i.e. the ASPM
L1 exit latency issue was already resolved, so my test was about the PCI
lock contention in general.) So, not exactly a major improvement, yet
not exactly a negligible one. Maybe it's worth optimizing, maybe not.

Anyway, that's a bit of digression. Let's focus primarily on Victor's
ASPM patch.

[1]
https://lore.kernel.org/all/tip-b5b0f00c760b6e9673ab79b88ede2f3c7a039f74@git.kernel.org/

[2]
https://lore.kernel.org/all/CA+55aFwi0tkdugfqNEz6M28RXM2jx6WpaDF4nfA=doUVdZgUNQ@mail.gmail.com/

Thanks,
Dmytro

      reply	other threads:[~2022-07-19 19:11 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-28 15:52 Victor Ding
2021-03-11 17:34 ` Bjorn Helgaas
2021-07-26 22:03   ` Bjorn Helgaas
2022-02-09  8:03     ` Victor Ding
2022-07-18 16:21       ` Dmytro Maluka
2022-07-19 19:11         ` Dmytro Maluka [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5e6b3acf-1909-de42-3da7-c591e23ab221@semihalf.com \
    --to=dmy@semihalf.com \
    --cc=adrian.hunter@intel.com \
    --cc=ben.chuang@genesyslogic.com.tw \
    --cc=bhelgaas@google.com \
    --cc=chris.packham@alliedtelesis.co.nz \
    --cc=helgaas@kernel.org \
    --cc=jaz@semihalf.com \
    --cc=kai.heng.feng@canonical.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=refactormyself@gmail.com \
    --cc=tglx@linutronix.de \
    --cc=tn@semihalf.com \
    --cc=ulf.hansson@linaro.org \
    --cc=victording@google.com \
    --cc=vidyas@nvidia.com \
    --cc=wangxiongfeng2@huawei.com \
    --cc=zide.chen@intel.com \
    --subject='Re: [PATCH v2] PCI/ASPM: Disable ASPM when save/restore PCI state' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).