From: Dexuan Cui <decui@microsoft.com>
To: Michael Kelley <mikelley@microsoft.com>,
Long Li <longli@microsoft.com>,
"longli@linuxonhyperv.com" <longli@linuxonhyperv.com>,
KY Srinivasan <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
Wei Liu <wei.liu@kernel.org>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Rob Herring <robh@kernel.org>,
Bjorn Helgaas <bhelgaas@google.com>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] PCI: hv: Fix a race condition when removing the device
Date: Thu, 22 Apr 2021 02:31:23 +0000 [thread overview]
Message-ID: <MW2PR2101MB0892B264810E6E6E54A96C4DBF469@MW2PR2101MB0892.namprd21.prod.outlook.com> (raw)
In-Reply-To: <MWHPR21MB15933F28861A2C448C3F233DD7479@MWHPR21MB1593.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Wednesday, April 21, 2021 2:06 PM
> ...
> > Yes I think put_hvpcibus() and get_hvpcibus() can be removed, as we have
> > changed to use
> > a dedicated workqueue for hbus since they were introduced.
> >
> > But we still need to call tasklet_disable/enable() the same way
> > hv_pci_suspend() does, the
> > reason is that we need to protect hbus->state. This value needs to be
> consistent for the
> > driver. For example, a CPU may decide to schedule a work on a work queue
> that we just
> > flushed or destroyed, by reading the wrong hbus->state.
> >
>
> Yes, I would agree the tasklet disable/enable are needed, especially since
> tasklet_disable()
> is what ensures that the tasklet is not currently running.
>
> If the hbus ref counting isn't needed any longer, I would strongly recommend
> adding
> a patch to the series that removes it. This synchronization stuff is hard
> enough to
> understand and reason about; having a leftover mechanism that doesn't really
> do
> anything useful makes it nearly impossible. :-)
>
> Dexuan -- I'm hoping you can take a look as well and see if you agree.
>
> Michael
I also think we can remove the reference counting.
But it looks like there is still race in hv_pci_remove() even with Long's
patch: in hv_pci_remove(), we disable the tasklet, change hbus->state to
hv_pcibus_removing, re-enable the tasklet and flush hbus->wq, and set
hbus->state to hv_pcibus_removed -- what if the channel callback runs
again? -- now hbus->state is no longer hv_pcibus_removing, so
hv_pci_devices_present() -> hv_pci_start_relations_work() and
hv_pci_eject_device() can still add new work items to hbus->wq, and the new
work items may race with the vmbus_close().
It looks like we should remove the state hv_pcibus_removed?
Thanks,
-- Dexuan
next prev parent reply other threads:[~2021-04-22 2:31 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-19 19:20 [PATCH] PCI: hv: Fix a race condition when removing the device longli
2021-04-21 17:33 ` Michael Kelley
2021-04-21 19:56 ` Long Li
2021-04-21 21:05 ` Michael Kelley
2021-04-22 2:31 ` Dexuan Cui [this message]
2021-04-22 3:57 ` Long Li
2021-04-22 4:19 ` Long Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MW2PR2101MB0892B264810E6E6E54A96C4DBF469@MW2PR2101MB0892.namprd21.prod.outlook.com \
--to=decui@microsoft.com \
--cc=bhelgaas@google.com \
--cc=haiyangz@microsoft.com \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=longli@linuxonhyperv.com \
--cc=longli@microsoft.com \
--cc=lorenzo.pieralisi@arm.com \
--cc=mikelley@microsoft.com \
--cc=robh@kernel.org \
--cc=sthemmin@microsoft.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).