Re: [PATCH v5 2/2] lockable: replaced locks with lock guard macros where appropriate
On Fri, Apr 3, 2020 at 9:21 PM wrote: > > From: Daniel Brodsky > > - 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 > --- > block/iscsi.c | 7 ++ > block/nfs.c | 51 --- > cpus-common.c | 14 +--- > 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 | 5 ++--- Just making sure this patch didn't get lost. ping http://patchwork.ozlabs.org/patch/1266336/
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
On Sat, Mar 28, 2020 at 9:12 AM Richard Henderson wrote: > > On 3/27/20 11:38 PM, Daniel Brodsky wrote: > > On Thu, Mar 26, 2020 at 11:01 AM Richard Henderson > > wrote: > >> > >> My preference is to add -Wno-tautological-type-limit-compare in > >> configure, so we don't have to work around this same issue elsewhere > >> in the code base. > >> > >> r~ > > > > What do you think would be the best way to add this? I could change > > all additions of the `-m32` flag to instead use `-m32 > > -Wno-tautological-type-limit-compare` or add the flag if qemu is being > > compiled with clang and `-m32` already enabled. > > I was going to add it unconditionally, with all of the other warning flags. > > Except that it doesn't work -- clang-9 *still* warns. Clearly a clang bug, > but > there doesn't seem to be any workaround at all except --disable-werror. > > > r~ Using `#pragma clang diagnostic ignored "-Wtautological-type-limit-compare"` suppresses the errors (on Clang 9). I could go and drop that in for the problem areas? There's only a few so it wouldn't be a major change. I'm thinking of adding a macro like this: #define PRAGMA(x) _Pragma(stringify(x)) #define IF_IGNORE_TYPE_LIMIT(statement) \ PRAGMA(clang diagnostic push) \ PRAGMA(clang diagnostic ignored "-Wtautological-type-limit-compare") \ if (statement) \ PRAGMA(clang diagnostic pop) and replacing the problem conditionals with it. Daniel
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
On Thu, Mar 26, 2020 at 11:01 AM Richard Henderson wrote: > > My preference is to add -Wno-tautological-type-limit-compare in > configure, so we don't have to work around this same issue elsewhere > in the code base. > > r~ What do you think would be the best way to add this? I could change all additions of the `-m32` flag to instead use `-m32 -Wno-tautological-type-limit-compare` or add the flag if qemu is being compiled with clang and `-m32` already enabled. Daniel
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
>
> There may be ways to rewrite that expression to avoid triggering the
> warning on a 32-bit platform. Untested, but does this help:
>
> if (sizeof(mask) > 4 && mask <= 0xu) {
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
Unfortunately, the compiler still catches that the statement is always
true for 32-bit.
An alternative might be to convert cases like this to a macro instead,
which doesn't
cause any errors.
Re: [PATCH v4 0/2] Replaced locks with lock guard macros
On Mon, Mar 23, 2020 at 6:25 AM Stefan Hajnoczi wrote:
>
> On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-re...@patchew.org wrote:
> > /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable
> > 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
> > QEMU_LOCK_GUARD(>lock);
> > ^
> > /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from
> > macro 'QEMU_LOCK_GUARD'
>
> Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
> we didn't see this warning before.
>
> clang does report them so let's silence the warning manually. Please
> add G_GNUC_UNUSED onto the g_autoptr variable definition in the
> QEMU_LOCK_GUARD() macro:
>
> #define QEMU_LOCK_GUARD(x) \
> g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED =
> \
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
>
> The WITH_*_LOCK_GUARD() macros should not require changes because the
> variable is both read and written.
>
> You can test locally by building with clang (llvm) instead of gcc:
>
> ./configure --cc=clang
Using --cc=clang is causing the following error in several different places:
qemu/target/ppc/translate.c:1894:18: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295
is always true [-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xu) {
^ ~~~
these errors don't come up when building with gcc. Any idea how to fix
this? Or should I just suppress it?
Re: [PATCH v3 2/2] lockable: replaced locks with lock guard macros where appropriate
On Fri, Mar 20, 2020 at 5:34 AM Dr. David Alan Gilbert
wrote:
>
> * dnbrd...@gmail.com (dnbrd...@gmail.com) wrote:
> > From: Daniel Brodsky
> >
> > - 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
> > ---
> > 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(-)
> >
>
>
>
> > 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(>error_mutex);
> > +QEMU_LOCK_GUARD(>error_mutex);
> > if (!s->error) {
> > s->error = error_copy(error);
> > }
> > -qemu_mutex_unlock(>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.
> > 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 = _recv_state->params[i];
> >
> > -qemu_mutex_lock(>mutex);
> > -if (multifd_recv_state->packet_num < p->packet_num) {
> > -multifd_recv_state->packet_num = p->packet_num;
> > +WITH_QEMU_LOCK_GUARD(>mutex) {
> > +if (multifd_recv_state->packet_num < p->packet_num) {
> > +multifd_recv_state->packet_num = p->packet_num;
> > +}
> > }
> > -qemu_mutex_unlock(>mutex);
> > trace_multifd_recv_sync_main_signal(p->id);
> > qemu_sem_post(>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(>src_page_req_mutex);
> > +QEMU_LOCK_GUARD(>src_page_req_mutex);
> > if (!QSIMPLEQ_EMPTY(>src_page_requests)) {
> > struct RAMSrcPageRequest *entry =
> > QSIMPLEQ_FIRST(>src_page_requests);
> > @@ -1385,7 +1385,6 @@ static RAMBlock *unqueue_page(RAMState *rs,
> > ram_addr_t *offset)
> > migration_consume_urgent_request();
> > }
> > }
> > -qemu_mutex_unlock(>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.
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index 6c45fa490f..9723b466cd 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -1473,7
Re: [PATCH v2 2/2] lockable: replaced locks with lock guard macros where appropriate
On Fri, Mar 20, 2020 at 5:06 AM Paolo Bonzini wrote:
>
> On 20/03/20 00:34, dnbrd...@gmail.com wrote:
> > index 682abd8e09..89f8a656a4 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1086,7 +1086,7 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState
> > *bs,
> > acb->task->expxferlen = acb->ioh->dxfer_len;
> >
> > data.size = 0;
> > -qemu_mutex_lock(>mutex);
> > +QEMU_LOCK_GUARD(>mutex);
> > if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
> > if (acb->ioh->iovec_count == 0) {
> > data.data = acb->ioh->dxferp;
> > @@ -1102,7 +1102,6 @@ static BlockAIOCB *iscsi_aio_ioctl(BlockDriverState
> > *bs,
> > iscsi_aio_ioctl_cb,
> > (data.size > 0) ? : NULL,
> > acb) != 0) {
> > -qemu_mutex_unlock(>mutex);
> > scsi_free_scsi_task(acb->task);
> > qemu_aio_unref(acb);
> > return NULL;
>
> Not exactly the same, should be okay but it also should be noted in the
> changelog.
Going to drop this change in the next version, I don't want this patch
to include cases
with possible side effects as I skipped other ones like this already.
> > void cpu_list_remove(CPUState *cpu)
> > {
> > -qemu_mutex_lock(_cpu_list_lock);
> > +QEMU_LOCK_GUARD(_cpu_list_lock);
> > if (!QTAILQ_IN_USE(cpu, node)) {
> > /* there is nothing to undo since cpu_exec_init() hasn't been
> > called */
> > qemu_mutex_unlock(_cpu_list_lock);
>
>
> Missed unlock.
>
> Otherwise looks good.
>
> Paolo
>
Thanks for the review, I'll fix the changes you pointed out in the next version.
Daniel
Re: [PATCH] lockable: replaced locks with lock guard macros where appropriate
On Thu, Mar 19, 2020 at 1:53 PM Eric Blake wrote: > > Hmm. This one is a different failure than the other patchew warnings > about variable redefinition; but is still evidence that it is missing > your "[PATCH] misc: fix __COUNTER__ macro to be referenced properly". > At any rate, the fact that we have a compiler warning about an unused > variable (when in reality it IS used by the auto-cleanup attribute) is > annoying; we may have to further tweak QEMU_LOCK_GUARD to add an > __attribute__((unused)) to shut up this particular compiler false positive. > > This might fix itself once I revise the patch to properly reference the prior patch before this one. If not then I can add another patch to get rid of the false positive.
Re: [PATCH] lockable: replaced locks with lock guard macros where appropriate
On Thu, Mar 19, 2020 at 1:48 PM Eric Blake wrote:
>
> On 3/19/20 11:19 AM, dnbrd...@gmail.com wrote:
> > From: danbrodsky
> >
> > - 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: danbrodsky
> > ---
> > block/iscsi.c | 23 +++
> > block/nfs.c | 53 ---
> > cpus-common.c | 13 ---
> > hw/display/qxl.c | 44 +--
> > hw/vfio/platform.c| 4 +---
> > 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, 90 insertions(+), 115 deletions(-)
>
> That's a rather big patch touching multiple areas of code at once; I'm
> not sure if it would be easier to review if you were to break it up into
> a series of smaller patches each touching a smaller group of related
> files. For example, I don't mind reviwing block/, but tend to shy away
> from migration/ code.
Is this necessary for a series of fairly basic changes? Most files are only
modified on 1 or 2 lines.
>
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 682abd8e09..df73bde114 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -1086,23 +1086,21 @@ static BlockAIOCB
*iscsi_aio_ioctl(BlockDriverState *bs,
> > acb->task->expxferlen = acb->ioh->dxfer_len;
> >
> > data.size = 0;
> > -qemu_mutex_lock(>mutex);
> > +QEMU_LOCK_GUARD(>mutex);
> > if (acb->task->xfer_dir == SCSI_XFER_WRITE) {
> > if (acb->ioh->iovec_count == 0) {
> > data.data = acb->ioh->dxferp;
> > data.size = acb->ioh->dxfer_len;
> > } else {
> > scsi_task_set_iov_out(acb->task,
> > - (struct scsi_iovec *)
acb->ioh->dxferp,
> > - acb->ioh->iovec_count);
> > + (struct scsi_iovec
*)acb->ioh->dxferp,
> > + acb->ioh->iovec_count);
>
> This looks like a spurious whitespace change. Why is it part of the
patch?
>
Sorry, it looks like my editor was autoformatting some areas of the text.
I'll remove
those changes in the next version.
> > }
> > }
> >
> > if (iscsi_scsi_command_async(iscsi, iscsilun->lun, acb->task,
> >iscsi_aio_ioctl_cb,
> > - (data.size > 0) ? : NULL,
> > - acb) != 0) {
> > -qemu_mutex_unlock(>mutex);
> > + (data.size > 0) ? : NULL, acb)
!= 0) {
> > scsi_free_scsi_task(acb->task);
>
> Unwrapping the line fit in 80 columns, but again, why are you mixing
> whitespace changes in rather than focusing on the cleanup of mutex
> actions? Did you create this patch mechanically with a tool like
> Coccinelle, as the source of your reflowing of lines? If so, what was
> the input to Coccinelle; if it was some other automated tool, can you
> include the formula so that someone else could reproduce your changes
> (whitespace and all)? If it was not automated, that's also okay, but
> then I would not expect as much whitespace churn.
>
Should I not be including changes that fix warnings in code check? I'll
correct
the mistakes and submit a new version without all the whitespace churn.
