| Message ID | 20190410210532.4538-3-logang@deltatee.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix disable_acs_redir parameter | expand |
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@deltatee.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > --- > 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 >
On 2019-04-12 2:44 p.m., Bjorn Helgaas wrote: > 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. That's a reasonable point. I didn't think it was that difficult and the kernel's RCU API is pretty straightforward. But I can resubmit later keeping the spinlock. You're right that it's not that big of a gain. >> 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. Ok, will do. Logan
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;
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. 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. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> Cc: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/pci.c | 89 ++++++++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 36 deletions(-)