Re: [PATCH v1 3/5] Add use of RCU for qemu_logfile.

["Alex Bennée" <alex.bennee@linaro.org>]

Robert Foley <robert.foley@linaro.org> writes:

> On Tue, 12 Nov 2019 at 12:36, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> >          }
>> > +        atomic_rcu_set(&qemu_logfile, logfile);
>> >      }
>> > -    qemu_mutex_unlock(&qemu_logfile_mutex);
>> > +    logfile = qemu_logfile;
>>
>> Isn't this read outside of the protection of both rcu_read_lock() and
>> the mutex? Could that race?
>
> This assignment is under the mutex.  This change moved the mutex
> unlock towards the end of the function, just a few lines below the
> call_rcu().

Ahh I see now, I went +- blind ;-)

>
>> > +
>> >      if (qemu_logfile &&
>> >          (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
>> > -        qemu_log_close();
>> > +        atomic_rcu_set(&qemu_logfile, NULL);
>> > +        call_rcu(logfile, qemu_logfile_free, rcu);
>>
>> I wonder if we can re-arrange the logic here so it's lets confusing? For
>> example the NULL qemu_loglevel can be detected at the start and we
>> should be able just to squash the current log and reset and go. I'm not
>> sure about the damonize/stdout check.
>>
>> >      }
>> > +    qemu_mutex_unlock(&qemu_logfile_mutex);
>> >  }
>> >
>
> Absolutely, the logic that was here originally can be improved.  We
> found it confusing also.
> We could move things around a bit and change it to something like this
> to help clarify.
>
>     bool need_to_open_file = false;
>     /*
>      * In all cases we only log if qemu_loglevel is set.
>      * And:
>      *     If not daemonized we will always log either to stderr
>      *       or to a file (if there is a logfilename).
>      *     If we are daemonized,
>      *       we will only log if there is a logfilename.
>      */
>     if (qemu_loglevel && (!is_daemonized() || logfilename) {
>         need_to_open_file = true;
>     }
>     g_assert(qemu_logfile_mutex.initialized);
>     qemu_mutex_lock(&qemu_logfile_mutex);
>
>     if(qemu_logfile && !need_to_open_file) {
>         /* Close file. */
>         QemuLogFile *logfile = qemu_logfile;
>         atomic_rcu_set(&qemu_logfile, NULL);
>         call_rcu(logfile, qemu_logfile_free, rcu);
>     } else if(!qemu_logfile && need_to_open_file) {
>         logfile = g_new0(QemuLogFile, 1);
>        __snip__ existing patch logic for opening the qemu_logfile will
> be inserted here.
>     }
>     qemu_mutex_unlock(&qemu_logfile_mutex);

That reads a lot better thanks. It's probably worth doing the clean-up in
a separate patch so it is easier to see the mutex's when added.

--
Alex Bennée