linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Rajat Jain <rajatja@google.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	Keith Busch <keith.busch@intel.com>,
	Vidya Sagar <vidyas@nvidia.com>,
	Philippe Ombredanne <pombredanne@nexb.com>,
	Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <garsilva@embeddedor.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Sinan Kaya <okaya@codeaurora.org>,
	Frederick Lawler <fred@fredlawl.com>,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	mayurkumar.patel@intel.com, rajatxjain@gmail.com,
	Richard Hughes <rhughes@redhat.com>,
	Carlos Garnacho <cgarnach@redhat.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Pali Rohar <pali.rohar@gmail.com>, Takashi Iwai <tiwai@suse.de>,
	Andy Whitcroft <apw@canonical.com>,
	Colin Ian King <colin.king@canonical.com>
Subject: Re: [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG
Date: Fri, 27 Jul 2018 15:26:19 -0500	[thread overview]
Message-ID: <20180727202619.GD173328@bhelgaas-glaptop.roam.corp.google.com> (raw)
In-Reply-To: <20180510233912.96454-1-rajatja@google.com>

[+cc Rafael, Richard, Carlos, Pali, Takashi, Andy, Colin for question
about how to expose ASPM power management in sysfs]

On Thu, May 10, 2018 at 04:39:12PM -0700, Rajat Jain wrote:
> ...
> And some suggestions from Bjorn here:
> https://www.spinics.net/lists/linux-pci/msg60541.html
> 
> This patch picks up one of the suggestion, to remove the
> CONFIG_PCIEASPM_DEBUG and thus make the code always
> avilable. This provides control to userspace to control
> ASPM on a per slot / device basis using sysfs interface.

TL;DR: When CONFIG_PCIEASPM_DEBUG=y, Linux makes ASPM control files
       visible in sysfs, associated with the upstream end (e.g., Root
       Port) of a link.  Should these files be associated with the
       downstream end (e.g., NIC, GPU, etc) instead?

I think we do need to make ASPM control files visible in sysfs, as
this patch does.  But I have a question about exactly *how* we want to
do this.  I had planned to merge this patch for v4.19, but I think
I'll postpone it until we figure this out.

ASPM is a power management feature of a PCIe link, and it affects the
devices on both ends of that link.  The upstream end (closest to the
CPU) is typically a Root Port, and the downstream end is typically an
Endpoint (e.g., a GPU, NIC, or NVMe device).  For example, my laptop
has these devices:

  00:1c.2 Intel Root Port to [bus 04]
  04:00.0 Intel 8260 Wireless NIC

There's a PCIe link between these two devices, and if both ends
support it, ASPM reduces power usage when the NIC is idle.  The
hardware reduces power usage automatically; the kernel only needs to
configure ASPM.

ASPM affects performance as well as power, so we have knobs to control
the tradeoff.  Starting with the big hammers, we currently have:

  - Kernel build-time setting (CONFIG_PCIEASPM_PERFORMANCE, etc).
    Requires kernel rebuild and reboot.

  - "pcie_aspm=X" kernel boot parameter.  Can only enable/disable and
    requires a reboot.

  - /sys/module/pcie_aspm/parameters/policy file.  At any time, root
    can set the system-wide ASPM policy to one of these settings:

      + highest performance, highest power consumption
      + moderate power saving at cost of some performance
      + aggressive power saving at cost of more performance
      + use whatever setting BIOS configured

  - Many /sys/devices/pci0000:00/0000:00:1c.2/power/link_state files.
    At any time, root can set the ASPM policy to one of the above
    settings, but for individual devices.  Currently these files are
    only available when CONFIG_PCIEASPM_DEBUG=y (Red Hat does not
    enable this, but Ubuntu does).

The patch below changes it so these /sys/devices/.../link_state files
*always* exist, regardless of CONFIG_PCIEASPM_DEBUG.

The question is where those sysfs files should be.  Currently they are
associated with the device at the *upstream* end of the link.  In the
example above, they're associated with the Root Port:

  /sys/devices/pci0000:00/0000:00:1c.2/power/link_state

I don't know if that's the right place, or if they should be
associated with the device at the *downstream* end of the link, i.e.,
04:00.0.  The downstream end might be better because:

  - It's easier to associate the downstream end with a device the user
    cares about, e.g., a NIC, GPU, etc.  This is mostly a user-
    interface issue.

  - A link can lead to a multi-function device, and the spec allows
    those functions to have different ASPM settings (see PCIe r4.0,
    sec 5.4.1).  With the sysfs files at the upstream end of the link,
    we have no way to configure those functions individually.

Any thoughts?

> ...
> diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
> index b12e28b3d8f9..089b9f559d88 100644
> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -46,14 +46,6 @@ config PCIEASPM
>  
>  	  When in doubt, say Y.
>  
> -config PCIEASPM_DEBUG
> -	bool "Debug PCI Express ASPM"
> -	depends on PCIEASPM
> -	default n
> -	help
> -	  This enables PCI Express ASPM debug support. It will add per-device
> -	  interface to control ASPM.
> -
>  choice
>  	prompt "Default ASPM policy"
>  	default PCIEASPM_DEFAULT
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index c687c817b47d..8ffc13d42baa 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer, const struct kernel_param *kp)
>  module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
>  	NULL, 0644);
>  
> -#ifdef CONFIG_PCIEASPM_DEBUG
>  static ssize_t link_state_show(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
> @@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev)
>  		sysfs_remove_file_from_group(&pdev->dev.kobj,
>  			&dev_attr_clk_ctl.attr, power_group);
>  }
> -#endif
>  
>  static int __init pcie_aspm_disable(char *str)
>  {
> -- 
> 2.17.0.441.gb46fe60e1d-goog
> 

  parent reply	other threads:[~2018-07-27 20:26 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08 23:01 [PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG Rajat Jain
2018-05-09  6:46 ` okaya
2018-05-09  9:43 ` kbuild test robot
2018-05-10 23:34 ` [PATCH] PM / s2idle: Clear the events_check_enabled flag Rajat Jain
2018-05-10 23:36   ` Rajat Jain
2018-05-10 23:39 ` [PATCH v2] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG Rajat Jain
2018-06-05 22:15   ` Rajat Jain
2018-06-09 23:49     ` Sinan Kaya
2018-06-29 23:27   ` Bjorn Helgaas
2018-07-27 20:26   ` Bjorn Helgaas [this message]
2018-07-27 21:03     ` Takashi Iwai
2018-07-29  0:16     ` Sinan Kaya
2018-07-30 14:14       ` Bjorn Helgaas
2018-07-30 16:08         ` Sinan Kaya
2018-07-30  8:32     ` Lukas Wunner
2018-07-30 17:26       ` Bjorn Helgaas
2018-07-30 16:18     ` Rajat Jain
2018-07-31  8:13     ` Pali Rohár

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=20180727202619.GD173328@bhelgaas-glaptop.roam.corp.google.com \
    --to=helgaas@kernel.org \
    --cc=apw@canonical.com \
    --cc=ard.biesheuvel@linaro.org \
    --cc=bhelgaas@google.com \
    --cc=cgarnach@redhat.com \
    --cc=colin.king@canonical.com \
    --cc=fred@fredlawl.com \
    --cc=garsilva@embeddedor.com \
    --cc=keescook@chromium.org \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=mayurkumar.patel@intel.com \
    --cc=okaya@codeaurora.org \
    --cc=pali.rohar@gmail.com \
    --cc=pombredanne@nexb.com \
    --cc=rajatja@google.com \
    --cc=rajatxjain@gmail.com \
    --cc=rhughes@redhat.com \
    --cc=rjw@rjwysocki.net \
    --cc=tiwai@suse.de \
    --cc=vidyas@nvidia.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).