Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Sergey Fedorov]

On 20/06/16 19:08, Paolo Bonzini wrote:
>>> The patch series changes things in stages.
>>>
>>> First we move the break/watchpoints into an array which is more
>>> amenable to RCU control that the QLIST. We then control the life time
>>> of references to break/watchpoint data by removing long held
>>> references in the target code and getting information when needed from
>>> the core. Then we stop dynamically allocation the watch/breakpoint
>>> data and store it directly in the array which makes iteration across
>>> the list a bit more cache friendly than referenced pointers. Finally
>>> addition and removal of elements of the array is put under RCU
>>> control. This ensures there is always a safe array of data to check
>>> in the run-loop.
>> I a little bit unsure if we really want to complicate things with RCU.
>> Why don't we simply protect the lists with a mutex given that there's no
>> contention expected? BTW, as it comes to debugging, I suppose we don't
>> expect great performance anyway.
> Mutexes do introduce some overhead.  The breakpoints list are mostly touched
> during translation, but watchpoints aren't so we could use tb_lock for
> breakpoints and a separate per-CPU mutex for watchpoints.  That could
> indeed work.

It's a good idea to use tb_lock for breakpoints. Actually, we could use
spinlocks for watchpoints, if we're careful. But I don't this it can be
so critical in performance.

Kind regards,
Sergey

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Sergey Fedorov]

On 20/06/16 19:27, Alex Bennée wrote:
> The watchpoint contention is the biggest one. FWIW I like the RCU
> approach because it is low impact when running (and I'm hoping faster as
> well by not being a linked list).

When can we expect any contention? I generally find the idea of RCU
really great, but in this particular case, I'm afraid, it could give us
less profit (performance) than pain (complexity).

> It's not a major problem in system mode because generally the system is
> halted when changes are made to the list. However I'd like to solve it
> properly for both system and user-mode so I can then forgot about
> another special case.

Could you please explain a bit more about problems in user-mode?

Thanks,
Sergey

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Alex Bennée]

Paolo Bonzini  writes:

>> > The patch series changes things in stages.
>> >
>> > First we move the break/watchpoints into an array which is more
>> > amenable to RCU control that the QLIST. We then control the life time
>> > of references to break/watchpoint data by removing long held
>> > references in the target code and getting information when needed from
>> > the core. Then we stop dynamically allocation the watch/breakpoint
>> > data and store it directly in the array which makes iteration across
>> > the list a bit more cache friendly than referenced pointers. Finally
>> > addition and removal of elements of the array is put under RCU
>> > control. This ensures there is always a safe array of data to check
>> > in the run-loop.
>>
>> I a little bit unsure if we really want to complicate things with RCU.
>> Why don't we simply protect the lists with a mutex given that there's no
>> contention expected? BTW, as it comes to debugging, I suppose we don't
>> expect great performance anyway.
>
> Mutexes do introduce some overhead.  The breakpoints list are mostly touched
> during translation, but watchpoints aren't so we could use tb_lock for
> breakpoints and a separate per-CPU mutex for watchpoints.  That could
> indeed work.

The watchpoint contention is the biggest one. FWIW I like the RCU
approach because it is low impact when running (and I'm hoping faster as
well by not being a linked list).

It's not a major problem in system mode because generally the system is
halted when changes are made to the list. However I'd like to solve it
properly for both system and user-mode so I can then forgot about
another special case.

--
Alex Bennée

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Paolo Bonzini]

> > The patch series changes things in stages.
> >
> > First we move the break/watchpoints into an array which is more
> > amenable to RCU control that the QLIST. We then control the life time
> > of references to break/watchpoint data by removing long held
> > references in the target code and getting information when needed from
> > the core. Then we stop dynamically allocation the watch/breakpoint
> > data and store it directly in the array which makes iteration across
> > the list a bit more cache friendly than referenced pointers. Finally
> > addition and removal of elements of the array is put under RCU
> > control. This ensures there is always a safe array of data to check
> > in the run-loop.
> 
> I a little bit unsure if we really want to complicate things with RCU.
> Why don't we simply protect the lists with a mutex given that there's no
> contention expected? BTW, as it comes to debugging, I suppose we don't
> expect great performance anyway.

