> 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