From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Zhao Yakui Date: Tue, 18 Dec 2018 09:37:12 +0800 Subject: [PATCH] Kernel/VHM: Refine the usage of spinlock in VHM The spin lock provides the below APIs to acquire spin_lock. spin_lock spin_lock_irq(spin_lock_irqsave) spin_lock_bh Now the spinlock in VHM has some problems. 1.The spinlock is acquired by spin_lock when some spinlock are used in softirq and normal threads. This has the potential risk. So it should be replaced by spin_lock_bh. 2. The spin_lock_irq is used to acquire the spinlock even when it is not used in interrupt context. So spin_lock_irq is replaced by spin_lock_bh. 3. the spinlock in vhm_irqfd is only acquired/released in normal process context. So it is unnecessary to use spin_lock_irq. Tracked-On: projectacrn/acrn-hypervisor#2085 Signed-off-by: Zhao Yakui Reviewed-by: Shuo Liu Reviewed-by: Yin, FengWei Tracked-On: PKT-1617 --- drivers/vhm/vhm_ioreq.c | 41 ++++++++++++++++++----------------------- drivers/vhm/vhm_irqfd.c | 19 +++++++++---------- 2 files changed, 27 insertions(+), 33 deletions(-) diff --git a/drivers/vhm/vhm_ioreq.c b/drivers/vhm/vhm_ioreq.c index 3f8bd4abe757..8b46f43aaec8 100644 --- a/drivers/vhm/vhm_ioreq.c +++ b/drivers/vhm/vhm_ioreq.c @@ -207,7 +207,6 @@ int acrn_ioreq_create_client(unsigned long vmid, ioreq_handler_t handler, { struct vhm_vm *vm; struct ioreq_client *client; - unsigned long flags; int client_id; might_sleep(); @@ -254,9 +253,9 @@ int acrn_ioreq_create_client(unsigned long vmid, ioreq_handler_t handler, init_waitqueue_head(&client->wq); /* When it is added to ioreq_client_list, the refcnt is increased */ - spin_lock_irqsave(&vm->ioreq_client_lock, flags); + spin_lock_bh(&vm->ioreq_client_lock); list_add(&client->list, &vm->ioreq_client_list); - spin_unlock_irqrestore(&vm->ioreq_client_lock, flags); + spin_unlock_bh(&vm->ioreq_client_lock); pr_info("vhm-ioreq: created ioreq client %d\n", client_id); @@ -279,7 +278,7 @@ void acrn_ioreq_clear_request(struct vhm_vm *vm) */ do { - spin_lock(&vm->ioreq_client_lock); + spin_lock_bh(&vm->ioreq_client_lock); list_for_each(pos, &vm->ioreq_client_list) { client = container_of(pos, struct ioreq_client, list); if (vm->ioreq_fallback_client == client->id) @@ -288,7 +287,7 @@ void acrn_ioreq_clear_request(struct vhm_vm *vm) if (has_pending) break; } - spin_unlock(&vm->ioreq_client_lock); + spin_unlock_bh(&vm->ioreq_client_lock); if (has_pending) schedule_timeout_interruptible(HZ / 100); @@ -360,7 +359,6 @@ static void acrn_ioreq_destroy_client_pervm(struct ioreq_client *client, struct vhm_vm *vm) { struct list_head *pos, *tmp; - unsigned long flags; set_bit(IOREQ_CLIENT_DESTROYING, &client->flags); acrn_ioreq_notify_client(client); @@ -368,18 +366,18 @@ static void acrn_ioreq_destroy_client_pervm(struct ioreq_client *client, while (client->vhm_create_kthread && !test_bit(IOREQ_CLIENT_EXIT, &client->flags)) msleep(10); - spin_lock_irqsave(&client->range_lock, flags); + spin_lock_bh(&client->range_lock); list_for_each_safe(pos, tmp, &client->range_list) { struct ioreq_range *range = container_of(pos, struct ioreq_range, list); list_del(&range->list); kfree(range); } - spin_unlock_irqrestore(&client->range_lock, flags); + spin_unlock_bh(&client->range_lock); - spin_lock_irqsave(&vm->ioreq_client_lock, flags); + spin_lock_bh(&vm->ioreq_client_lock); list_del(&client->list); - spin_unlock_irqrestore(&vm->ioreq_client_lock, flags); + spin_unlock_bh(&vm->ioreq_client_lock); if (client->id == vm->ioreq_fallback_client) vm->ioreq_fallback_client = -1; @@ -414,16 +412,15 @@ EXPORT_SYMBOL_GPL(acrn_ioreq_destroy_client); static void __attribute__((unused)) dump_iorange(struct ioreq_client *client) { struct list_head *pos; - unsigned long flags; - spin_lock_irqsave(&client->range_lock, flags); + spin_lock_bh(&client->range_lock); list_for_each(pos, &client->range_list) { struct ioreq_range *range = container_of(pos, struct ioreq_range, list); pr_debug("\tio range: type %d, start 0x%lx, " "end 0x%lx\n", range->type, range->start, range->end); } - spin_unlock_irqrestore(&client->range_lock, flags); + spin_unlock_bh(&client->range_lock); } /* @@ -435,7 +432,6 @@ int acrn_ioreq_add_iorange(int client_id, uint32_t type, { struct ioreq_client *client; struct ioreq_range *range; - unsigned long flags; if (client_id < 0 || client_id >= MAX_CLIENT) { pr_err("vhm-ioreq: no client for id %d\n", client_id); @@ -464,9 +460,9 @@ int acrn_ioreq_add_iorange(int client_id, uint32_t type, range->start = start; range->end = end; - spin_lock_irqsave(&client->range_lock, flags); + spin_lock_bh(&client->range_lock); list_add(&range->list, &client->range_list); - spin_unlock_irqrestore(&client->range_lock, flags); + spin_unlock_bh(&client->range_lock); acrn_ioreq_put_client(client); return 0; @@ -479,7 +475,6 @@ int acrn_ioreq_del_iorange(int client_id, uint32_t type, struct ioreq_client *client; struct ioreq_range *range; struct list_head *pos, *tmp; - unsigned long flags; if (client_id < 0 || client_id >= MAX_CLIENT) { pr_err("vhm-ioreq: no client for id %d\n", client_id); @@ -498,7 +493,7 @@ int acrn_ioreq_del_iorange(int client_id, uint32_t type, might_sleep(); - spin_lock_irqsave(&client->range_lock, flags); + spin_lock_bh(&client->range_lock); list_for_each_safe(pos, tmp, &client->range_list) { range = container_of(pos, struct ioreq_range, list); if (range->type == type) { @@ -516,7 +511,7 @@ int acrn_ioreq_del_iorange(int client_id, uint32_t type, } } } - spin_unlock_irqrestore(&client->range_lock, flags); + spin_unlock_bh(&client->range_lock); acrn_ioreq_put_client(client); return 0; @@ -862,7 +857,7 @@ static struct ioreq_client *acrn_ioreq_find_client_by_request(struct vhm_vm *vm, target_client = 0; fallback_client = 0; - spin_lock(&vm->ioreq_client_lock); + spin_lock_bh(&vm->ioreq_client_lock); list_for_each(pos, &vm->ioreq_client_list) { client = container_of(pos, struct ioreq_client, list); @@ -879,7 +874,7 @@ static struct ioreq_client *acrn_ioreq_find_client_by_request(struct vhm_vm *vm, continue; } - spin_lock(&client->range_lock); + spin_lock_bh(&client->range_lock); list_for_each(range_pos, &client->range_list) { range = container_of(range_pos, struct ioreq_range, list); @@ -889,12 +884,12 @@ static struct ioreq_client *acrn_ioreq_find_client_by_request(struct vhm_vm *vm, break; } } - spin_unlock(&client->range_lock); + spin_unlock_bh(&client->range_lock); if (found) break; } - spin_unlock(&vm->ioreq_client_lock); + spin_unlock_bh(&vm->ioreq_client_lock); if (target_client > 0) return acrn_ioreq_get_client(target_client); diff --git a/drivers/vhm/vhm_irqfd.c b/drivers/vhm/vhm_irqfd.c index b8c122d5ea1f..3eb832114fed 100644 --- a/drivers/vhm/vhm_irqfd.c +++ b/drivers/vhm/vhm_irqfd.c @@ -171,15 +171,14 @@ static void vhm_irqfd_shutdown(struct acrn_vhm_irqfd *irqfd) static void vhm_irqfd_shutdown_work(struct work_struct *work) { - unsigned long flags; struct acrn_vhm_irqfd *irqfd = container_of(work, struct acrn_vhm_irqfd, shutdown); struct vhm_irqfd_info *info = irqfd->info; - spin_lock_irqsave(&info->irqfds_lock, flags); + spin_lock(&info->irqfds_lock); if (vhm_irqfd_is_active(info, irqfd)) vhm_irqfd_shutdown(irqfd); - spin_unlock_irqrestore(&info->irqfds_lock, flags); + spin_unlock(&info->irqfds_lock); } /* @@ -251,19 +250,19 @@ static int acrn_irqfd_assign(struct vhm_irqfd_info *info, init_waitqueue_func_entry(&irqfd->wait, vhm_irqfd_wakeup); init_poll_funcptr(&irqfd->pt, vhm_irqfd_poll_func); - spin_lock_irq(&info->irqfds_lock); + spin_lock(&info->irqfds_lock); list_for_each_entry(tmp, &info->irqfds, list) { if (irqfd->eventfd != tmp->eventfd) continue; /* This fd is used for another irq already. */ ret = -EBUSY; - spin_unlock_irq(&info->irqfds_lock); + spin_unlock(&info->irqfds_lock); goto fail; } list_add_tail(&irqfd->list, &info->irqfds); - spin_unlock_irq(&info->irqfds_lock); + spin_unlock(&info->irqfds_lock); /* Check the pending event in this stage */ events = f.file->f_op->poll(f.file, &irqfd->pt); @@ -294,7 +293,7 @@ static int acrn_irqfd_deassign(struct vhm_irqfd_info *info, if (IS_ERR(eventfd)) return PTR_ERR(eventfd); - spin_lock_irq(&info->irqfds_lock); + spin_lock(&info->irqfds_lock); list_for_each_entry_safe(irqfd, tmp, &info->irqfds, list) { if (irqfd->eventfd == eventfd) { @@ -303,7 +302,7 @@ static int acrn_irqfd_deassign(struct vhm_irqfd_info *info, } } - spin_unlock_irq(&info->irqfds_lock); + spin_unlock(&info->irqfds_lock); eventfd_ctx_put(eventfd); return 0; @@ -371,10 +370,10 @@ void acrn_irqfd_deinit(uint16_t vmid) destroy_workqueue(info->wq); - spin_lock_irq(&info->irqfds_lock); + spin_lock(&info->irqfds_lock); list_for_each_entry_safe(irqfd, tmp, &info->irqfds, list) vhm_irqfd_shutdown(irqfd); - spin_unlock_irq(&info->irqfds_lock); + spin_unlock(&info->irqfds_lock); /* put one more to release it */ put_irqfd_info(info); -- https://clearlinux.org