Peter Xu <pet...@redhat.com> writes:
> On Thu, May 24, 2018 at 11:03:55AM +0200, Markus Armbruster wrote:
>> Peter Xu <pet...@redhat.com> writes:
>>
>> > Similar to previous patch, but introduce a new global big lock for
>> > mon_fdsets. Take it where needed.
>>
>> The previous patch is "monitor: more comments on lock-free
>> fleids/funcs". Sure you mean that one?
>
> No... I'll remove that sentence directly:
>
> "Introduce a new global big lock for mon_fdsets. Take it where needed."
Works for me.
>> > The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
>> > qemu_mutex_unlock() which might pollute errno, so we need to make sure
>> > the correct errno be passed up to the callers. To make things simpler,
>> > we let monitor_fdset_get_fd() return the -errno directly when error
>> > happens, then in qemu_open() we translate it back into errno.
>>
>> Suggest s/translate/move/.
>
> Okay.
>
>> >
>> > Signed-off-by: Peter Xu <pet...@redhat.com>
>> > ---
>> > monitor.c | 70 +++++++++++++++++++++++++++++++++++++++++-----------
>> > util/osdep.c | 3 ++-
>> > 2 files changed, 58 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index f01589f945..6266ff65c4 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -271,6 +271,9 @@ typedef struct QMPRequest QMPRequest;
>> > /* Protects mon_list, monitor_event_state. */
>>
>> Not this patch's problem: there is no monitor_event_state. Screwed up
>> in commit d622cb5879c. I guess it's monitor_qapi_event_state.
>
> I'll append another patch to do that, and move these fields together.
>
>>
>> > static QemuMutex monitor_lock;
>> >
>> > +/* Protects mon_fdsets */
>> > +static QemuMutex mon_fdsets_lock;
>> > +
>> > static QTAILQ_HEAD(mon_list, Monitor) mon_list;
>> > static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
>> > static int mon_refcount;
>>
>> Suggest to move mon_fdsets next to the lock protecting it.
>
> Will do.
>
>>
>> > @@ -287,6 +290,16 @@ static QEMUClockType event_clock_type =
>> > QEMU_CLOCK_REALTIME;
>> > static void monitor_command_cb(void *opaque, const char *cmdline,
>> > void *readline_opaque);
>> >
>> > +/*
>> > + * This lock can be used very early, even during param parsing.
>>
>> Do you mean CLI parsing?
>
> Yes, will s/param/CLI/.
>
>>
>> > + * Meanwhile it can also be used even at the end of main. Let's keep
>> > + * it initialized for the whole lifecycle of QEMU.
>> > + */
>>
>> Awkward question, since our main() is such a tangled mess, but here goes
>> anyway... The existing place to initialize monitor.c's globals is
>> monitor_init_globals(). But that one runs too late, I guess:
>> parse_add_fd() runs earlier, and calls monitor_fdset_add_fd(). Unclean
>> even without this lock; no module should be used before its
>> initialization function runs. Sure we can't run monitor_init_globals()
>> sufficiently early?
>
> Please see the comment for monitor_init_globals():
>
> /*
> * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
> * depends on configure_accelerator() above.
> */
> monitor_init_globals();
>
> So I guess it won't work to directly move it earlier. The init
> dependency of QEMU is really complicated. I'll be fine now that we
> mark totally independent init functions (like this one, which is a
> mutex init only) as constructor, then we can save some time on
> ordering issue.
Let me rephrase. There's a preexisting issue: main() calls monitor.c
functions before calling its initialization function
monitor_init_globals(). This needs to be cleaned up. Would you be
willing to do it?
Possible solutions:
* Perhaps we can move monitor_init_globals() up and/or the code calling
into monitor.c early down sufficiently.
* Calculate event_clock_type on each use instead of ahead of time. It's
qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither
of its users needs to be fast. Then move monitor_init_globals before
the code calling into monitor.c.
I'm not opposed to use of constructors for self-contained initialization
(no calls to other modules). But I don't like initialization spread
over multiple functions.
>> > +static void __attribute__((constructor)) mon_fdsets_lock_init(void)
>> > +{
>> > + qemu_mutex_init(&mon_fdsets_lock);
>> > +}
>> > +
>> > /**
>> > * Is @mon a QMP monitor?
>> > */
>> > @@ -2316,9 +2329,11 @@ static void monitor_fdsets_cleanup(void)
>> > MonFdset *mon_fdset;
>> > MonFdset *mon_fdset_next;
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) {
>> > monitor_fdset_cleanup(mon_fdset);
>> > }
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > }
>> >
>> > AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool
>> > has_opaque,
>> > @@ -2353,6 +2368,7 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd,
>> > int64_t fd, Error **errp)
>> > MonFdsetFd *mon_fdset_fd;
>> > char fd_str[60];
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > if (mon_fdset->id != fdset_id) {
>> > continue;
>> > @@ -2372,10 +2388,12 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd,
>> > int64_t fd, Error **errp)
>> > goto error;
>> > }
>> > monitor_fdset_cleanup(mon_fdset);
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > return;
>> > }
>> >
>> > error:
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > if (has_fd) {
>> > snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%"
>> > PRId64,
>> > fdset_id, fd);
>> > @@ -2391,6 +2409,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>> > MonFdsetFd *mon_fdset_fd;
>> > FdsetInfoList *fdset_list = NULL;
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
>> > FdsetFdInfoList *fdsetfd_list = NULL;
>> > @@ -2420,6 +2439,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>> > fdset_info->next = fdset_list;
>> > fdset_list = fdset_info;
>> > }
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> >
>> > return fdset_list;
>> > }
>> > @@ -2432,6 +2452,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);
>> > if (has_fdset_id) {
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > /* Break if match found or match impossible due to ordering
>> > by ID */
>> > @@ -2452,6 +2473,7 @@ 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 */
>> > @@ -2502,16 +2524,21 @@ 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;
>> > }
>> >
>> > int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>> > {
>> > -#ifndef _WIN32
>> > +#ifdef _WIN32
>> > + return -ENOENT;
>>
>> ENOENT feels odd, but I guess makes some sense since there is no way to
>> add entries.
>>
>> > +#else
>> > MonFdset *mon_fdset;
>> > MonFdsetFd *mon_fdset_fd;
>> > int mon_fd_flags;
>> > + int ret = -ENOENT;
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > if (mon_fdset->id != fdset_id) {
>> > continue;
>> > @@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int
>> > flags)
>> > QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
>> > mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
>> > if (mon_fd_flags == -1) {
>> > - return -1;
>> > + ret = -errno;
>> > + goto out;
>> > }
>> >
>> > if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
>> > - return mon_fdset_fd->fd;
>> > + ret = mon_fdset_fd->fd;
>> > + goto out;
>> > }
>> > }
>> > - errno = EACCES;
>> > - return -1;
>> > + ret = -EACCES;
>> > + break;
>> > }
>>
>> I still think
>>
>> ret = -EACCES;
>> goto out;
>> }
>> ret = -ENOENT;
>>
>> out:
>>
>> would be clearer.
>
> I'll take your advice.
>
>>
>> > +out:
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > + return ret;
>> > #endif
>> > -
>> > - errno = ENOENT;
>> > - return -1;
>> > }
>> >
>> > int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
>> > {
>> > MonFdset *mon_fdset;
>> > MonFdsetFd *mon_fdset_fd_dup;
>> > + int ret = -1;
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > if (mon_fdset->id != fdset_id) {
>> > continue;
>> > }
>> > QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>> > if (mon_fdset_fd_dup->fd == dup_fd) {
>> > - return -1;
>> > + ret = -1;
>>
>> Dead assignment. Alternatively,
>>
>> > + goto out;
>> > }
>> > }
>> > mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
>> > mon_fdset_fd_dup->fd = dup_fd;
>> > QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
>> > - return 0;
>> > + ret = 0;
>> > + break;
>> > }
>>
>> ... add
>>
>> ret = -1;
>>
>> here, and drop the initializer. Your choice.
>
> The variable "ret" brought some trouble, so I decided to remove it
> directly. :)
>
> @@ -2538,20 +2569,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int
> dup_fd)
> MonFdset *mon_fdset;
> MonFdsetFd *mon_fdset_fd_dup;
>
> + qemu_mutex_lock(&mon_fdsets_lock);
> QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> if (mon_fdset->id != fdset_id) {
> continue;
> }
> QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> if (mon_fdset_fd_dup->fd == dup_fd) {
> - return -1;
> + goto err;
> }
> }
> mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup));
> mon_fdset_fd_dup->fd = dup_fd;
> QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next);
> + qemu_mutex_unlock(&mon_fdsets_lock);
> return 0;
> }
> +
> +err:
> + qemu_mutex_unlock(&mon_fdsets_lock);
> return -1;
> }
Works for me.
>>
>> > - return -1;
>> > +
>> > +out:
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > + return ret;
>> > }
>> >
>> > static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>> > {
>> > MonFdset *mon_fdset;
>> > MonFdsetFd *mon_fdset_fd_dup;
>> > + int ret = -1;
>> >
>> > + qemu_mutex_lock(&mon_fdsets_lock);
>> > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>> > QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
>> > if (mon_fdset_fd_dup->fd == dup_fd) {
>> > @@ -2570,14 +2608,18 @@ static int monitor_fdset_dup_fd_find_remove(int
>> > dup_fd, bool remove)
>> > if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
>> > monitor_fdset_cleanup(mon_fdset);
>> > }
>> > - return -1;
>> > + ret = -1;
>> > + goto out;
>> > } else {
>> > - return mon_fdset->id;
>> > + ret = mon_fdset->id;
>> > + goto out;
>> > }
>> > }
>> > }
>> > }
>> > - return -1;
>> > +out:
>> > + qemu_mutex_unlock(&mon_fdsets_lock);
>> > + return ret;
>> > }
>>
>> Likewise.
>
> I'll do similar thing to drop "ret":
>
> @@ -2560,6 +2596,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd,
> bool remove)
> MonFdset *mon_fdset;
> MonFdsetFd *mon_fdset_fd_dup;
>
> + qemu_mutex_lock(&mon_fdsets_lock);
> QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
> if (mon_fdset_fd_dup->fd == dup_fd) {
> @@ -2568,13 +2605,17 @@ static int monitor_fdset_dup_fd_find_remove(int
> dup_fd, bool remove)
> if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> monitor_fdset_cleanup(mon_fdset);
> }
> - return -1;
> + goto err;
> } else {
> + qemu_mutex_unlock(&mon_fdsets_lock);
> return mon_fdset->id;
> }
> }
> }
> }
> +
> +err:
> + qemu_mutex_unlock(&mon_fdsets_lock);
> return -1;
> }
Also works for me.