Mutexes do introduce some overhead.  The breakpoints list are mostly touched
during translation, but watchpoints aren't so we could use tb_lock for
breakpoints and a separate per-CPU mutex for watchpoints.  That could
indeed work.

Paolo

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Sergey Fedorov]

On 17/06/16 19:33, Alex Bennée wrote:
> Last time I went through the MTTCG code the access to the
> break/watchpoint code was annotated with "RCU?". The code currently
> gets away with avoiding locks for the gdbstub as the guest execution
> state is usually halted. However when used for modelling architectural
> debug registers there is no such protection.

I'm not so sure if there's any architecture which permits changing
breakpoins/watchpoints of one core from another.

> The patch series changes things in stages.
>
> First we move the break/watchpoints into an array which is more
> amenable to RCU control that the QLIST. We then control the life time
> of references to break/watchpoint data by removing long held
> references in the target code and getting information when needed from
> the core. Then we stop dynamically allocation the watch/breakpoint
> data and store it directly in the array which makes iteration across
> the list a bit more cache friendly than referenced pointers. Finally
> addition and removal of elements of the array is put under RCU
> control. This ensures there is always a safe array of data to check
> in the run-loop.

I a little bit unsure if we really want to complicate things with RCU.
Why don't we simply protect the lists with a mutex given that there's no
contention expected? BTW, as it comes to debugging, I suppose we don't
expect great performance anyway.

Kind regards,
Sergey

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Sergey Fedorov]

On 20/06/16 17:25, Paolo Bonzini wrote:
>
> On 20/06/2016 15:55, Sergey Fedorov wrote:
 I'm not sure why you say that arrays are more amenable than QTAILQ
 (though indeed include/qemu/rcu_queue.h only includes QLIST for now),
 but I feel bad asking you to redo all the work...
>> Is there any realistic way to manage *doubly* linked lists in RCU?
> Linux does that fine with circular linked list, but off the top of my
> head I cannot think of any reason why QTAILQ would not work.

Silly me, I see that now :)

Thanks,
Sergey

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Paolo Bonzini]

On 20/06/2016 15:55, Sergey Fedorov wrote:
>> > I'm not sure why you say that arrays are more amenable than QTAILQ
>> > (though indeed include/qemu/rcu_queue.h only includes QLIST for now),
>> > but I feel bad asking you to redo all the work...
> Is there any realistic way to manage *doubly* linked lists in RCU?

Linux does that fine with circular linked list, but off the top of my
head I cannot think of any reason why QTAILQ would not work.

Paolo

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Sergey Fedorov]

On 17/06/16 20:01, Paolo Bonzini wrote:
>
> On 17/06/2016 18:33, Alex Bennée wrote:
>> First we move the break/watchpoints into an array which is more
>> amenable to RCU control that the QLIST. We then control the life time
>> of references to break/watchpoint data by removing long held
>> references in the target code and getting information when needed from
>> the core. Then we stop dynamically allocation the watch/breakpoint
>> data and store it directly in the array which makes iteration across
>> the list a bit more cache friendly than referenced pointers. Finally
>> addition and removal of elements of the array is put under RCU
>> control. This ensures there is always a safe array of data to check
>> in the run-loop.
> I'm not sure why you say that arrays are more amenable than QTAILQ
> (though indeed include/qemu/rcu_queue.h only includes QLIST for now),
> but I feel bad asking you to redo all the work...

Is there any realistic way to manage *doubly* linked lists in RCU?

Kind regards,
Sergey

Re: [Qemu-devel] [RFC 0/7] Safe watch and breakpoint manipulation

[Paolo Bonzini]

On 17/06/2016 18:33, Alex Bennée wrote:
> First we move the break/watchpoints into an array which is more
> amenable to RCU control that the QLIST. We then control the life time
> of references to break/watchpoint data by removing long held
> references in the target code and getting information when needed from
> the core. Then we stop dynamically allocation the watch/breakpoint
> data and store it directly in the array which makes iteration across
> the list a bit more cache friendly than referenced pointers. Finally
> addition and removal of elements of the array is put under RCU
> control. This ensures there is always a safe array of data to check
> in the run-loop.

I'm not sure why you say that arrays are more amenable than QTAILQ
(though indeed include/qemu/rcu_queue.h only includes QLIST for now),
but I feel bad asking you to redo all the work...

Paolo