Re: [Qemu-devel] [RFC PATCH 13/26] cpus: only take BQL for sleeping threads

From: Paolo Bonzini <pbonzini@redhat.com>
To: "Alex Bennée" <alex.bennee@linaro.org>,
	"Pavel Dovgalyuk" <dovgaluk@ispras.ru>
Cc: 'David Hildenbrand' <david@redhat.com>,
	'Pavel Dovgalyuk' <Pavel.Dovgaluk@ispras.ru>,
	qemu-devel@nongnu.org, kwolf@redhat.com,
	'peter maydell' <peter.maydell@linaro.org>,
	'boost lists' <boost.lists@gmail.com>,
	quintela@redhat.com, jasowang@redhat.com, mst@redhat.com,
	zuban32s@gmail.com,
	'maria klimushenkova' <maria.klimushenkova@ispras.ru>,
	kraxel@redhat.com
Subject: Re: [Qemu-devel] [RFC PATCH 13/26] cpus: only take BQL for sleeping threads
Date: Mon, 13 Nov 2017 11:58:45 +0100	[thread overview]
Message-ID: <8d53d934-3e69-cabc-d02a-b395ac7f9bcf@redhat.com> (raw)
In-Reply-To: <87k1yutri5.fsf@linaro.org>

On 13/11/2017 11:14, Alex Bennée wrote:
> 
> Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:
> 
>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> From: "David Hildenbrand" <david@redhat.com>
>>>> On 02.11.2017 12:08, Paolo Bonzini wrote:
>>>>> On 31/10/2017 12:26, Pavel Dovgalyuk wrote:
>>>>>> From: Alex Bennée <alex.bennee@linaro.org>
>>>>>>
>>>>>> Now the only real need to hold the BQL is for when we sleep on the
>>>>>> cpu->halt conditional. The lock is actually dropped while the thread
>>>>>> sleeps so the actual window for contention is pretty small. This also
>>>>>> means we can remove the special case hack for exclusive work and
>>>>>> simply declare that work no longer has an implicit BQL held. This
>>>>>> isn't a major problem async work is generally only changing things in
>>>>>> the context of its own vCPU. If it needs to work across vCPUs it
>>>>>> should be using the exclusive mechanism or possibly taking the lock
>>>>>> itself.
>>>>>>
>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>> Tested-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
>>>>>
>>>>> At least cpu_throttle_thread would fail with this patch.
>>>>>
>>>>> Also I am not sure if the s390 SIGP handlers are ready for this.
>>>>>
>>>>
>>>> We have a global lock to the SIGP "facility". However we need the BQL in
>>>> order to inject interrupts into CPUs (otherwise it would trigger an
>>>> assert when injecting).
>>>>
>>>> We inject Restart and Stop interrupts from run_on_cpu. This requires the
>>>> BQL. So Paolo should be right, this change would break s390x.
>>>
>>> I had some patches to access interrupt_request with the atomic builtins.  If
>>> Pavel can first extract the other changes to the icount mechanism, I can
>>> update them.
>>
>> What changes do you mean here?
>> I'm not sure that I understand clearly how threads interact with BQL.
>> These patches were authored by Alex and we'll have to get him into the
>> discussion.
> 
> Do you want me to re-spin my sub-set of the patches as a new base?

I think the first part to be merged is changes to cpu-exec.c and
friends.  These might even go into 2.11.

Paolo