From: Michael Kelley (EOSG) Sent: Monday, March 5, 2018 15:48
@@ -1756,11 +1757,23 @@ static void hv_pci_devices_present(struct
hv_pcibus_device
*hbus, }
spin_lock_irqsave(&hbus->device_list_lock, flags);
- /*
* If pending_dr is true, we have already queued a work,* which will see the new dr. Otherwise, we need to* queue a new work.*/- pending_dr = !list_empty(&hbus->dr_list); list_add_tail(&dr->list_entry, &hbus->dr_list);
- spin_unlock_irqrestore(&hbus->device_list_lock, flags);
A minor point: The spin_unlock_irqrestore() call can stay here. Once we have the list status in a local variable and the new entry is added to the list, nothing bad can happen if we drop the spin lock. At worst, and very unlikely, we'll queue work when some other thread has already queued work to process the list entry, but that's no big deal. I'd argue for keeping the code covered by a spin lock as small as possible.
Michael
I agree. Will fix this in v3.
- get_hvpcibus(hbus);
- queue_work(hbus->wq, &dr_wrk->wrk);
- if (pending_dr) {
kfree(dr_wrk);- } else {
get_hvpcibus(hbus);queue_work(hbus->wq, &dr_wrk->wrk);- }
- spin_unlock_irqrestore(&hbus->device_list_lock, flags);
}
To receive more comments from others, I'll hold off v3 until tomorrow.
Thanks, -- Dexuan