[v5] coroutine rwlock downgrade fix, minor VDI changes

[PATCH v5 0/6] coroutine rwlock downgrade fix, minor VDI changes

Posted by Paolo Bonzini 4 years, 8 months ago

This is a resubmit of David Edmondson's series at
https://patchew.org/QEMU/20210309144015.557477-1-david.edmondson@oracle.com/.
After closer analysis on IRC, the CoRwlock's attempt to ensure
fairness turned out to be flawed.  Therefore, this series
reimplements CoRwlock without using a CoQueue.  Tracking whether
each queued coroutine is a reader/writer makes it possible to
never wake a writer when only readers should be allowed and
vice versa.

v2->v3: new CoRwlock implementation

v3->v4: fix upgrade and add a test for that, too

v4->v5: typo

David Edmondson (4):
  block/vdi: When writing new bmap entry fails, don't leak the buffer
  block/vdi: Don't assume that blocks are larger than VdiHeader
  coroutine/mutex: Store the coroutine in the CoWaitRecord only once
  test-coroutine: Add rwlock downgrade test

Paolo Bonzini (2):
  coroutine-lock: reimplement CoRwlock to fix downgrade bug
  test-coroutine: add rwlock upgrade test

 block/vdi.c                 |  11 ++-
 include/qemu/coroutine.h    |  17 ++--
 tests/unit/test-coroutine.c | 161 ++++++++++++++++++++++++++++++++++++
 util/qemu-coroutine-lock.c  | 149 +++++++++++++++++++++------------
 4 files changed, 274 insertions(+), 64 deletions(-)

-- 
2.29.2

Re: [PATCH v5 0/6] coroutine rwlock downgrade fix, minor VDI changes

Posted by Max Reitz 4 years, 7 months ago

On 17.03.21 19:00, Paolo Bonzini wrote:
> This is a resubmit of David Edmondson's series at
> https://patchew.org/QEMU/20210309144015.557477-1-david.edmondson@oracle.com/.
> After closer analysis on IRC, the CoRwlock's attempt to ensure
> fairness turned out to be flawed.  Therefore, this series
> reimplements CoRwlock without using a CoQueue.  Tracking whether
> each queued coroutine is a reader/writer makes it possible to
> never wake a writer when only readers should be allowed and
> vice versa.

I’m not a maintainer for patches 3 through 6.  Stefan is, though.  So 
Stefan, can you take this series with an R-b from me on 1 and 2?

Max

Re: [PATCH v5 0/6] coroutine rwlock downgrade fix, minor VDI changes

Posted by Stefan Hajnoczi 4 years, 7 months ago

On Wed, Mar 17, 2021 at 07:00:07PM +0100, Paolo Bonzini wrote:
> This is a resubmit of David Edmondson's series at
> https://patchew.org/QEMU/20210309144015.557477-1-david.edmondson@oracle.com/.
> After closer analysis on IRC, the CoRwlock's attempt to ensure
> fairness turned out to be flawed.  Therefore, this series
> reimplements CoRwlock without using a CoQueue.  Tracking whether
> each queued coroutine is a reader/writer makes it possible to
> never wake a writer when only readers should be allowed and
> vice versa.
> 
> v2->v3: new CoRwlock implementation
> 
> v3->v4: fix upgrade and add a test for that, too
> 
> v4->v5: typo
> 
> David Edmondson (4):
>   block/vdi: When writing new bmap entry fails, don't leak the buffer
>   block/vdi: Don't assume that blocks are larger than VdiHeader
>   coroutine/mutex: Store the coroutine in the CoWaitRecord only once
>   test-coroutine: Add rwlock downgrade test
> 
> Paolo Bonzini (2):
>   coroutine-lock: reimplement CoRwlock to fix downgrade bug
>   test-coroutine: add rwlock upgrade test
> 
>  block/vdi.c                 |  11 ++-
>  include/qemu/coroutine.h    |  17 ++--
>  tests/unit/test-coroutine.c | 161 ++++++++++++++++++++++++++++++++++++
>  util/qemu-coroutine-lock.c  | 149 +++++++++++++++++++++------------
>  4 files changed, 274 insertions(+), 64 deletions(-)

I had questions about the rwlock implementation. The other patches look
ready to go.

Stefan

Re: [PATCH v5 0/6] coroutine rwlock downgrade fix, minor VDI changes

Posted by Paolo Bonzini 4 years, 7 months ago

On 24/03/21 17:23, Stefan Hajnoczi wrote:
> On Wed, Mar 17, 2021 at 07:00:07PM +0100, Paolo Bonzini wrote:
>> This is a resubmit of David Edmondson's series at
>> https://patchew.org/QEMU/20210309144015.557477-1-david.edmondson@oracle.com/.
>> After closer analysis on IRC, the CoRwlock's attempt to ensure
>> fairness turned out to be flawed.  Therefore, this series
>> reimplements CoRwlock without using a CoQueue.  Tracking whether
>> each queued coroutine is a reader/writer makes it possible to
>> never wake a writer when only readers should be allowed and
>> vice versa.
>>
>> v2->v3: new CoRwlock implementation
>>
>> v3->v4: fix upgrade and add a test for that, too
>>
>> v4->v5: typo
>>
>> David Edmondson (4):
>>    block/vdi: When writing new bmap entry fails, don't leak the buffer
>>    block/vdi: Don't assume that blocks are larger than VdiHeader
>>    coroutine/mutex: Store the coroutine in the CoWaitRecord only once
>>    test-coroutine: Add rwlock downgrade test
>>
>> Paolo Bonzini (2):
>>    coroutine-lock: reimplement CoRwlock to fix downgrade bug
>>    test-coroutine: add rwlock upgrade test
>>
>>   block/vdi.c                 |  11 ++-
>>   include/qemu/coroutine.h    |  17 ++--
>>   tests/unit/test-coroutine.c | 161 ++++++++++++++++++++++++++++++++++++
>>   util/qemu-coroutine-lock.c  | 149 +++++++++++++++++++++------------
>>   4 files changed, 274 insertions(+), 64 deletions(-)
> 
> I had questions about the rwlock implementation. The other patches look
> ready to go.

Cool, none of them seem to be blockers but you raised good points.  I'll 
send v6 tomorrow.

Paolo