iommu/iova: Make the rcache depot properly flexible
Subsystems (2)
Robin Murphy
maintainerv2 cover iommu/iova: Make the rcache depot properly flexible
v1: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
Here's a quick update, cleaning up the silly bugs from v1 and taking John's suggestion to improve the comments.
Cheers,
Robin.
Robin Murphy (2):
iommu/iova: Make the rcache depot scale better
iommu/iova: Manage the depot list size
drivers/iommu/iova.c | 94 ++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 30 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Robin Murphy
maintainerPATCH v2 1/2 iommu/iova: Make the rcache depot scale better
The algorithm in the original paper specifies the storage of full magazines in the depot as an unbounded list rather than a fixed-size array. It turns out to be pretty straightforward to do this in our implementation with no significant loss of efficiency. This allows the depot to scale up to the working set sizes of larger systems, while also potentially saving some memory on smaller ones too.
Since this involves touching struct iova_magazine with the requisite care, we may as well reinforce the comment with a proper assertion too.
---
v2: Fix freeing loops, Improve comment and add matching assertion
drivers/iommu/iova.c | 65 ++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 30 deletions(-)
drivers/iommu/iova.c +35 -30
@@ -622,15 +622,19 @@ EXPORT_SYMBOL_GPL(reserve_iova);/* * As kmalloc's buffer size is fixed to power of 2, 127 is chosen to * assure size of 'iova_magazine' to be 1024 bytes, so that no memory -* will be wasted. +* will be wasted. Since only full magazines are inserted into the depot, +* we don't need to waste PFN capacity on a separate list head either. */ #define IOVA_MAG_SIZE 127 -#define MAX_GLOBAL_MAGS 32/* magazines per bin */ struct iova_magazine { -unsigned long size; +union { +unsigned long size; +struct iova_magazine *next; + };unsigned long pfns[IOVA_MAG_SIZE ]; }; +static_assert (!(sizeof (struct iova_magazine ) & (sizeof (struct iova_magazine ) -1 )));struct iova_cpu_rcache {spinlock_t lock;
@@ -640,8 +644,7 @@ struct iova_cpu_rcache {struct iova_rcache {spinlock_t lock; -unsigned long depot_size; -struct iova_magazine *depot[MAX_GLOBAL_MAGS ]; +struct iova_magazine *depot;struct iova_cpu_rcache __percpu *cpu_rcaches; };
@@ -717,6 +720,21 @@ static void iova_magazine_push(struct iova_magazine *mag, unsigned long pfn) mag->pfns[mag->size++] = pfn; } +static struct iova_magazine *iova_depot_pop (struct iova_rcache *rcache) +{ +struct iova_magazine *mag = rcache->depot; + + rcache->depot = mag->next; + mag->size =IOVA_MAG_SIZE ; +return mag; +} + +static void iova_depot_push (struct iova_rcache *rcache,struct iova_magazine *mag) +{ + mag->next = rcache->depot; + rcache->depot = mag; +} +int iova_domain_init_rcaches (struct iova_domain *iovad) {unsigned int cpu;
@@ -734,7 +752,6 @@ int iova_domain_init_rcaches(struct iova_domain *iovad) rcache = &iovad->rcaches[i];spin_lock_init (&rcache->lock); - rcache->depot_size =0 ; rcache->cpu_rcaches =__alloc_percpu (sizeof (*cpu_rcache),cache_line_size ());if (!rcache->cpu_rcaches) {
@@ -776,7 +793,6 @@ static bool __iova_rcache_insert(struct iova_domain *iovad, struct iova_rcache *rcache, unsigned long iova_pfn) { -struct iova_magazine *mag_to_free =NULL ;struct iova_cpu_rcache *cpu_rcache;bool can_insert = false;unsigned long flags;
@@ -794,12 +810,7 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,if (new_mag) {spin_lock (&rcache->lock); -if (rcache->depot_size <MAX_GLOBAL_MAGS ) { - rcache->depot[rcache->depot_size++] = - cpu_rcache->loaded; - }else { - mag_to_free = cpu_rcache->loaded; - } +iova_depot_push (rcache, cpu_rcache->loaded);spin_unlock (&rcache->lock); cpu_rcache->loaded = new_mag;
@@ -812,11 +823,6 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,spin_unlock_irqrestore (&cpu_rcache->lock, flags); -if (mag_to_free) { -iova_magazine_free_pfns (mag_to_free, iovad); -iova_magazine_free (mag_to_free); - } -return can_insert; }
@@ -854,9 +860,9 @@ static unsigned long __iova_rcache_get(struct iova_rcache *rcache, has_pfn = true; }else {spin_lock (&rcache->lock); -if (rcache->depot_size >0 ) { +if (rcache->depot) {iova_magazine_free (cpu_rcache->loaded); - cpu_rcache->loaded = rcache->depot[--rcache->depot_size]; + cpu_rcache->loaded =iova_depot_pop (rcache); has_pfn = true; }spin_unlock (&rcache->lock);
@@ -895,9 +901,8 @@ static void free_iova_rcaches(struct iova_domain *iovad)struct iova_rcache *rcache;struct iova_cpu_rcache *cpu_rcache;unsigned int cpu; -int i, j; -for (i =0 ; i <IOVA_RANGE_CACHE_MAX_SIZE ; ++i) { +for (int i =0 ; i <IOVA_RANGE_CACHE_MAX_SIZE ; ++i) { rcache = &iovad->rcaches[i];if (!rcache->cpu_rcaches)break ;
@@ -907,8 +912,8 @@ static void free_iova_rcaches(struct iova_domain *iovad)iova_magazine_free (cpu_rcache->prev); }free_percpu (rcache->cpu_rcaches); -for (j =0 ; j < rcache->depot_size; ++j) -iova_magazine_free (rcache->depot[j]); +while (rcache->depot) +iova_magazine_free (iova_depot_pop (rcache)); }kfree (iovad->rcaches);
@@ -942,16 +947,16 @@ static void free_global_cached_iovas(struct iova_domain *iovad) {struct iova_rcache *rcache;unsigned long flags; -int i, j; -for (i =0 ; i <IOVA_RANGE_CACHE_MAX_SIZE ; ++i) { +for (int i =0 ; i <IOVA_RANGE_CACHE_MAX_SIZE ; ++i) { rcache = &iovad->rcaches[i];spin_lock_irqsave (&rcache->lock, flags); -for (j =0 ; j < rcache->depot_size; ++j) { -iova_magazine_free_pfns (rcache->depot[j], iovad); -iova_magazine_free (rcache->depot[j]); +while (rcache->depot) { +struct iova_magazine *mag =iova_depot_pop (rcache); + +iova_magazine_free_pfns (mag, iovad); +iova_magazine_free (mag); } - rcache->depot_size =0 ;spin_unlock_irqrestore (&rcache->lock, flags); } }
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Re: [PATCH v2 1/2] iommu/iova: Make the rcache depot scale better
Quoted 7 lines · Robin Murphy
The algorithm in the original paper specifies the storage of full magazines in the depot as an unbounded list rather than a fixed-size array. It turns out to be pretty straightforward to do this in our implementation with no significant loss of efficiency. This allows the depot to scale up to the working set sizes of larger systems, while also potentially saving some memory on smaller ones too.
Quoted 6 lines
Since this involves touching struct
iova_magazinewith the requisite care, we may as well reinforce the comment with a proper assertion too.Reviewed-by John Garry <[email protected]>Signed-off-by Robin Murphy <[email protected]>---
Robin Murphy
maintainerPATCH v2 2/2 iommu/iova: Manage the depot list size
In reply to Robin MurphyAutomatically scaling the depot up to suit the peak capacity of a workload is all well and good, but it would be nice to have a way to scale it back down again if the workload changes. To that end, add backround reclaim that will gradually free surplus magazines if the depot size remains above a reasonable threshold for long enough.
---
v2: fix reschedule delay typo
drivers/iommu/iova.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
drivers/iommu/iova.c +29 -0
@@ -11,6 +11,7 @@#include <linux/smp.h> #include <linux/bitops.h> #include <linux/cpu.h> +#include <linux/workqueue.h> /* The anchor node sits above the top of the usable address space */ #define IOVA_ANCHOR ~0UL
@@ -627,6 +628,8 @@ EXPORT_SYMBOL_GPL(reserve_iova);*/ #define IOVA_MAG_SIZE 127 +#define IOVA_DEPOT_DELAY msecs_to_jiffies(100) + struct iova_magazine { union { unsigned long size;
@@ -644,8 +647,11 @@ struct iova_cpu_rcache {struct iova_rcache {spinlock_t lock; +unsigned int depot_size;struct iova_magazine *depot;struct iova_cpu_rcache __percpu *cpu_rcaches; +struct iova_domain *iovad; +struct delayed_work work; };static struct iova_magazine *iova_magazine_alloc (gfp_t flags)
@@ -726,6 +732,7 @@ static struct iova_magazine *iova_depot_pop(struct iova_rcache *rcache) rcache->depot = mag->next; mag->size =IOVA_MAG_SIZE ; + rcache->depot_size--;return mag; }
@@ -733,6 +740,24 @@ static void iova_depot_push(struct iova_rcache *rcache, struct iova_magazine *ma { mag->next = rcache->depot; rcache->depot = mag; + rcache->depot_size++; +} + +static void iova_depot_work_func (struct work_struct *work) +{ +struct iova_rcache *rcache =container_of (work,typeof (*rcache), work.work); +struct iova_magazine *mag =NULL ; + +spin_lock (&rcache->lock); +if (rcache->depot_size >num_online_cpus ()) + mag =iova_depot_pop (rcache); +spin_unlock (&rcache->lock); + +if (mag) { +iova_magazine_free_pfns (mag, rcache->iovad); +iova_magazine_free (mag); +schedule_delayed_work (&rcache->work,IOVA_DEPOT_DELAY ); + } }int iova_domain_init_rcaches (struct iova_domain *iovad)
@@ -752,6 +777,8 @@ int iova_domain_init_rcaches(struct iova_domain *iovad) rcache = &iovad->rcaches[i];spin_lock_init (&rcache->lock); + rcache->iovad = iovad; +INIT_DELAYED_WORK (&rcache->work, iova_depot_work_func); rcache->cpu_rcaches =__alloc_percpu (sizeof (*cpu_rcache),cache_line_size ());if (!rcache->cpu_rcaches) {
@@ -812,6 +839,7 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,spin_lock (&rcache->lock);iova_depot_push (rcache, cpu_rcache->loaded);spin_unlock (&rcache->lock); +schedule_delayed_work (&rcache->work,IOVA_DEPOT_DELAY ); cpu_rcache->loaded = new_mag; can_insert = true;
@@ -912,6 +940,7 @@ static void free_iova_rcaches(struct iova_domain *iovad)iova_magazine_free (cpu_rcache->prev); }free_percpu (rcache->cpu_rcaches); +cancel_delayed_work_sync (&rcache->work);while (rcache->depot)iova_magazine_free (iova_depot_pop (rcache)); }
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Re: [PATCH v2 2/2] iommu/iova: Manage the depot list size
Quoted 7 lines · Robin Murphy
Automatically scaling the depot up to suit the peak capacity of a workload is all well and good, but it would be nice to have a way to scale it back down again if the workload changes. To that end, add backround reclaim that will gradually free surplus magazines if the depot size remains above a reasonable threshold for long enough.
Signed-off-by Robin Murphy <[email protected]>
Re: [PATCH v2 2/2] iommu/iova: Manage the depot list size
In reply to Robin MurphyHi Robin,
Quoted 62 lines · Robin Murphy
Automatically scaling the depot up to suit the peak capacity of a workload is all well and good, but it would be nice to have a way to scale it back down again if the workload changes. To that end, add backround reclaim that will gradually free surplus magazines if the depot size remains above a reasonable threshold for long enough.
Signed-off-by Robin Murphy <[email protected]>---
v2: fix reschedule delay typo
drivers/iommu/iova.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)drivers/iommu/iova.c +10
-0@@ -11,6 +11,7 @@#include <linux/smp.h> #include <linux/bitops.h> #include <linux/cpu.h> +#include <linux/workqueue.h> /* The anchor node sits above the top of the usable address space */ #define IOVA_ANCHOR ~0UL@@ -627,6 +628,8 @@ EXPORT_SYMBOL_GPL(reserve_iova);*/ #define IOVA_MAG_SIZE 127 +#define IOVA_DEPOT_DELAY msecs_to_jiffies(100) + struct iova_magazine { union { unsigned long size;@@ -644,8 +647,11 @@ struct iova_cpu_rcache {struct iova_rcache {spinlock_t lock; +unsigned int depot_size;struct iova_magazine *depot;struct iova_cpu_rcache __percpu *cpu_rcaches; +struct iova_domain *iovad; +struct delayed_work work; };static struct iova_magazine *iova_magazine_alloc (gfp_t flags)@@ -726,6 +732,7 @@ static struct iova_magazine *iova_depot_pop(struct iova_rcache *rcache) rcache->depot = mag->next; mag->size =IOVA_MAG_SIZE ; + rcache->depot_size--;return mag; }@@ -733,6 +740,24 @@ static void iova_depot_push(struct iova_rcache *rcache, struct iova_magazine *ma { mag->next = rcache->depot; rcache->depot = mag; + rcache->depot_size++; +} +
drivers/iommu/iova.c static void iova_depot_push(struct iova_rcache *rcache, struct iova_magazine *ma · line 755
Quoted 10 lines
drivers/iommu/iova.c +10
-0+static void iova_depot_work_func (struct work_struct *work) +{ +struct iova_rcache *rcache =container_of (work,typeof (*rcache), work.work); +struct iova_magazine *mag =NULL ; + +spin_lock (&rcache->lock); +if (rcache->depot_size >num_online_cpus ()) + mag =iova_depot_pop (rcache); +spin_unlock (&rcache->lock); +
Running fio test on nvme disk is causing hard LOCKUP issue with the below kernel logs
[ 7620.507689] watchdog: Watchdog detected hard LOCKUP on cpu 334 [ 7620.507694] Modules linked in: nvme_fabrics intel_rapl_msr intel_rapl_common amd64_edac edac_mce_amd kvm_amd kvm rapl wmi_bmof ipmi_ssif binfmt_misc nls_iso8859_1 input_leds joydev acpi_ipmi ipmi_si ccp ipmi_devintf k10temp ipmi_msghandler mac_hid sch_fq_codel dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua msr ramoops reed_solomon pstore_blk pstore_zone efi_pstore ip_tables x_tables autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear hid_generic usbhid ast dax_hmem i2c_algo_bit hid drm_shmem_helper drm_kms_helper cxl_acpi crct10dif_pclmul crc32_pclmul ghash_clmulni_intel sha512_ssse3 aesni_intel crypto_simd cryptd cxl_core nvme ahci drm tg3 nvme_core xhci_pci libahci i2c_piix4 xhci_pci_renesas wmi [ 7620.507795] CPU: 334 PID: 3524 Comm: kworker/334:1 Not tainted 6.5.0-woiovapatchfix #1 [ 7620.507800] Hardware name: AMD Corporation [ 7620.507803] Workqueue: events iova_depot_work_func [ 7620.507813] RIP: 0010:native_queued_spin_lock_slowpath+0x8b/0x300 [ 7620.507823] Code: 24 0f b6 d2 c1 e2 08 30 e4 09 d0 a9 00 01 ff ff 0f 85 f6 01 00 00 85 c0 74 14 41 0f b6 04 24 84 c0 74 0b f3 90 41 0f b6 04 24 <84> c0 75 f5 b8 01 00 00 00 66 41 89 04 24 5b 41 5c 41 5d 41 5e 41 [ 7620.507826] RSP: 0018:ffa000001e450c60 EFLAGS: 00000002 [ 7620.507830] RAX: 0000000000000001 RBX: 000000000000007f RCX: 0000000000000001 [ 7620.507832] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ff11000167b7c000 [ 7620.507834] RBP: ffa000001e450c88 R08: 0000000000000086 R09: ff110055f3bf9800 [ 7620.507836] R10: 0000000000001000 R11: ffa000001e450ff8 R12: ff11000167b7c000 [ 7620.507838] R13: ff110060639dce08 R14: 000000000007fc54 R15: ff110055f3bf9800 [ 7620.507840] FS: 0000000000000000(0000) GS:ff11005ecf380000(0000) knlGS:0000000000000000 [ 7620.507843] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 7620.507846] CR2: 00007f3678bc4010 CR3: 000000016e00c003 CR4: 0000000000771ee0 [ 7620.507848] PKRU: 55555554 [ 7620.507850] Call Trace: [ 7620.507852] <NMI> [ 7620.507857] ? show_regs+0x6e/0x80 [ 7620.507865] ? watchdog_hardlockup_check+0x16e/0x330 [ 7620.507872] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507877] ? watchdog_overflow_callback+0x70/0x80 [ 7620.507883] ? __perf_event_overflow+0x13b/0x2b0 [ 7620.507888] ? x86_perf_event_set_period+0xe7/0x1d0 [ 7620.507895] ? perf_event_overflow+0x1d/0x30 [ 7620.507901] ? amd_pmu_v2_handle_irq+0x158/0x2f0 [ 7620.507911] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507914] ? flush_tlb_one_kernel+0x12/0x30 [ 7620.507920] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507923] ? set_pte_vaddr_p4d+0x5c/0x70 [ 7620.507931] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507933] ? set_pte_vaddr+0x80/0xb0 [ 7620.507938] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507942] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507945] ? native_set_fixmap+0x6d/0x90 [ 7620.507949] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507952] ? ghes_copy_tofrom_phys+0x79/0x120 [ 7620.507960] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507962] ? __ghes_peek_estatus.isra.0+0x4e/0xc0 [ 7620.507968] ? srso_alias_return_thunk+0x5/0x7f [ 7620.507973] ? perf_event_nmi_handler+0x31/0x60 [ 7620.507978] ? nmi_handle+0x68/0x180 [ 7620.507985] ? default_do_nmi+0x45/0x120 [ 7620.507991] ? exc_nmi+0x142/0x1c0 [ 7620.507996] ? end_repeat_nmi+0x16/0x67 [ 7620.508006] ? native_queued_spin_lock_slowpath+0x8b/0x300 [ 7620.508012] ? native_queued_spin_lock_slowpath+0x8b/0x300 [ 7620.508019] ? native_queued_spin_lock_slowpath+0x8b/0x300 [ 7620.508024] </NMI> [ 7620.508026] <IRQ> [ 7620.508029] _raw_spin_lock+0x2d/0x40 [ 7620.508034] free_iova_fast+0x12e/0x1b0 [ 7620.508041] fq_ring_free+0x9b/0x150 [ 7620.508045] ? amd_iommu_unmap_pages+0x49/0x130 [ 7620.508051] iommu_dma_free_iova+0x10c/0x300 [ 7620.508058] __iommu_dma_unmap+0xca/0x140 [ 7620.508067] iommu_dma_unmap_page+0x4f/0xb0 [ 7620.508073] dma_unmap_page_attrs+0x183/0x1c0 [ 7620.508081] nvme_unmap_data+0x105/0x150 [nvme] [ 7620.508095] nvme_pci_complete_batch+0xaf/0xd0 [nvme] [ 7620.508106] nvme_irq+0x7b/0x90 [nvme] [ 7620.508116] ? __pfx_nvme_pci_complete_batch+0x10/0x10 [nvme] [ 7620.508125] __handle_irq_event_percpu+0x50/0x1b0 [ 7620.508133] handle_irq_event+0x3d/0x80 [ 7620.508138] handle_edge_irq+0x90/0x240 [ 7620.508143] __common_interrupt+0x52/0x100 [ 7620.508149] ? srso_alias_return_thunk+0x5/0x7f [ 7620.508153] common_interrupt+0x8d/0xa0 [ 7620.508157] </IRQ> [ 7620.508158] <TASK> [ 7620.508161] asm_common_interrupt+0x2b/0x40 [ 7620.508165] RIP: 0010:iova_depot_work_func+0x28/0xb0 [ 7620.508170] Code: 90 90 f3 0f 1e fa 0f 1f 44 00 00 55 48 89 e5 41 56 4c 8d 77 e0 41 55 41 54 49 89 fc 4c 89 f7 e8 2e 9d 5e 00 8b 05 58 d6 bb 01 <41> 39 44 24 e4 77 14 4c 89 f7 e8 f9 9d 5e 00 41 5c 41 5d 41 5e 5d [ 7620.508173] RSP: 0018:ffa0000025513e30 EFLAGS: 00000246 [ 7620.508176] RAX: 0000000000000200 RBX: ff110001988f4540 RCX: ff11005ecf3b3ca8 [ 7620.508178] RDX: 0000000000000001 RSI: ff110001000420b0 RDI: ff11000167b7c000 [ 7620.508180] RBP: ffa0000025513e48 R08: ff110001000420b0 R09: ff110001988f45c0 [ 7620.508181] R10: 0000000000000007 R11: 0000000000000007 R12: ff11000167b7c020 [ 7620.508183] R13: ff110001001f1600 R14: ff11000167b7c000 R15: ff110001001f1605 [ 7620.508193] process_one_work+0x178/0x350 [ 7620.508202] ? __pfx_worker_thread+0x10/0x10 [ 7620.508207] worker_thread+0x2f7/0x420 [ 7620.508215] ? __pfx_worker_thread+0x10/0x10 [ 7620.508220] kthread+0xf8/0x130 [ 7620.508226] ? __pfx_kthread+0x10/0x10 [ 7620.508231] ret_from_fork+0x3d/0x60 [ 7620.508235] ? __pfx_kthread+0x10/0x10 [ 7620.508240] ret_from_fork_asm+0x1b/0x30 [ 7620.508252] </TASK>
Looking into the trace and your patch figured out that in iova_depot_work_func workqueue function rcache->lock is taken via spin_lock. But the same lock will be taken from IRQ context also. So, to prevent IRQ when the rcache->lock is taken we should disable IRQ.
Therefore use spin_lock_irqsave in place of normal spin_lock.
Below changes fixes the issue
drivers/iommu/iova.c +0 -0
@@ -747,11 +747,12 @@ static void iova_depot_work_func(struct work_struct *work) {
struct iova_rcache *rcache = container_of(work, typeof(*rcache),
work.work); struct iova_magazine *mag = NULL;
+ unsigned long flags;
- spin_lock(&rcache->lock);
+ spin_lock_irqsave(&rcache->lock, flags); if (rcache->depot_size > num_online_cpus()) mag = iova_depot_pop(rcache);
- spin_unlock(&rcache->lock);
+ spin_unlock_irqrestore(&rcache->lock, flags);
if (mag) {
iova_magazine_free_pfns(mag, rcache->iovad);Quoted 33 lines
drivers/iommu/iova.c +9
-0+if (mag) { +iova_magazine_free_pfns (mag, rcache->iovad); +iova_magazine_free (mag); +schedule_delayed_work (&rcache->work,IOVA_DEPOT_DELAY ); + } }int iova_domain_init_rcaches (struct iova_domain *iovad)@@ -752,6 +777,8 @@ int iova_domain_init_rcaches(struct iova_domain *iovad) rcache = &iovad->rcaches[i];spin_lock_init (&rcache->lock); + rcache->iovad = iovad; +INIT_DELAYED_WORK (&rcache->work, iova_depot_work_func); rcache->cpu_rcaches =__alloc_percpu (sizeof (*cpu_rcache),cache_line_size ());if (!rcache->cpu_rcaches) {@@ -812,6 +839,7 @@ static bool __iova_rcache_insert(struct iova_domain *iovad,spin_lock (&rcache->lock);iova_depot_push (rcache, cpu_rcache->loaded);spin_unlock (&rcache->lock); +schedule_delayed_work (&rcache->work,IOVA_DEPOT_DELAY ); cpu_rcache->loaded = new_mag; can_insert = true;@@ -912,6 +940,7 @@ static void free_iova_rcaches(struct iova_domain *iovad)iova_magazine_free (cpu_rcache->prev); }free_percpu (rcache->cpu_rcaches); +cancel_delayed_work_sync (&rcache->work);while (rcache->depot)iova_magazine_free (iova_depot_pop (rcache)); }
Robin Murphy
maintainerRe: [PATCH v2 2/2] iommu/iova: Manage the depot list size
Quoted 7 lines · Dheeraj Kumar
Hi Robin,
Quoted 4 lines · Robin Murphy
Automatically scaling the depot up to suit the peak capacity of a workload is all well and good, but it would be nice to have a way to scale it back down again if the workload changes. To that end, add backround reclaim that will gradually free surplus magazines if the
[ bah, I'll have to fix that typo too - thanks for the squiggle, Thunderbird ]
[...]
Quoted 3 lines
Looking into the trace and your patch figured out that in
iova_depot_work_funcworkqueue functionrcache->lockis taken viaspin_lock. But the same lock will be taken from IRQ context also. So, to prevent IRQ when thercache->lockis taken we should disable IRQ.
Therefore usespin_lock_irqsavein place of normalspin_lock.
Oof, indeed it seems I totally failed to consider IRQs... this tweak looks right to me, thanks for the catch! I'll get a v3 ready for -rc1...
Cheers,
Robin.
Quoted 58 lines
Below changes fixes the issue
drivers/iommu/iova.c +3
-2@@ -747,11 +747,12 @@ static void iova_depot_work_func(struct work_struct *work) { struct iova_rcache *rcache = container_of(work, typeof(*rcache), work.work); struct iova_magazine *mag = NULL; + unsigned long flags; - spin_lock(&rcache->lock); + spin_lock_irqsave(&rcache->lock, flags); if (rcache->depot_size > num_online_cpus()) mag = iova_depot_pop(rcache); - spin_unlock(&rcache->lock); + spin_unlock_irqrestore(&rcache->lock, flags);if (mag) {
iova_magazine_free_pfns(mag, rcache->iovad);Quoted 33 lines
+ if (mag) {
+iova_magazine_free_pfns(mag, rcache->iovad); +iova_magazine_free(mag);
+schedule_delayed_work(&rcache->work, IOVA_DEPOT_DELAY); + }
}
intiova_domain_init_rcaches(struct iova_domain *iovad)(quoted diff) +4
-0@@ -752,6 +777,8 @@ int iova_domain_init_rcaches(struct iova_domain *iovad) rcache = &iovad->rcaches[i]; spin_lock_init(&rcache->lock); + rcache->iovad = iovad; + INIT_DELAYED_WORK(&rcache->work, iova_depot_work_func); rcache->cpu_rcaches = __alloc_percpu(sizeof(*cpu_rcache), cache_line_size()); if (!rcache->cpu_rcaches) {@@ -812,6 +839,7 @@ static bool __iova_rcache_insert(struct iova_domain *iovad, spin_lock(&rcache->lock); iova_depot_push(rcache, cpu_rcache->loaded); spin_unlock(&rcache->lock); + schedule_delayed_work(&rcache->work, IOVA_DEPOT_DELAY); cpu_rcache->loaded = new_mag; can_insert = true;@@ -912,6 +940,7 @@ static void free_iova_rcaches(struct iova_domain *iovad) iova_magazine_free(cpu_rcache->prev); } free_percpu(rcache->cpu_rcaches); + cancel_delayed_work_sync(&rcache->work); while (rcache->depot) iova_magazine_free(iova_depot_pop(rcache));}
Quoted 11 lines · Robin Murphy
v1: https://lore.kernel.org/linux-iommu/[email protected]/
Hi all,
Here's a quick update, cleaning up the silly bugs from v1 and taking John's suggestion to improve the comments.
Cheers,
Robin.
Quoted 7 lines
Robin Murphy (2):
iommu/iova: Make the rcache depot scale better
iommu/iova: Manage the depot list sizedrivers/iommu/iova.c | 94 ++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 30 deletions(-)
Sorry for the delay, I got some trouble with our performance test environment. I had made some performance test for this patches, and the result looks good.
Thanks,
Zekun