pci/aer: kmalloc the aer_err_info struct once
RFC 1/1 pci/aer: kmalloc the aer_err_info struct once
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.
--- 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(-)
drivers/pci/pcie/aer/aerdrv.c +1 -0
@@ -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 ); }
drivers/pci/pcie/aer/aerdrv.h +1 -0
@@ -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;
drivers/pci/pcie/aer/aerdrv_core.c +15 -8
@@ -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; }/*
@@ -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); }/**
Signature · 1 line
1.8.3.1
Quoted 64 lines · Jon Derrick
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_infostruct for eachp_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 <[email protected]>--- 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(-)drivers/pci/pcie/aer/aerdrv.c +1
-0@@ -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 ); }drivers/pci/pcie/aer/aerdrv.h +1
-0@@ -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;drivers/pci/pcie/aer/aerdrv_core.c +8
-3@@ -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) {
drivers/pci/pcie/aer/aerdrv_core.c static inline void aer_process_err_devices(struct pcie_device *p_device, · line 734
Quoted 10 lines
drivers/pci/pcie/aer/aerdrv_core.c +7
-3-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.
Quoted 15 lines
drivers/pci/pcie/aer/aerdrv_core.c +0
-2}/* @@ -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); }/** Signature · 1 line
1.8.3.1
Quoted 10 lines
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 structaer_err_infois only about 70 bytes. Why don't we just includeaer_error_infodirectly inaer_rpcand allocate the whole shebang once at probe time? I don't really see what we gain by doing the allocation in the runtime path.
Sounds good! Will follow up with a patch