* Daniel Brodsky (dnbrd...@gmail.com) wrote:
> On Fri, Mar 20, 2020 at 5:34 AM Dr. David Alan Gilbert
> <dgilb...@redhat.com> wrote:
> >
> > * dnbrd...@gmail.com (dnbrd...@gmail.com) wrote:
> > > From: Daniel Brodsky <dnbrd...@gmail.com>
> > >
> > > - ran regexp "qemu_mutex_lock\(.*\).*\n.*if" to find targets
> > > - replaced result with QEMU_LOCK_GUARD if all unlocks at function end
> > > - replaced result with WITH_QEMU_LOCK_GUARD if unlock not at end
> > >
> > > Signed-off-by: Daniel Brodsky <dnbrd...@gmail.com>
> > > ---
> > > block/iscsi.c | 11 +++-------
> > > block/nfs.c | 51 ++++++++++++++++++++-----------------------
> > > cpus-common.c | 13 +++++------
> > > hw/display/qxl.c | 43 +++++++++++++++++-------------------
> > > hw/vfio/platform.c | 5 ++---
> > > migration/migration.c | 3 +--
> > > migration/multifd.c | 8 +++----
> > > migration/ram.c | 3 +--
> > > monitor/misc.c | 4 +---
> > > ui/spice-display.c | 14 ++++++------
> > > util/log.c | 4 ++--
> > > util/qemu-timer.c | 17 +++++++--------
> > > util/rcu.c | 8 +++----
> > > util/thread-pool.c | 3 +--
> > > util/vfio-helpers.c | 4 ++--
> > > 15 files changed, 84 insertions(+), 107 deletions(-)
> > >
> >
> > <snip>
> >
> > > diff --git a/migration/migration.c b/migration/migration.c
> > > index c1d88ace7f..2f0bd6d8b4 100644
> > > --- a/migration/migration.c
> > > +++ b/migration/migration.c
> > > @@ -1652,11 +1652,10 @@ static void migrate_fd_cleanup_bh(void *opaque)
> > >
> > > void migrate_set_error(MigrationState *s, const Error *error)
> > > {
> > > - qemu_mutex_lock(&s->error_mutex);
> > > + QEMU_LOCK_GUARD(&s->error_mutex);
> > > if (!s->error) {
> > > s->error = error_copy(error);
> > > }
> > > - qemu_mutex_unlock(&s->error_mutex);
> > > }
> >
> > There are some other places in migration.c that would really benefit;
> > for example, migrate_send_rp_message, if you use a LOCK_QUARD
> > there, then you can remove the 'int ret', and the goto error.
> > In postcopy_pause, the locks on qemu_file_lock would work well using the
> > WITH_QEMU_LOCK_GUARD.
>
> I did a basic pass through for targets and that one didn't come up. I can add
> more replacements later, but there are ~300 mutex locks that might be worth
> replacing and going through them manually in one go is too tedious.
Sure; the send_rp_message case is quite a nice example of where the
guard makes the code simpler.
> > > void migrate_fd_error(MigrationState *s, const Error *error)
> > > diff --git a/migration/multifd.c b/migration/multifd.c
> > > index cb6a4a3ab8..9123c111a3 100644
> > > --- a/migration/multifd.c
> > > +++ b/migration/multifd.c
> > > @@ -894,11 +894,11 @@ void multifd_recv_sync_main(void)
> > > for (i = 0; i < migrate_multifd_channels(); i++) {
> > > MultiFDRecvParams *p = &multifd_recv_state->params[i];
> > >
> > > - qemu_mutex_lock(&p->mutex);
> > > - if (multifd_recv_state->packet_num < p->packet_num) {
> > > - multifd_recv_state->packet_num = p->packet_num;
> > > + WITH_QEMU_LOCK_GUARD(&p->mutex) {
> > > + if (multifd_recv_state->packet_num < p->packet_num) {
> > > + multifd_recv_state->packet_num = p->packet_num;
> > > + }
> > > }
> > > - qemu_mutex_unlock(&p->mutex);
> > > trace_multifd_recv_sync_main_signal(p->id);
> > > qemu_sem_post(&p->sem_sync);
> > > }
> >
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index c12cfdbe26..87a670cfbf 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -1368,7 +1368,7 @@ static RAMBlock *unqueue_page(RAMState *rs,
> > > ram_addr_t *offset)
> > > return NULL;
> > > }
> > >
> > > - qemu_mutex_lock(&rs->src_page_req_mutex);
> > > + QEMU_LOCK_GUARD(&rs->src_page_req_mutex);
> > > if (!QSIMPLEQ_EMPTY(&rs->src_page_requests)) {
> > > struct RAMSrcPageRequest *entry =
> > > QSIMPLEQ_FIRST(&rs->src_page_requests);
> > > @@ -1385,7 +1385,6 @@ static RAMBlock *unqueue_page(RAMState *rs,
> > > ram_addr_t *offset)
> > > migration_consume_urgent_request();
> > > }
> > > }
> > > - qemu_mutex_unlock(&rs->src_page_req_mutex);
> > >
> > > return block;
> > > }
> >
> > Is there a reason you didn't do the matching pair at the bottom of
> > ram_save_queue_pages ?
> >
> > Dave
> >
>
> According to https://wiki.qemu.org/ToDo/LockGuards cases that are trivial (no
> conditional logic) shouldn't be replaced.
OK
So for what you've already go tthere,
For migration:
Acked-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> > > diff --git a/monitor/misc.c b/monitor/misc.c
> > > index 6c45fa490f..9723b466cd 100644
> > > --- a/monitor/misc.c
> > > +++ b/monitor/misc.c
> > > @@ -1473,7 +1473,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool
> > > has_fdset_id, int64_t fdset_id,
> > > MonFdsetFd *mon_fdset_fd;
> > > AddfdInfo *fdinfo;
> > >
> > > - qemu_mutex_lock(&mon_fdsets_lock);
> > > + QEMU_LOCK_GUARD(&mon_fdsets_lock);
> > > if (has_fdset_id) {
> > > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> > > /* Break if match found or match impossible due to ordering
> > > by ID */
> > > @@ -1494,7 +1494,6 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool
> > > has_fdset_id, int64_t fdset_id,
> > > if (fdset_id < 0) {
> > > error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> > > "fdset-id",
> > > "a non-negative value");
> > > - qemu_mutex_unlock(&mon_fdsets_lock);
> > > return NULL;
> > > }
> > > /* Use specified fdset ID */
> > > @@ -1545,7 +1544,6 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool
> > > has_fdset_id, int64_t fdset_id,
> > > fdinfo->fdset_id = mon_fdset->id;
> > > fdinfo->fd = mon_fdset_fd->fd;
> > >
> > > - qemu_mutex_unlock(&mon_fdsets_lock);
> > > return fdinfo;
> > > }
> > >
> > > diff --git a/ui/spice-display.c b/ui/spice-display.c
> > > index 6babe24909..19632fdf6c 100644
> > > --- a/ui/spice-display.c
> > > +++ b/ui/spice-display.c
> > > @@ -18,6 +18,7 @@
> > > #include "qemu/osdep.h"
> > > #include "ui/qemu-spice.h"
> > > #include "qemu/timer.h"
> > > +#include "qemu/lockable.h"
> > > #include "qemu/main-loop.h"
> > > #include "qemu/option.h"
> > > #include "qemu/queue.h"
> > > @@ -483,12 +484,12 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay
> > > *ssd)
> > > {
> > > graphic_hw_update(ssd->dcl.con);
> > >
> > > - qemu_mutex_lock(&ssd->lock);
> > > - if (QTAILQ_EMPTY(&ssd->updates) && ssd->ds) {
> > > - qemu_spice_create_update(ssd);
> > > - ssd->notify++;
> > > + WITH_QEMU_LOCK_GUARD(&ssd->lock) {
> > > + if (QTAILQ_EMPTY(&ssd->updates) && ssd->ds) {
> > > + qemu_spice_create_update(ssd);
> > > + ssd->notify++;
> > > + }
> > > }
> > > - qemu_mutex_unlock(&ssd->lock);
> > >
> > > trace_qemu_spice_display_refresh(ssd->qxl.id, ssd->notify);
> > > if (ssd->notify) {
> > > @@ -580,7 +581,7 @@ static int interface_get_cursor_command(QXLInstance
> > > *sin, QXLCommandExt *ext)
> > > SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
> > > int ret;
> > >
> > > - qemu_mutex_lock(&ssd->lock);
> > > + QEMU_LOCK_GUARD(&ssd->lock);
> > > if (ssd->ptr_define) {
> > > *ext = ssd->ptr_define->ext;
> > > ssd->ptr_define = NULL;
> > > @@ -592,7 +593,6 @@ static int interface_get_cursor_command(QXLInstance
> > > *sin, QXLCommandExt *ext)
> > > } else {
> > > ret = false;
> > > }
> > > - qemu_mutex_unlock(&ssd->lock);
> > > return ret;
> > > }
> > >
> > > diff --git a/util/log.c b/util/log.c
> > > index 2da6cb31dc..bdb3d712e8 100644
> > > --- a/util/log.c
> > > +++ b/util/log.c
> > > @@ -25,6 +25,7 @@
> > > #include "qemu/cutils.h"
> > > #include "trace/control.h"
> > > #include "qemu/thread.h"
> > > +#include "qemu/lockable.h"
> > >
> > > static char *logfilename;
> > > static QemuMutex qemu_logfile_mutex;
> > > @@ -94,7 +95,7 @@ void qemu_set_log(int log_flags)
> > > if (qemu_loglevel && (!is_daemonized() || logfilename)) {
> > > need_to_open_file = true;
> > > }
> > > - qemu_mutex_lock(&qemu_logfile_mutex);
> > > + QEMU_LOCK_GUARD(&qemu_logfile_mutex);
> > > if (qemu_logfile && !need_to_open_file) {
> > > logfile = qemu_logfile;
> > > atomic_rcu_set(&qemu_logfile, NULL);
> > > @@ -136,7 +137,6 @@ void qemu_set_log(int log_flags)
> > > }
> > > atomic_rcu_set(&qemu_logfile, logfile);
> > > }
> > > - qemu_mutex_unlock(&qemu_logfile_mutex);
> > > }
> > >
> > > void qemu_log_needs_buffers(void)
> > > diff --git a/util/qemu-timer.c b/util/qemu-timer.c
> > > index d548d3c1ad..b6575a2cd5 100644
> > > --- a/util/qemu-timer.c
> > > +++ b/util/qemu-timer.c
> > > @@ -459,17 +459,16 @@ void timer_mod_anticipate_ns(QEMUTimer *ts, int64_t
> > > expire_time)
> > > QEMUTimerList *timer_list = ts->timer_list;
> > > bool rearm;
> > >
> > > - qemu_mutex_lock(&timer_list->active_timers_lock);
> > > - if (ts->expire_time == -1 || ts->expire_time > expire_time) {
> > > - if (ts->expire_time != -1) {
> > > - timer_del_locked(timer_list, ts);
> > > + WITH_QEMU_LOCK_GUARD(&timer_list->active_timers_lock) {
> > > + if (ts->expire_time == -1 || ts->expire_time > expire_time) {
> > > + if (ts->expire_time != -1) {
> > > + timer_del_locked(timer_list, ts);
> > > + }
> > > + rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
> > > + } else {
> > > + rearm = false;
> > > }
> > > - rearm = timer_mod_ns_locked(timer_list, ts, expire_time);
> > > - } else {
> > > - rearm = false;
> > > }
> > > - qemu_mutex_unlock(&timer_list->active_timers_lock);
> > > -
> > > if (rearm) {
> > > timerlist_rearm(timer_list);
> > > }
> > > diff --git a/util/rcu.c b/util/rcu.c
> > > index 177a675619..60a37f72c3 100644
> > > --- a/util/rcu.c
> > > +++ b/util/rcu.c
> > > @@ -31,6 +31,7 @@
> > > #include "qemu/atomic.h"
> > > #include "qemu/thread.h"
> > > #include "qemu/main-loop.h"
> > > +#include "qemu/lockable.h"
> > > #if defined(CONFIG_MALLOC_TRIM)
> > > #include <malloc.h>
> > > #endif
> > > @@ -141,14 +142,14 @@ static void wait_for_readers(void)
> > >
> > > void synchronize_rcu(void)
> > > {
> > > - qemu_mutex_lock(&rcu_sync_lock);
> > > + QEMU_LOCK_GUARD(&rcu_sync_lock);
> > >
> > > /* Write RCU-protected pointers before reading p_rcu_reader->ctr.
> > > * Pairs with smp_mb_placeholder() in rcu_read_lock().
> > > */
> > > smp_mb_global();
> > >
> > > - qemu_mutex_lock(&rcu_registry_lock);
> > > + QEMU_LOCK_GUARD(&rcu_registry_lock);
> > > if (!QLIST_EMPTY(®istry)) {
> > > /* In either case, the atomic_mb_set below blocks stores that
> > > free
> > > * old RCU-protected pointers.
> > > @@ -169,9 +170,6 @@ void synchronize_rcu(void)
> > >
> > > wait_for_readers();
> > > }
> > > -
> > > - qemu_mutex_unlock(&rcu_registry_lock);
> > > - qemu_mutex_unlock(&rcu_sync_lock);
> > > }
> > >
> > >
> > > diff --git a/util/thread-pool.c b/util/thread-pool.c
> > > index 4ed9b89ab2..d763cea505 100644
> > > --- a/util/thread-pool.c
> > > +++ b/util/thread-pool.c
> > > @@ -210,7 +210,7 @@ static void thread_pool_cancel(BlockAIOCB *acb)
> > >
> > > trace_thread_pool_cancel(elem, elem->common.opaque);
> > >
> > > - qemu_mutex_lock(&pool->lock);
> > > + QEMU_LOCK_GUARD(&pool->lock);
> > > if (elem->state == THREAD_QUEUED &&
> > > /* No thread has yet started working on elem. we can try to
> > > "steal"
> > > * the item from the worker if we can get a signal from the
> > > @@ -225,7 +225,6 @@ static void thread_pool_cancel(BlockAIOCB *acb)
> > > elem->ret = -ECANCELED;
> > > }
> > >
> > > - qemu_mutex_unlock(&pool->lock);
> > > }
> > >
> > > static AioContext *thread_pool_get_aio_context(BlockAIOCB *acb)
> > > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> > > index ddd9a96e76..b310b23003 100644
> > > --- a/util/vfio-helpers.c
> > > +++ b/util/vfio-helpers.c
> > > @@ -21,6 +21,7 @@
> > > #include "standard-headers/linux/pci_regs.h"
> > > #include "qemu/event_notifier.h"
> > > #include "qemu/vfio-helpers.h"
> > > +#include "qemu/lockable.h"
> > > #include "trace.h"
> > >
> > > #define QEMU_VFIO_DEBUG 0
> > > @@ -667,14 +668,13 @@ int qemu_vfio_dma_reset_temporary(QEMUVFIOState *s)
> > > .size = QEMU_VFIO_IOVA_MAX - s->high_water_mark,
> > > };
> > > trace_qemu_vfio_dma_reset_temporary(s);
> > > - qemu_mutex_lock(&s->lock);
> > > + QEMU_LOCK_GUARD(&s->lock);
> > > if (ioctl(s->container, VFIO_IOMMU_UNMAP_DMA, &unmap)) {
> > > error_report("VFIO_UNMAP_DMA failed: %s", strerror(errno));
> > > qemu_mutex_unlock(&s->lock);
> > > return -errno;
> > > }
> > > s->high_water_mark = QEMU_VFIO_IOVA_MAX;
> > > - qemu_mutex_unlock(&s->lock);
> > > return 0;
> > > }
> > >
> > > --
> > > 2.25.1
> > >
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
> >
>
> Daniel
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
