Re: [PATCH 5/6] co-shared-resource: protect with a mutex

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

On 14/05/2021 17:30, Vladimir Sementsov-Ogievskiy wrote:
> 14.05.2021 17:32, Emanuele Giuseppe Esposito wrote:
>>
>>
>> On 14/05/2021 16:26, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.05.2021 17:10, Emanuele Giuseppe Esposito wrote:
>>>>
>>>>
>>>> On 12/05/2021 17:44, Stefan Hajnoczi wrote:
>>>>> On Mon, May 10, 2021 at 10:59:40AM +0200, Emanuele Giuseppe 
>>>>> Esposito wrote:
>>>>>> co-shared-resource is currently not thread-safe, as also reported
>>>>>> in co-shared-resource.h. Add a QemuMutex because 
>>>>>> co_try_get_from_shres
>>>>>> can also be invoked from non-coroutine context.
>>>>>>
>>>>>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>>>> ---
>>>>>>   util/qemu-co-shared-resource.c | 26 ++++++++++++++++++++++----
>>>>>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>>>>
>>>>> Hmm...this thread-safety change is more fine-grained than I was
>>>>> expecting. If we follow this strategy basically any data structure 
>>>>> used
>>>>> by coroutines needs its own fine-grained lock (like Java's Object base
>>>>> class which has its own lock).
>>>>>
>>>>> I'm not sure I like it since callers may still need coarser grained
>>>>> locks to protect their own state or synchronize access to multiple
>>>>> items of data. Also, some callers may not need thread-safety.
>>>>>
>>>>> Can the caller to be responsible for locking instead (e.g. using
>>>>> CoMutex)?
>>>>
>>>> Right now co-shared-resource is being used only by block-copy, so I 
>>>> guess locking it from the caller or within the API won't really 
>>>> matter in this case.
>>>>
>>>> One possible idea on how to delegate this to the caller without 
>>>> adding additional small lock/unlock in block-copy is to move 
>>>> co_get_from_shres in block_copy_task_end, and calling it only when a 
>>>> boolean passed to block_copy_task_end is true.
>>>>
>>>> Otherwise make b_c_task_end always call co_get_from_shres and then 
>>>> include co_get_from_shres in block_copy_task_create, so that we 
>>>> always add and in case remove (if error) in the shared resource.
>>>>
>>>> Something like:
>>>>
>>>> diff --git a/block/block-copy.c b/block/block-copy.c
>>>> index 3a447a7c3d..1e4914b0cb 100644
>>>> --- a/block/block-copy.c
>>>> +++ b/block/block-copy.c
>>>> @@ -233,6 +233,7 @@ static coroutine_fn BlockCopyTask 
>>>> *block_copy_task_create(BlockCopyState *s,
>>>>       /* region is dirty, so no existent tasks possible in it */
>>>>       assert(!find_conflicting_task(s, offset, bytes));
>>>>       QLIST_INSERT_HEAD(&s->tasks, task, list);
>>>> +    co_get_from_shres(s->mem, task->bytes);
>>>>       qemu_co_mutex_unlock(&s->tasks_lock);
>>>>
>>>>       return task;
>>>> @@ -269,6 +270,7 @@ static void coroutine_fn 
>>>> block_copy_task_end(BlockCopyTask *task, int ret)
>>>>           bdrv_set_dirty_bitmap(task->s->copy_bitmap, task->offset, 
>>>> task->bytes);
>>>>       }
>>>>       qemu_co_mutex_lock(&task->s->tasks_lock);
>>>> +    co_put_to_shres(task->s->mem, task->bytes);
>>>>       task->s->in_flight_bytes -= task->bytes;
>>>>       QLIST_REMOVE(task, list);
>>>>       progress_set_remaining(task->s->progress,
>>>> @@ -379,7 +381,6 @@ static coroutine_fn int 
>>>> block_copy_task_run(AioTaskPool *pool,
>>>>
>>>>       aio_task_pool_wait_slot(pool);
>>>>       if (aio_task_pool_status(pool) < 0) {
>>>> -        co_put_to_shres(task->s->mem, task->bytes);
>>>>           block_copy_task_end(task, -ECANCELED);
>>>>           g_free(task);
>>>>           return -ECANCELED;
>>>> @@ -498,7 +499,6 @@ static coroutine_fn int 
>>>> block_copy_task_entry(AioTask *task)
>>>>       }
>>>>       qemu_mutex_unlock(&t->s->calls_lock);
>>>>
>>>> -    co_put_to_shres(t->s->mem, t->bytes);
>>>>       block_copy_task_end(t, ret);
>>>>
>>>>       return ret;
>>>> @@ -687,8 +687,6 @@ block_copy_dirty_clusters(BlockCopyCallState 
>>>> *call_state)
>>>>
>>>>           trace_block_copy_process(s, task->offset);
>>>>
>>>> -        co_get_from_shres(s->mem, task->bytes);
>>>
>>> we want to get from shres here, after possible call to 
>>> block_copy_task_shrink(), as task->bytes may be reduced.
>>
>> Ah right, I missed that. So I guess if we want the caller to protect 
>> co-shared-resource, get_from_shres stays where it is, and put_ instead 
>> can still go into task_end (with a boolean enabling it).
> 
> honestly, I don't follow how it helps thread-safety

 From my understanding, the whole point here is to have no lock in 
co-shared-resource but let the caller take care of it (block-copy).

The above was just an idea on how to do it.

> 
>>>
>>>> -
>>>>           offset = task_end(task);
>>>>           bytes = end - offset;
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>> diff --git a/util/qemu-co-shared-resource.c 
>>>>>> b/util/qemu-co-shared-resource.c
>>>>>> index 1c83cd9d29..c455d02a1e 100644
>>>>>> --- a/util/qemu-co-shared-resource.c
>>>>>> +++ b/util/qemu-co-shared-resource.c
>>>>>> @@ -32,6 +32,7 @@ struct SharedResource {
>>>>>>       uint64_t available;
>>>>>>       CoQueue queue;
>>>>>> +    QemuMutex lock;
>>>>>
>>>>> Please add a comment indicating what this lock protects.
>>>>>
>>>>> Thread safety should also be documented in the header file so API 
>>>>> users
>>>>> know what to expect.
>>>>
>>>> Will do, thanks.
>>>>
>>>> Emanuele
>>>>
>>>
>>>
>>
> 
>