Re: [PATCH 4/6] progressmeter: protect with a mutex

[Emanuele Giuseppe Esposito]

On 10/05/2021 13:28, Vladimir Sementsov-Ogievskiy wrote:
> 10.05.2021 11:59, Emanuele Giuseppe Esposito wrote:
> > Progressmeter is protected by the AioContext mutex, which
> > is taken by the block jobs and their caller (like blockdev).
> >
> > We would like to remove the dependency of block layer code on the
> > AioContext mutex, since most drivers and the core I/O code are already
> > not relying on it.
> >
> > The simplest thing to do is to add a mutex to be able to provide
> > an accurate snapshot of the progress values to the caller.
> >
> > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> > ---
> >   blockjob.c                    | 33 +++++++++++++++++++++++++--------
>
> [..]
>
> > --- a/include/qemu/progress_meter.h
> > +++ b/include/qemu/progress_meter.h
> > @@ -27,6 +27,8 @@
> >   #ifndef QEMU_PROGRESS_METER_H
> >   #define QEMU_PROGRESS_METER_H
> > +#include "qemu/lockable.h"
> > +
> >   typedef struct ProgressMeter {
> >       /**
> >        * Current progress. The unit is arbitrary as long as the ratio 
> > between
> > @@ -37,21 +39,50 @@ typedef struct ProgressMeter {
> >       /** Estimated current value at the completion of the process */
> >       uint64_t total;
> > +
> > +    QemuMutex lock;
> >   } ProgressMeter;
> > +static inline void progress_init(ProgressMeter *pm)
> > +{
> > +    qemu_mutex_init(&pm->lock);
> > +}
> > +
> > +static inline void progress_destroy(ProgressMeter *pm)
> > +{
> > +    qemu_mutex_destroy(&pm->lock);
> > +}
>
>
> Could we instead add a c file and add the structure private? Then we'll 
> have progress_new() and progress_free() APIs instead.
>
> This way, it would be a lot simpler to control that nobady use structure 
> fields directly.
Makes sense, I'll do it.
> > +
> > +static inline void progress_get_snapshot(ProgressMeter *pm,
> > +                                         uint64_t *current, uint64_t 
> > *total)
> > +{
> > +    QEMU_LOCK_GUARD(&pm->lock);
> > +
> > +    if (current) {
> > +        *current = pm->current;
> > +    }
> > +
> > +    if (total) {
> > +        *total = pm->total;
> > +    }
> > +}
>
> We don't have caller that pass only one pointer. So we can just do
>
> *current = pm->current;
> *total = pm->total;
>
> implicitly requiring both pointers to be non NULL.

Is it so performance critical that we need to skip these safety checks?
IMHO we can keep it as it is.

Thank you,
Emanuele
>