On Wed, Apr 10, 2019 at 03:05:32PM -0600, Logan Gunthorpe wrote:
> Clean up the 'resource_alignment' parameter code to use kstrdup
> in the initcall routine instead of a static buffer that wastes memory
> regardless of whether the feature is used. This allows us to drop
> 'COMMAND_LINE_SIZE' bytes (typically 256-4096 depending on architecture)
> of static data.
>
> This is similar to what has been done for the 'disable_acs_redir'
> parameter.
>
> This conversion also allows us to use RCU instead of the spinlock to
> deal with the concurrency issue which further reduces memory usage.
I'm unconvinced about this part. Spinlocks are CS 101 material and
I'm a little hesitant to use a graduate-level technique like RCU in a
case where it doesn't really buy us much -- we don't need the
performance advantage and the size advantage seems minimal. But I'm
an RCU ignoramus and maybe need to be educated.
> As part of the clean up we also squash pci_get_resource_alignment_param()
> into resource_alignment_show() and pci_set_resource_alignment_param()
> into resource_alignment_store() seeing these functions only had one
> caller and the show/store wrappers were needlessly thin.
Squashing makes sense and would be nice as a separate patch.
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
> drivers/pci/pci.c | 89 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 53 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 766f5779db92..13767c2409ae 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5896,9 +5896,7 @@ resource_size_t __weak pcibios_default_alignment(void)
> return 0;
> }
>
> -#define RESOURCE_ALIGNMENT_PARAM_SIZE COMMAND_LINE_SIZE
> -static char resource_alignment_param[RESOURCE_ALIGNMENT_PARAM_SIZE] = {0};
> -static DEFINE_SPINLOCK(resource_alignment_lock);
> +static const char __rcu *resource_alignment_param;
>
> /**
> * pci_specified_resource_alignment - get resource alignment specified by user.
> @@ -5916,9 +5914,9 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> const char *p;
> int ret;
>
> - spin_lock(&resource_alignment_lock);
> - p = resource_alignment_param;
> - if (!*p && !align)
> + rcu_read_lock();
> + p = rcu_dereference(resource_alignment_param);
> + if (!p)
> goto out;
> if (pci_has_flag(PCI_PROBE_ONLY)) {
> align = 0;
> @@ -5956,7 +5954,7 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> p++;
> }
> out:
> - spin_unlock(&resource_alignment_lock);
> + rcu_read_unlock();
> return align;
> }
>
> @@ -6082,35 +6080,48 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> }
> }
>
> -static ssize_t pci_set_resource_alignment_param(const char *buf, size_t count)
> +static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
> {
> - if (count > RESOURCE_ALIGNMENT_PARAM_SIZE - 1)
> - count = RESOURCE_ALIGNMENT_PARAM_SIZE - 1;
> - spin_lock(&resource_alignment_lock);
> - strncpy(resource_alignment_param, buf, count);
> - resource_alignment_param[count] = '\0';
> - spin_unlock(&resource_alignment_lock);
> - return count;
> -}
> + const char *p;
> + size_t count = 0;
>
> -static ssize_t pci_get_resource_alignment_param(char *buf, size_t size)
> -{
> - size_t count;
> - spin_lock(&resource_alignment_lock);
> - count = snprintf(buf, size, "%s", resource_alignment_param);
> - spin_unlock(&resource_alignment_lock);
> - return count;
> -}
> + rcu_read_lock();
> + p = rcu_dereference(resource_alignment_param);
> + if (!p)
> + goto out;
>
> -static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
> -{
> - return pci_get_resource_alignment_param(buf, PAGE_SIZE);
> + count = snprintf(buf, PAGE_SIZE, "%s", p);
> +
> + /*
> + * When set by the command line there will not be a
> + * line feed, which is ugly. So conditionally add it here.
> + */
> + if (buf[count - 2] != '\n' && count < PAGE_SIZE - 1) {
> + buf[count - 1] = '\n';
> + buf[count++] = 0;
> + }
> +
> +out:
> + rcu_read_unlock();
> +
> + return count;
> }
>
> static ssize_t resource_alignment_store(struct bus_type *bus,
> const char *buf, size_t count)
> {
> - return pci_set_resource_alignment_param(buf, count);
> + const char *p, *old;
> +
> + p = kstrndup(buf, count, GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> +
> + old = rcu_dereference_protected(resource_alignment_param, 1);
> + rcu_assign_pointer(resource_alignment_param, p);
> + synchronize_rcu();
> + kfree(old);
> +
> + return count;
> }
>
> static BUS_ATTR_RW(resource_alignment);
> @@ -6238,8 +6249,8 @@ static int __init pci_setup(char *str)
> } else if (!strncmp(str, "cbmemsize=", 10)) {
> pci_cardbus_mem_size = memparse(str + 10, &str);
> } else if (!strncmp(str, "resource_alignment=", 19)) {
> - pci_set_resource_alignment_param(str + 19,
> - strlen(str + 19));
> + RCU_INIT_POINTER(resource_alignment_param,
> + str + 19);
> } else if (!strncmp(str, "ecrc=", 5)) {
> pcie_ecrc_get_policy(str + 5);
> } else if (!strncmp(str, "hpiosize=", 9)) {
> @@ -6275,15 +6286,21 @@ static int __init pci_setup(char *str)
> early_param("pci", pci_setup);
>
> /*
> - * 'disable_acs_redir_param' is initialized in pci_setup(), above, to point
> - * to data in the __initdata section which will be freed after the init
> - * sequence is complete. We can't allocate memory in pci_setup() because some
> - * architectures do not have any memory allocation service available during
> - * an early_param() call. So we allocate memory and copy the variable here
> - * before the init section is freed.
> + * 'resource_alignment_param' and 'disable_acs_redir_param' are initialized
> + * in pci_setup(), above, to point to data in the __initdata section which
> + * will be freed after the init sequence is complete. We can't allocate memory
> + * in pci_setup() because some architectures do not have any memory allocation
> + * service available during an early_param() call. So we allocate memory and
> + * copy the variable here before the init section is freed.
> */
> static int __init pci_realloc_setup_params(void)
> {
> + const char *p;
> +
> + p = rcu_dereference_protected(resource_alignment_param, 1);
> + p = kstrdup(p, GFP_KERNEL);
> + rcu_assign_pointer(resource_alignment_param, p);
> +
> disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
>
> return 0;
> --
> 2.20.1
>
