From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
George Dunlap <George.Dunlap@eu.citrix.com>,
Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
Stefano Stabellini <sstabellini@kernel.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 4/5] evtchn: convert domain event lock to an r/w one
Date: Wed, 23 Dec 2020 13:57:55 +0100 [thread overview]
Message-ID: <d0b3079b-ae83-a14e-1fc6-ea76bdc7db79@suse.com> (raw)
In-Reply-To: <aa169dc2-77f2-b3e9-80f4-d5f4d6ea54f1@xen.org>
On 23.12.2020 12:22, Julien Grall wrote:
> Hi Jan,
>
> On 22/12/2020 09:46, Jan Beulich wrote:
>> On 21.12.2020 18:45, Julien Grall wrote:
>>> On 14/12/2020 09:40, Jan Beulich wrote:
>>>> On 11.12.2020 11:57, Julien Grall wrote:
>>>>> On 11/12/2020 10:32, Jan Beulich wrote:
>>>>>> On 09.12.2020 12:54, Julien Grall wrote:
>>>>>>> On 23/11/2020 13:29, Jan Beulich wrote:
>>>>>>>> @@ -620,7 +620,7 @@ int evtchn_close(struct domain *d1, int
>>>>>>>> long rc = 0;
>>>>>>>>
>>>>>>>> again:
>>>>>>>> - spin_lock(&d1->event_lock);
>>>>>>>> + write_lock(&d1->event_lock);
>>>>>>>>
>>>>>>>> if ( !port_is_valid(d1, port1) )
>>>>>>>> {
>>>>>>>> @@ -690,13 +690,11 @@ int evtchn_close(struct domain *d1, int
>>>>>>>> BUG();
>>>>>>>>
>>>>>>>> if ( d1 < d2 )
>>>>>>>> - {
>>>>>>>> - spin_lock(&d2->event_lock);
>>>>>>>> - }
>>>>>>>> + read_lock(&d2->event_lock);
>>>>>>>
>>>>>>> This change made me realized that I don't quite understand how the
>>>>>>> rwlock is meant to work for event_lock. I was actually expecting this to
>>>>>>> be a write_lock() given there are state changed in the d2 events.
>>>>>>
>>>>>> Well, the protection needs to be against racing changes, i.e.
>>>>>> parallel invocations of this same function, or evtchn_close().
>>>>>> It is debatable whether evtchn_status() and
>>>>>> domain_dump_evtchn_info() would better also be locked out
>>>>>> (other read_lock() uses aren't applicable to interdomain
>>>>>> channels).
>>>>>>
>>>>>>> Could you outline how a developper can find out whether he/she should
>>>>>>> use read_lock or write_lock?
>>>>>>
>>>>>> I could try to, but it would again be a port type dependent
>>>>>> model, just like for the per-channel locks.
>>>>>
>>>>> It is quite important to have clear locking strategy (in particular
>>>>> rwlock) so we can make correct decision when to use read_lock or write_lock.
>>>>>
>>>>>> So I'd like it to
>>>>>> be clarified first whether you aren't instead indirectly
>>>>>> asking for these to become write_lock()
>>>>>
>>>>> Well, I don't understand why this is a read_lock() (even with your
>>>>> previous explanation). I am not suggesting to switch to a write_lock(),
>>>>> but instead asking for the reasoning behind the decision.
>>>>
>>>> So if what I've said in my previous reply isn't enough (including the
>>>> argument towards using two write_lock() here), I'm struggling to
>>>> figure what else to say. The primary goal is to exclude changes to
>>>> the same ports. For this it is sufficient to hold just one of the two
>>>> locks in writer mode, as the other (racing) one will acquire that
>>>> same lock for at least reading. The question whether both need to use
>>>> writer mode can only be decided when looking at the sites acquiring
>>>> just one of the locks in reader mode (hence the reference to
>>>> evtchn_status() and domain_dump_evtchn_info()) - if races with them
>>>> are deemed to be a problem, switching to both-writers will be needed.
>>>
>>> I had another look at the code based on your explanation. I don't think
>>> it is fine to allow evtchn_status() to be concurrently called with
>>> evtchn_close().
>>>
>>> evtchn_close() contains the following code:
>>>
>>> chn2->state = ECS_UNBOUND;
>>> chn2->u.unbound.remote_domid = d1->domain_id;
>>>
>>> Where chn2 is a event channel of the remote domain (d2). Your patch will
>>> only held the read lock for d2.
>>>
>>> However evtchn_status() expects the event channel state to not change
>>> behind its back. This assumption doesn't hold for d2, and you could
>>> possibly end up to see the new value of chn2->state after the new
>>> chn2->u.unbound.remote_domid.
>>>
>>> Thanksfully, it doesn't look like chn2->u.interdomain.remote_domain
>>> would be overwritten. Otherwise, this would be a straight dereference of
>>> an invalid pointer.
>>>
>>> So I think, we need to held the write event lock for both domain.
>>
>> Well, okay. Three considerations though:
>>
>> 1) Neither evtchn_status() nor domain_dump_evtchn_info() appear to
>> have a real need to acquire the per-domain lock. They could as well
>> acquire the per-channel ones. (In the latter case this will then
>> also allow inserting the so far missing process_pending_softirqs()
>> call; it shouldn't be made with a lock held.)
> I agree that evtchn_status() doesn't need to acquire the per-domain
> lock. I am not entirely sure about domain_dump_evtchn_info() because
> AFAICT the PIRQ tree (used by domain_pirq_to_irq()) is protected with
> d->event_lock.
It is, but calling it without the lock just to display the IRQ
is not a problem afaict.
>> 3) With the per-channel double locking and with 1) addressed I
>> can't really see the need for the double per-domain locking in
>> evtchn_bind_interdomain() and evtchn_close(). The write lock is
>> needed for the domain allocating a new port or freeing one. But why
>> is there any need for holding the remote domain's lock, when its
>> side of the channel gets guarded by the per-channel lock anyway?
>
> If 1) is addressed, then I think it should be fine to just acquire the
> read event lock of the remote domain.
For bind-interdomain I've eliminated the double locking, so the
question goes away there altogether. While for close I thought
I had managed to eliminate it too, the change looks to be
causing a deadlock of some sort, which I'll have to figure out.
However, the change might be controversial anyway, because I
need to play games already prior to fixing that bug ...
All of this said - for the time being it'll be both write_lock()
in evtchn_close(), as I consider it risky to make the remote one
a read_lock() merely based on the observation that there is
currently (i.e. with 1) addressed) no conflict.
Jan
next prev parent reply other threads:[~2020-12-23 12:58 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-23 13:26 [PATCH v3 0/5] evtchn: (not so) recent XSAs follow-on Jan Beulich
2020-11-23 13:28 ` [PATCH v3 1/5] evtchn: drop acquiring of per-channel lock from send_guest_{global,vcpu}_virq() Jan Beulich
2020-12-02 19:03 ` Julien Grall
2020-12-03 9:46 ` Jan Beulich
2020-12-09 9:53 ` Julien Grall
2020-12-09 14:24 ` Jan Beulich
2020-11-23 13:28 ` [PATCH v3 2/5] evtchn: avoid access tearing for ->virq_to_evtchn[] accesses Jan Beulich
2020-12-02 21:14 ` Julien Grall
2020-11-23 13:28 ` [PATCH v3 3/5] evtchn: convert vIRQ lock to an r/w one Jan Beulich
2020-12-09 11:16 ` Julien Grall
2020-11-23 13:29 ` [PATCH v3 4/5] evtchn: convert domain event " Jan Beulich
2020-12-09 11:54 ` Julien Grall
2020-12-11 10:32 ` Jan Beulich
2020-12-11 10:57 ` Julien Grall
2020-12-14 9:40 ` Jan Beulich
2020-12-21 17:45 ` Julien Grall
2020-12-22 9:46 ` Jan Beulich
2020-12-23 11:22 ` Julien Grall
2020-12-23 12:57 ` Jan Beulich [this message]
2020-12-23 13:19 ` Julien Grall
2020-12-23 13:36 ` Jan Beulich
2020-11-23 13:30 ` [PATCH v3 5/5] evtchn: don't call Xen consumer callback with per-channel lock held Jan Beulich
2020-11-30 10:39 ` Isaila Alexandru
2020-12-02 21:10 ` Julien Grall
2020-12-03 10:09 ` Jan Beulich
2020-12-03 14:40 ` Tamas K Lengyel
2020-12-04 11:28 ` Julien Grall
2020-12-04 11:48 ` Jan Beulich
2020-12-04 11:51 ` Julien Grall
2020-12-04 12:01 ` Jan Beulich
2020-12-04 15:09 ` Julien Grall
2020-12-07 8:02 ` Jan Beulich
2020-12-07 17:22 ` Julien Grall
2020-12-04 15:21 ` Tamas K Lengyel
2020-12-04 15:29 ` Julien Grall
2020-12-04 19:15 ` Tamas K Lengyel
2020-12-04 19:22 ` Julien Grall
2020-12-04 21:23 ` Tamas K Lengyel
2020-12-07 15:28 ` Jan Beulich
2020-12-07 17:30 ` Julien Grall
2020-12-07 17:35 ` Tamas K Lengyel
2020-12-23 13:12 ` Jan Beulich
2020-12-23 13:33 ` Julien Grall
2020-12-23 13:41 ` Jan Beulich
2020-12-23 14:44 ` Julien Grall
2020-12-23 14:56 ` Jan Beulich
2020-12-23 15:08 ` Julien Grall
2020-12-23 15:15 ` Tamas K Lengyel
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=d0b3079b-ae83-a14e-1fc6-ea76bdc7db79@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=iwj@xenproject.org \
--cc=julien@xen.org \
--cc=sstabellini@kernel.org \
--cc=wl@xen.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).