From: "Alex Bennée" <alex.bennee@linaro.org>
To: Robert Foley <robert.foley@linaro.org>
Cc: Peter Puhov <peter.puhov@linaro.org>, qemu-devel@nongnu.org
Subject: Re: [PATCH v1 3/5] Add use of RCU for qemu_logfile.
Date: Wed, 13 Nov 2019 15:24:35 +0000 [thread overview]
Message-ID: <87k1837qa4.fsf@linaro.org> (raw)
In-Reply-To: <CAEyhzFtk4uewBK9dY1SpJh4UVn7g3Pd8qCvqhskJX38ci9jWSA@mail.gmail.com>
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
next prev parent reply other threads:[~2019-11-13 15:25 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 15:01 [PATCH v1 0/5] Make the qemu_logfile handle thread safe Robert Foley
2019-11-12 15:01 ` [PATCH v1 1/5] Add a mutex to guarantee single writer to qemu_logfile handle Robert Foley
2019-11-12 17:09 ` Alex Bennée
2019-11-16 11:57 ` Richard Henderson
2019-11-18 12:39 ` Robert Foley
2019-11-12 15:01 ` [PATCH v1 2/5] qemu_log_lock/unlock now preserves the " Robert Foley
2019-11-12 17:17 ` Alex Bennée
2019-11-16 12:26 ` Richard Henderson
2019-11-12 15:01 ` [PATCH v1 3/5] Add use of RCU for qemu_logfile Robert Foley
2019-11-12 17:36 ` Alex Bennée
2019-11-12 19:20 ` Robert Foley
2019-11-13 15:24 ` Alex Bennée [this message]
2019-11-12 15:01 ` [PATCH v1 4/5] Added tests for close and change of logfile Robert Foley
2019-11-12 18:21 ` Alex Bennée
2019-11-12 15:01 ` [PATCH v1 5/5] Fix double free issue in qemu_set_log_filename() Robert Foley
2019-11-12 18:23 ` Alex Bennée
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87k1837qa4.fsf@linaro.org \
--to=alex.bennee@linaro.org \
--cc=peter.puhov@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=robert.foley@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).