From: Prarit Bhargava <prarit@redhat.com>
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
Richard Cochran <richardcochran@gmail.com>
Subject: Re: [PATCH 0/8] Move ntp state to be protected by timekeeping lock
Date: Thu, 28 Mar 2013 08:54:08 -0400 [thread overview]
Message-ID: <51543D70.8040808@redhat.com> (raw)
In-Reply-To: <1364242098-5977-1-git-send-email-john.stultz@linaro.org>
On 03/25/2013 04:08 PM, John Stultz wrote:
>
> The problem of course, is that the new asynchronous behavior the
> shadow updates breaks some of the assumptions on how the NTP state
> is used. Thus to correct this, we really need to serialize the ntp
> state updates along with the timekeeping state. With the added side
> benefit that of reducing lock acquisitions.
>
> The downside is that the timekeeping state has been cleaned up to
> live nicely in the timekeeper struct, which nicely bounded what the
> timekeeping lock protected. Where as 99% of the NTP state was all
> static to ntp.c, and was protected by the ntp.c static ntp_lock, so
> it was all nicely encapsulated as well.
>
> This patchset makes the lock ownership lines less obvious, but I've
> been sure to keep the ntp state static to ntp.c and instead provided
> some accessors via ntp-internal.h that timekeping code can use to
> make changes. The only really ugly part is that do_adjtimex() has
> to split some of the logic between timekeeping.c and ntp.c in order
> to really get the locking done correctly.
John, I have no technical objection to the patch ... but after reviewing the
changes I think you've significantly changed the way the locking works in the
NTP code, and IMO, some note should be made in the code about the timekeeper
lock and its impact to ntp. It's not trivial reading this code and I think the
dropping of the ntp lock will confuse the casual viewer.
IMO of course.
>
> I may try to rework the code in the future so that the timekeeper
> holds the ntp state and passes it to the ntp.c functions to be
> modified, but that is a much deeper rework then I'd like to do right
> now, and causes fruther complexity to the shadow-state updates, since
> we'd end up unnecessarily copying the ntp state back and forth every
> time.
>
> This applies on top of my fortglx/3.10/time queue here:
> git://git.linaro.org/people/jstultz/linux.git fortglx/3.10/time
>
> If you want to see this entire set (along with Thomas' shadow-update
> work) it can be found here:
> git://git.linaro.org/people/jstultz/linux.git dev/tglx-shadowtime
> or
> http://git.linaro.org/gitweb?p=people/jstultz/linux.git;a=shortlog;h=refs/heads/dev/tglx-shadowtime
>
Beyond the above comment, a quick test shows that ntp does work AFAICT (at least
on F18 + your git tree. I'll try and do a heavier test next week.
So for now ...
Acked-by: Prarit Bhargava <prarit@redhat.com>
P.
prev parent reply other threads:[~2013-03-28 12:54 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-25 20:08 [PATCH 0/8] Move ntp state to be protected by timekeeping lock John Stultz
2013-03-25 20:08 ` [PATCH 1/8] ntp: Split out timex validation from do_adjtimex John Stultz
2013-03-25 20:08 ` [PATCH 2/8] ntp: Move do_adjtimex() and hardpps() functions to timekeeping.c John Stultz
2013-03-25 20:08 ` [PATCH 3/8] ntp: Move timex validation to timekeeping do_adjtimex call John Stultz
2013-03-25 20:08 ` [PATCH 4/8] ntp: Rework do_adjtimex to take timespec and tai arguments John Stultz
2013-03-25 20:08 ` [PATCH 5/8] timekeeping: Move ADJ_SETOFFSET to top level do_adjtimex() John Stultz
2013-03-25 20:08 ` [PATCH 6/8] timekeeping: Hold timekeepering locks in do_adjtimex and hardpps John Stultz
2013-03-25 20:08 ` [PATCH 7/8] timekeeping: Simplify tai updating from do_adjtimex John Stultz
2013-03-25 20:08 ` [PATCH 8/8] ntp: Remove ntp_lock, using the timekeeping locks to protect ntp state John Stultz
2013-03-26 19:14 ` [PATCH 0/8] Move ntp state to be protected by timekeeping lock Richard Cochran
2013-03-28 12:54 ` Prarit Bhargava [this message]
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=51543D70.8040808@redhat.com \
--to=prarit@redhat.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=tglx@linutronix.de \
/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