RE: [PATCH v2 5/6] PCI: hv: hv_pci_devices_present(): only queue a new work when necessary

[Dexuan Cui]

> 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