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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 6 Mar 2018 00:17:06 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     "Michael Kelley (EOSG)" <Michael.H.Kelley@...rosoft.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "olaf@...fle.de" <olaf@...fle.de>,
        "apw@...onical.com" <apw@...onical.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "driverdev-devel@...uxdriverproject.org" 
        <driverdev-devel@...uxdriverproject.org>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>,
        "marcelo.cerri@...onical.com" <marcelo.cerri@...onical.com>,
        Jack Morgenstein <jackm@...lanox.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH v2 5/6] PCI: hv: hv_pci_devices_present(): only queue a
 new work when necessary

> 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