On Tue, Sep 06, 2016 at 12:35:52PM -0600, Jon Derrick wrote:
> AER injecting tests with many devices and nosourceid are resulting in
> soft lockups, mostly due to the config reads, but there's also a
> kmalloc/kfree pair for the aer_err_info struct for each p_device.
>
> When a device emits an error, it's not unreasonable to assume that it
> may emit another error soon. Instead of mallocing the aer error info
> struct each pass through the aer isr, malloc it once per root port and
> hold the reference through the life of the root port. This may save a
> few cycles if there are many devices downstream of the root port and
> no-source-id checking is enabled, disabling the fast path and requiring
> checking all devices for errors.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@xxxxxxxxx>
> ---
> drivers/pci/pcie/aer/aerdrv.c | 1 +
> drivers/pci/pcie/aer/aerdrv.h | 1 +
> drivers/pci/pcie/aer/aerdrv_core.c | 23 +++++++++++++++--------
> 3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer/aerdrv.c b/drivers/pci/pcie/aer/aerdrv.c
> index 48d21e0..dab15d3 100644
> --- a/drivers/pci/pcie/aer/aerdrv.c
> +++ b/drivers/pci/pcie/aer/aerdrv.c
> @@ -286,6 +286,7 @@ static void aer_remove(struct pcie_device *dev)
>
> flush_work(&rpc->dpc_handler);
> aer_disable_rootport(rpc);
> + kfree(rpc->e_info);
> kfree(rpc);
> set_service_data(dev, NULL);
> }
> diff --git a/drivers/pci/pcie/aer/aerdrv.h b/drivers/pci/pcie/aer/aerdrv.h
> index 945c939..2c5a5b8 100644
> --- a/drivers/pci/pcie/aer/aerdrv.h
> +++ b/drivers/pci/pcie/aer/aerdrv.h
> @@ -60,6 +60,7 @@ struct aer_rpc {
> struct pcie_device *rpd; /* Root Port device */
> struct work_struct dpc_handler;
> struct aer_err_source e_sources[AER_ERROR_SOURCES_MAX];
> + struct aer_err_info *e_info;
> unsigned short prod_idx; /* Error Producer Index */
> unsigned short cons_idx; /* Error Consumer Index */
> int isr;
> diff --git a/drivers/pci/pcie/aer/aerdrv_core.c b/drivers/pci/pcie/aer/aerdrv_core.c
> index 521e39c..e1b2e6c 100644
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -715,14 +715,23 @@ static inline void aer_process_err_devices(struct pcie_device *p_device,
> static void aer_isr_one_error(struct pcie_device *p_device,
> struct aer_err_source *e_src)
> {
> - struct aer_err_info *e_info;
> + struct aer_rpc *rpc = get_service_data(p_device);
> + struct aer_err_info *e_info = rpc->e_info;
>
> - /* struct aer_err_info might be big, so we allocate it with slab */
> - e_info = kmalloc(sizeof(struct aer_err_info), GFP_KERNEL);
> + /*
> + * struct aer_err_info might be big, so we allocate it with slab.
> + * It's not unreasonable to assume a faulting device might emit
> + * another error, so try to only malloc once and keep the
> + * reference through the root port's life.
> + */
> if (!e_info) {
> - dev_printk(KERN_DEBUG, &p_device->port->dev,
> - "Can't allocate mem when processing AER errors\n");
> - return;
> + e_info = kmalloc(sizeof(struct aer_err_info), GFP_KERNEL);
> + if (!e_info) {
> + dev_printk(KERN_DEBUG, &p_device->port->dev,
> + "Can't allocate mem when processing AER errors\n");
> + return;
> + }
> + rpc->e_info = e_info;
I like the idea of this. The part I *don't* like is using kmalloc()
in this path.
We've always done this, and this patch means we would only do it the
first time for each device, but the struct aer_rpc (which we allocate
for each device at probe time) is over 900 bytes, while the struct
aer_err_info is only about 70 bytes. Why don't we just include
aer_error_info directly in aer_rpc and allocate the whole shebang once
at probe time? I don't really see what we gain by doing the
allocation in the runtime path.
> }
>
> /*
> @@ -762,8 +771,6 @@ static void aer_isr_one_error(struct pcie_device *p_device,
> if (find_source_device(p_device->port, e_info))
> aer_process_err_devices(p_device, e_info);
> }
> -
> - kfree(e_info);
> }
>
> /**
> --
> 1.8.3.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
