[PATCH v2 0/2] Fix disable_acs_redir parameter

[PATCH v2 0/2] Fix disable_acs_redir parameter

[Logan Gunthorpe]

Hi Bjorn

This is v2 of my patch to fix the disable_acs_redir paramater.

Per our discussion I've found a much better way which can be
applied to the resource_alignment paramater. Thus, I've included
a second patch that removes the static buffer used there.

I'd appreciate it if the first patch can be applied quickly to
fix the bug I caused. The second patch can go in slowly.

Thanks,

Logan

Logan Gunthorpe (2):
  PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored
  PCI: Clean up resource_alignment parameter to not require static
    buffer

 drivers/pci/pci.c | 96 +++++++++++++++++++++++++++++++----------------
 1 file changed, 64 insertions(+), 32 deletions(-)

--
2.20.1

[PATCH v2 1/2] PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored

[Logan Gunthorpe]

In most cases, kmalloc will not be available early in boot when
pci_setup() is called. Thus, the kstrdup call that was added to fix the
__initdata bug with the disable_acs_redir parameter usually returns
NULL. Thus the parameter is discarded and it does not take into effect.

To fix this, we store the string that's in initdata until a initcall
function can allocate the memory appropriately. This way we
don't need any additional static memory.

Fixes: d2fd6e81912a ("PCI: Fix __initdata issue with "pci=disable_acs_redir" parameter")
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7c1b362f599a..766f5779db92 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6262,8 +6262,7 @@ static int __init pci_setup(char *str)
 			} else if (!strncmp(str, "pcie_scan_all", 13)) {
 				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
 			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
-				disable_acs_redir_param =
-					kstrdup(str + 18, GFP_KERNEL);
+				disable_acs_redir_param = str + 18;
 			} else {
 				printk(KERN_ERR "PCI: Unknown option `%s'\n",
 						str);
@@ -6274,3 +6273,19 @@ static int __init pci_setup(char *str)
 	return 0;
 }
 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.
+ */
+static int __init pci_realloc_setup_params(void)
+{
+	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
+
+	return 0;
+}
+pure_initcall(pci_realloc_setup_params);
-- 
2.20.1

Re: [PATCH v2 1/2] PCI: Fix issue with "pci=disable_acs_redir" parameter being ignored

[Bjorn Helgaas]

On Wed, Apr 10, 2019 at 03:05:31PM -0600, Logan Gunthorpe wrote:
> In most cases, kmalloc will not be available early in boot when
> pci_setup() is called. Thus, the kstrdup call that was added to fix the
> __initdata bug with the disable_acs_redir parameter usually returns
> NULL. Thus the parameter is discarded and it does not take into effect.
> 
> To fix this, we store the string that's in initdata until a initcall
> function can allocate the memory appropriately. This way we
> don't need any additional static memory.
> 
> Fixes: d2fd6e81912a ("PCI: Fix __initdata issue with "pci=disable_acs_redir" parameter")
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>

Applied to for-linus for v5.1, thanks, Logan!

> ---
>  drivers/pci/pci.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7c1b362f599a..766f5779db92 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6262,8 +6262,7 @@ static int __init pci_setup(char *str)
>  			} else if (!strncmp(str, "pcie_scan_all", 13)) {
>  				pci_add_flags(PCI_SCAN_ALL_PCIE_DEVS);
>  			} else if (!strncmp(str, "disable_acs_redir=", 18)) {
> -				disable_acs_redir_param =
> -					kstrdup(str + 18, GFP_KERNEL);
> +				disable_acs_redir_param = str + 18;
>  			} else {
>  				printk(KERN_ERR "PCI: Unknown option `%s'\n",
>  						str);
> @@ -6274,3 +6273,19 @@ static int __init pci_setup(char *str)
>  	return 0;
>  }
>  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.
> + */
> +static int __init pci_realloc_setup_params(void)
> +{
> +	disable_acs_redir_param = kstrdup(disable_acs_redir_param, GFP_KERNEL);
> +
> +	return 0;
> +}
> +pure_initcall(pci_realloc_setup_params);
> -- 
> 2.20.1
> 

[PATCH v2 2/2] PCI: Clean up resource_alignment parameter to not require static buffer

[Logan Gunthorpe]

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(-)

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

Re: [PATCH v2 2/2] PCI: Clean up resource_alignment parameter to not require static buffer

[Bjorn Helgaas]

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
> 

Re: [PATCH v2 2/2] PCI: Clean up resource_alignment parameter to not require static buffer

[Logan Gunthorpe]

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