From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>,
"julien@xen.org" <julien@xen.org>,
"sstabellini@kernel.org" <sstabellini@kernel.org>,
Oleksandr Tyshchenko <Oleksandr_Tyshchenko@epam.com>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
Artem Mygaiev <Artem_Mygaiev@epam.com>,
"andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
"george.dunlap@citrix.com" <george.dunlap@citrix.com>,
"paul@xen.org" <paul@xen.org>,
Bertrand Marquis <bertrand.marquis@arm.com>,
Rahul Singh <rahul.singh@arm.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v6 03/13] vpci: move lock outside of struct vpci
Date: Mon, 7 Feb 2022 14:27:33 +0000 [thread overview]
Message-ID: <38b779e6-94fc-b54f-39e2-f97739b6846a@epam.com> (raw)
In-Reply-To: <YgEqXhxOqKq92+Xn@Air-de-Roger>
On 07.02.22 16:19, Roger Pau Monné wrote:
> On Mon, Feb 07, 2022 at 01:53:34PM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 07.02.22 14:46, Roger Pau Monné wrote:
>>> On Mon, Feb 07, 2022 at 11:08:39AM +0000, Oleksandr Andrushchenko wrote:
>>>> ======================================
>>>>
>>>> Bottom line:
>>>> ======================================
>>>>
>>>> 1. vpci_{read|write} are not protected with pcidevs_lock and can run in
>>>> parallel with pci_remove_device which can remove pdev after vpci_{read|write}
>>>> acquired the pdev pointer. This may lead to a fail due to pdev dereference.
>>>>
>>>> So, to protect pdev dereference vpci_{read|write} must also use pdevs_lock.
>>> We would like to take the pcidevs_lock only while fetching the device
>>> (ie: pci_get_pdev_by_domain), afterwards it should be fine to lock the
>>> device using a vpci specific lock so calls to vpci_{read,write} can be
>>> partially concurrent across multiple domains.
>> This means this can't be done a pre-req patch, but as a part of the
>> patch which changes locking.
>>> In fact I think Jan had already pointed out that the pci lock would
>>> need taking while searching for the device in vpci_{read,write}.
>> I was referring to the time after we found pdev and it is currently
>> possible to free pdev while using it after the search
>>> It seems to me that if you implement option 3 below taking the
>>> per-domain rwlock in read mode in vpci_{read|write} will already
>>> protect you from the device being removed if the same per-domain lock
>>> is taken in write mode in vpci_remove_device.
>> Yes, it should. Again this can't be done as a pre-req patch because
>> this relies on pdev->vpci_lock
> Hm, no, I don't think so. You could introduce this per-domain rwlock
> in a prepatch, and then move the vpci lock outside of the vpci struct.
> I see no problem with that.
>
>>>> 2. The only offending place which is in the way of pci_dev->vpci_lock is
>>>> modify_bars. If it can be re-worked to track already mapped and unmapped
>>>> regions then we can avoid having a possible deadlock and can use
>>>> pci_dev->vpci_lock (rangesets won't help here as we also need refcounting be
>>>> implemented).
>>> I think a refcounting based solution will be very complex to
>>> implement. I'm however happy to be proven wrong.
>> I can't estimate, but I have a feeling that all these plays around locking
>> is just because of this single piece of code. No other place suffer from
>> pdev->vpci_lock and no d->lock
>>>> If pcidevs_lock is used for vpci_{read|write} then no deadlock is possible,
>>>> but modify_bars code must be re-worked not to lock itself (pdev->vpci_lock and
>>>> tmp->vpci_lock when pdev == tmp, this is minor).
>>> Taking the pcidevs lock (a global lock) is out of the picture IMO, as
>>> it's going to serialize all calls of vpci_{read|write}, and would
>>> create too much contention on the pcidevs lock.
>> I understand that. But if we would like to fix the existing code I see
>> no other alternative.
>>>> 3. We may think about a per-domain rwlock and pdev->vpci_lock, so this solves
>>>> modify_bars's two pdevs access. But this doesn't solve possible pdev
>>>> de-reference in vpci_{read|write} vs pci_remove_device.
>>> pci_remove device will call vpci_remove_device, so as long as
>>> vpci_remove_device taken the per-domain lock in write (exclusive) mode
>>> it should be fine.
>> I think I need to see if there are any other places which similarly
>> require the write lock
>>>> @Roger, @Jan, I would like to hear what do you think about the above analysis
>>>> and how can we proceed with locking re-work?
>>> I think the per-domain rwlock seems like a good option. I would do
>>> that as a pre-patch.
>> It is. But it seems it won't solve the thing we started this adventure for:
>>
>> With per-domain read lock and still ABBA in modify_bars (hope the below
>> is correctly seen with a monospace font):
>>
>> cpu0: vpci_write-> d->RLock -> pdev1->lock -> rom_write -> modify_bars: tmp (pdev2) ->lock
>> cpu1: vpci_write-> d->RLock pdev2->lock -> cmd_write -> modify_bars: tmp (pdev1) ->lock
>>
>> There is no API to upgrade read lock to write lock in modify_bars which could help,
>> so in both cases vpci_write should take write lock.
> I've thought more than once that it would be nice to have a
> write_{upgrade,downgrade} (read_downgrade maybe?) or similar helper.
Yes, this is the real use-case for that
>
> I think you could also drop the read lock, take the write lock and
> check that &pdev->vpci->header == header in order to be sure
> pdev->vpci hasn't been recreated.
And have pdev freed in between....
> You would have to do similar in
> order to get back again from a write lock into a read one.
Not sure this is reliable.
>
> We should avoid taking the rwlock in write mode in vpci_write
> unconditionally.
Yes, but without upgrading the read lock I see no way it can be done
>
> Thanks, Roger.
Thank you,
Oleksandr
next prev parent reply other threads:[~2022-02-07 14:27 UTC|newest]
Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-04 6:34 [PATCH v6 00/13] PCI devices passthrough on Arm, part 3 Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 01/13] xen/pci: arm: add stub for is_memory_hole Oleksandr Andrushchenko
2022-02-04 8:51 ` Julien Grall
2022-02-04 9:01 ` Oleksandr Andrushchenko
2022-02-04 9:41 ` Julien Grall
2022-02-04 9:47 ` Oleksandr Andrushchenko
2022-02-04 9:57 ` Julien Grall
2022-02-04 10:35 ` Oleksandr Andrushchenko
2022-02-04 11:00 ` Julien Grall
2022-02-04 11:25 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 02/13] rangeset: add RANGESETF_no_print flag Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 03/13] vpci: move lock outside of struct vpci Oleksandr Andrushchenko
2022-02-04 7:52 ` Jan Beulich
2022-02-04 8:13 ` Oleksandr Andrushchenko
2022-02-04 8:36 ` Jan Beulich
2022-02-04 8:58 ` Oleksandr Andrushchenko
2022-02-04 9:15 ` Jan Beulich
2022-02-04 10:12 ` Oleksandr Andrushchenko
2022-02-04 10:49 ` Jan Beulich
2022-02-04 11:13 ` Roger Pau Monné
2022-02-04 11:37 ` Jan Beulich
2022-02-04 12:37 ` Oleksandr Andrushchenko
2022-02-04 12:47 ` Jan Beulich
2022-02-04 12:53 ` Oleksandr Andrushchenko
2022-02-04 13:03 ` Jan Beulich
2022-02-04 13:06 ` Roger Pau Monné
2022-02-04 14:43 ` Oleksandr Andrushchenko
2022-02-04 14:57 ` Roger Pau Monné
2022-02-07 11:08 ` Oleksandr Andrushchenko
2022-02-07 12:34 ` Jan Beulich
2022-02-07 12:57 ` Oleksandr Andrushchenko
2022-02-07 13:02 ` Jan Beulich
2022-02-07 12:46 ` Roger Pau Monné
2022-02-07 13:53 ` Oleksandr Andrushchenko
2022-02-07 14:11 ` Jan Beulich
2022-02-07 14:27 ` Roger Pau Monné
2022-02-07 14:33 ` Jan Beulich
2022-02-07 14:35 ` Oleksandr Andrushchenko
2022-02-07 15:11 ` Oleksandr Andrushchenko
2022-02-07 15:26 ` Jan Beulich
2022-02-07 16:07 ` Oleksandr Andrushchenko
2022-02-07 16:15 ` Jan Beulich
2022-02-07 16:21 ` Oleksandr Andrushchenko
2022-02-07 16:37 ` Jan Beulich
2022-02-07 16:44 ` Oleksandr Andrushchenko
2022-02-08 7:35 ` Oleksandr Andrushchenko
2022-02-08 8:57 ` Jan Beulich
2022-02-08 9:03 ` Oleksandr Andrushchenko
2022-02-08 10:50 ` Roger Pau Monné
2022-02-08 11:13 ` Oleksandr Andrushchenko
2022-02-08 13:38 ` Roger Pau Monné
2022-02-08 13:52 ` Oleksandr Andrushchenko
2022-02-08 8:53 ` Jan Beulich
2022-02-08 9:00 ` Oleksandr Andrushchenko
2022-02-08 10:11 ` Roger Pau Monné
2022-02-08 10:32 ` Oleksandr Andrushchenko
2022-02-07 16:08 ` Roger Pau Monné
2022-02-07 16:12 ` Jan Beulich
2022-02-07 14:28 ` Oleksandr Andrushchenko
2022-02-07 14:19 ` Roger Pau Monné
2022-02-07 14:27 ` Oleksandr Andrushchenko [this message]
2022-02-04 11:37 ` Oleksandr Andrushchenko
2022-02-04 12:15 ` Roger Pau Monné
2022-02-04 10:57 ` Roger Pau Monné
2022-02-04 6:34 ` [PATCH v6 04/13] vpci: restrict unhandled read/write operations for guests Oleksandr Andrushchenko
2022-02-04 14:11 ` Jan Beulich
2022-02-04 14:24 ` Oleksandr Andrushchenko
2022-02-08 8:00 ` Oleksandr Andrushchenko
2022-02-08 9:04 ` Jan Beulich
2022-02-08 9:09 ` Oleksandr Andrushchenko
2022-02-08 9:05 ` Roger Pau Monné
2022-02-08 9:10 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 05/13] vpci: add hooks for PCI device assign/de-assign Oleksandr Andrushchenko
2022-02-07 16:28 ` Jan Beulich
2022-02-08 8:32 ` Oleksandr Andrushchenko
2022-02-08 9:13 ` Jan Beulich
2022-02-08 9:27 ` Oleksandr Andrushchenko
2022-02-08 9:44 ` Jan Beulich
2022-02-08 9:55 ` Oleksandr Andrushchenko
2022-02-08 10:09 ` Jan Beulich
2022-02-08 10:22 ` Oleksandr Andrushchenko
2022-02-08 10:29 ` Jan Beulich
2022-02-08 10:52 ` Oleksandr Andrushchenko
2022-02-08 11:00 ` Jan Beulich
2022-02-08 11:25 ` Oleksandr Andrushchenko
2022-02-10 8:21 ` Oleksandr Andrushchenko
2022-02-10 9:22 ` Jan Beulich
2022-02-10 9:33 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 06/13] vpci/header: implement guest BAR register handlers Oleksandr Andrushchenko
2022-02-07 17:06 ` Jan Beulich
2022-02-08 8:06 ` Oleksandr Andrushchenko
2022-02-08 9:16 ` Jan Beulich
2022-02-08 9:29 ` Roger Pau Monné
2022-02-08 9:25 ` Roger Pau Monné
2022-02-08 9:31 ` Oleksandr Andrushchenko
2022-02-08 9:48 ` Jan Beulich
2022-02-08 9:57 ` Oleksandr Andrushchenko
2022-02-08 10:15 ` Jan Beulich
2022-02-08 10:29 ` Oleksandr Andrushchenko
2022-02-08 13:58 ` Roger Pau Monné
2022-02-04 6:34 ` [PATCH v6 07/13] vpci/header: handle p2m range sets per BAR Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 08/13] vpci/header: program p2m with guest BAR view Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 09/13] vpci/header: emulate PCI_COMMAND register for guests Oleksandr Andrushchenko
2022-02-04 14:25 ` Jan Beulich
2022-02-08 8:13 ` Oleksandr Andrushchenko
2022-02-08 9:33 ` Jan Beulich
2022-02-08 9:38 ` Oleksandr Andrushchenko
2022-02-08 9:52 ` Jan Beulich
2022-02-08 9:58 ` Oleksandr Andrushchenko
2022-02-08 11:11 ` Roger Pau Monné
2022-02-08 11:29 ` Oleksandr Andrushchenko
2022-02-08 14:09 ` Roger Pau Monné
2022-02-08 14:13 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 10/13] vpci/header: reset the command register when adding devices Oleksandr Andrushchenko
2022-02-04 14:30 ` Jan Beulich
2022-02-04 14:37 ` Oleksandr Andrushchenko
2022-02-07 7:29 ` Jan Beulich
2022-02-07 11:27 ` Oleksandr Andrushchenko
2022-02-07 12:38 ` Jan Beulich
2022-02-07 12:51 ` Oleksandr Andrushchenko
2022-02-07 12:54 ` Jan Beulich
2022-02-07 14:17 ` Oleksandr Andrushchenko
2022-02-07 14:31 ` Jan Beulich
2022-02-07 14:46 ` Oleksandr Andrushchenko
2022-02-07 15:05 ` Jan Beulich
2022-02-07 15:14 ` Oleksandr Andrushchenko
2022-02-07 15:28 ` Jan Beulich
2022-02-07 15:59 ` Oleksandr Andrushchenko
2022-02-10 12:54 ` Oleksandr Andrushchenko
2022-02-10 13:36 ` Jan Beulich
2022-02-10 13:56 ` Oleksandr Andrushchenko
2022-02-10 12:59 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 11/13] vpci: add initial support for virtual PCI bus topology Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 12/13] xen/arm: translate virtual PCI bus topology for guests Oleksandr Andrushchenko
2022-02-04 7:56 ` Jan Beulich
2022-02-04 8:18 ` Oleksandr Andrushchenko
2022-02-04 6:34 ` [PATCH v6 13/13] xen/arm: account IO handlers for emulated PCI MSI-X Oleksandr Andrushchenko
2022-02-11 15:28 ` Julien Grall
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=38b779e6-94fc-b54f-39e2-f97739b6846a@epam.com \
--to=oleksandr_andrushchenko@epam.com \
--cc=Artem_Mygaiev@epam.com \
--cc=Oleksandr_Tyshchenko@epam.com \
--cc=Volodymyr_Babchuk@epam.com \
--cc=andrew.cooper3@citrix.com \
--cc=bertrand.marquis@arm.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=paul@xen.org \
--cc=rahul.singh@arm.com \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.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).