About ceph_clock_now()

About ceph_clock_now()

[Erwan Velu]

Hi folks,

When reading the code I noticed that ceph_clock_now() is mostly used to compute a local difference of time.
I do see you are using "clock_gettime(CLOCK_REALTIME, &tp)" but was wondering if CLOCK_MONOTONIC_RAW isn't more accurate.

I mean if the local system time is readujsted by NTP (or similar) some computation could be wrong leading to some possible mis-interpretation or wrong scheduling.

Any thoughts on that ?

I could work on a PR is you find this interesting.

Cheers,
Erwan

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 2922 bytes --]

On 12/01/2016, Erwan Velu wrote:
> Hi folks,

Hello!

> When reading the code I noticed that ceph_clock_now() is mostly used to compute a local difference of time.
> I do see you are using "clock_gettime(CLOCK_REALTIME, &tp)" but was wondering if CLOCK_MONOTONIC_RAW isn't more accurate.
> 
> I mean if the local system time is readujsted by NTP (or similar) some computation could be wrong leading to some possible mis-interpretation or wrong scheduling.

I have done some work starting in that direction currently in master. In
common/ceph_time.h I define coarse and fine versions of the real clock
and monotonic clock and I've switched the osdc subsystem over to use them.

We use the std::chrono C++ time library, but we define our own clocks
(so we can pick the right one for performance/resolution tradeoffs) and an
in-memory timespan representation (so not every function taking a duration has
to be a template.)

The real_clock and mono_clock classes correspond to CLOCK_REALTIME and CLOCK_MONOTONIC, and coarse_real_clock and coarse_mono_clock correspond to CLOCK_REALTIME_COARSE and CLOCK_MONOTONIC_COARSE. If you require millisecond resolution but nothing finer, you should prefer the coarse variants.

The std::chrono library lets us integrate with other C++ libraries (and use the
standard concurrency features) and has the advantage of stronger typing; we
can't use a duration where a time point is required or subtract a monotonic
time point from a real time point.

As I understand it, CLOCK_MONOTONIC_RAW is generally the wrong choice.
The raw hardware clock is prone to drift, and NTP synchronization (or other
corrective) makes the elapsed time measurements more accurate.

However, I am not a Time Lord. If someone knows better, please correct me here.

One concern with monotonic clocks, even for elapsed time, is that their epoch is
unspecified. You cannot send a monotonic time over the network to another
machine nor can you save it to a file and re-read it later. Anything to be
serialized must be a real time.

This is unfortunate. If I were a Time Lord I'd travel back in time and make the
POSIX committee adopt CLOCK_UTC and CLOCK_TAI so we could have serializable
monotonic time values.

> I could work on a PR is you find this interesting.

I find it interesting. If you want to change more subsystems to use ceph_time
and use monotonic and coarse clocks in appropriate places I think that would be
wonderful. It might be worthwhile to focus on the places where we use double to
represent a time value.

Please read through ceph_time.h and osdc to get a feel for it. If you think it
could use improvement, I'd be happy to hear your ideas.

Thank you.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Erwan Velu]

>As I understand it, CLOCK_MONOTONIC_RAW is generally the wrong choice.
>The raw hardware clock is prone to drift, and NTP synchronization (or other
>corrective) makes the elapsed time measurements more accurate.

You consider the MONOTONIC clock drifting a lot ?
I mean an NTP adjustement can make a serious jump forward or even worse backward.


>One concern with monotonic clocks, even for elapsed time, is that their epoch is
>unspecified. You cannot send a monotonic time over the network to another
>machine nor can you save it to a file and re-read it later. Anything to be
>serialized must be a real time.

While speaking about ceph_clock_now(), I was more likely targeting lots of local code like
  start = ceph_clock_now();
  ... do_stuff ...
  elapse_time = ceph_clock_now() - start


I'm thinking of code like this in void OSD::tick_without_osd_lock():
Starting at https://github.com/ceph/ceph/blob/master/src/osd/OSD.cc#L4009.

This code is trying to estimate if a mon is still alive by computing a difference of system time by
doing a  "if (now - last_pg_stats_sent > max) {" where now & last_pg_stats_sent are the output of "ceph_clock_now()"

As ceph_clock_now() is using CLOCK_REALTIME, we could have cases where a simple drift caused by a NTP trigger that code.
As we are only trying to compute a difference of absolute time (and not system time), using a MONOTONIC_* seems much more accurate at my taste.

I know that's pretty picky but that could exists.


>I find it interesting. If you want to change more subsystems to use ceph_time
>and use monotonic and coarse clocks in appropriate places I think that would be
>wonderful. It might be worthwhile to focus on the places where we use double to
>represent a time value.
>Please read through ceph_time.h and osdc to get a feel for it. If you think it
>could use improvement, I'd be happy to hear your ideas.

You mean we could switch some of ceph_clock_now() calls to ceph_time ? 
We could use coarse_mono_clock to perform that.

If you agree that switching code like the one I'm speaking is valuable, I can work on it.

Re: About ceph_clock_now()

[Sage Weil]

On Wed, 13 Jan 2016, Erwan Velu wrote:
> >One concern with monotonic clocks, even for elapsed time, is that their epoch is
> >unspecified. You cannot send a monotonic time over the network to another
> >machine nor can you save it to a file and re-read it later. Anything to be
> >serialized must be a real time.
> 
> While speaking about ceph_clock_now(), I was more likely targeting lots of local code like
>   start = ceph_clock_now();
>   ... do_stuff ...
>   elapse_time = ceph_clock_now() - start
> 
> 
> I'm thinking of code like this in void OSD::tick_without_osd_lock():
> Starting at https://github.com/ceph/ceph/blob/master/src/osd/OSD.cc#L4009.
> 
> This code is trying to estimate if a mon is still alive by computing a difference of system time by
> doing a  "if (now - last_pg_stats_sent > max) {" where now & last_pg_stats_sent are the output of "ceph_clock_now()"
> 
> As ceph_clock_now() is using CLOCK_REALTIME, we could have cases where a simple drift caused by a NTP trigger that code.
> As we are only trying to compute a difference of absolute time (and not system time), using a MONOTONIC_* seems much more accurate at my taste.
> 
> I know that's pretty picky but that could exists.

That's a perfect example of where the monotonic time should be used. There 
are a zillion instances of this pattern that could be cleaned up... :)

> >I find it interesting. If you want to change more subsystems to use ceph_time
> >and use monotonic and coarse clocks in appropriate places I think that would be
> >wonderful. It might be worthwhile to focus on the places where we use double to
> >represent a time value.
> >Please read through ceph_time.h and osdc to get a feel for it. If you think it
> >could use improvement, I'd be happy to hear your ideas.
> 
> You mean we could switch some of ceph_clock_now() calls to ceph_time ? 
> We could use coarse_mono_clock to perform that.
> 
> If you agree that switching code like the one I'm speaking is valuable, 
> I can work on it.

That would be great!

sage

Re: About ceph_clock_now()

[Matt Benjamin]

Hi,

----- Original Message -----
> From: "Sage Weil" <sweil@redhat.com>
> To: "Erwan Velu" <evelu@redhat.com>
> Cc: "Adam C. Emerson" <aemerson@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
> Sent: Wednesday, January 13, 2016 8:48:00 AM
> Subject: Re: About ceph_clock_now()
> 

> > 
> > You mean we could switch some of ceph_clock_now() calls to ceph_time ?
> > We could use coarse_mono_clock to perform that.
> > 
> > If you agree that switching code like the one I'm speaking is valuable,
> > I can work on it.
> 
> That would be great!

Yes, ack.  A coordinated effort to propagate the new interfaces changes through the codebase would be of great help.

Matt

> 
> sage
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-707-0660
fax.  734-769-8938
cel.  734-216-5309

Re: About ceph_clock_now()

[Erwan Velu]

Thanks for your insights. 

I even wonder if the change of the daylight time saving could not trigger that bug I'm thinking about.
Especially if the server time is in local time .... That's an horrible vision :(

I'll work on that next week, I have to complete some work before switching to that.

Cheers,

----- Mail original -----
De: "Sage Weil" <sweil@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Adam C. Emerson" <aemerson@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Mercredi 13 Janvier 2016 14:48:00
Objet: Re: About ceph_clock_now()

On Wed, 13 Jan 2016, Erwan Velu wrote:
> >One concern with monotonic clocks, even for elapsed time, is that their epoch is
> >unspecified. You cannot send a monotonic time over the network to another
> >machine nor can you save it to a file and re-read it later. Anything to be
> >serialized must be a real time.
> 
> While speaking about ceph_clock_now(), I was more likely targeting lots of local code like
>   start = ceph_clock_now();
>   ... do_stuff ...
>   elapse_time = ceph_clock_now() - start
> 
> 
> I'm thinking of code like this in void OSD::tick_without_osd_lock():
> Starting at https://github.com/ceph/ceph/blob/master/src/osd/OSD.cc#L4009.
> 
> This code is trying to estimate if a mon is still alive by computing a difference of system time by
> doing a  "if (now - last_pg_stats_sent > max) {" where now & last_pg_stats_sent are the output of "ceph_clock_now()"
> 
> As ceph_clock_now() is using CLOCK_REALTIME, we could have cases where a simple drift caused by a NTP trigger that code.
> As we are only trying to compute a difference of absolute time (and not system time), using a MONOTONIC_* seems much more accurate at my taste.
> 
> I know that's pretty picky but that could exists.

That's a perfect example of where the monotonic time should be used. There 
are a zillion instances of this pattern that could be cleaned up... :)

> >I find it interesting. If you want to change more subsystems to use ceph_time
> >and use monotonic and coarse clocks in appropriate places I think that would be
> >wonderful. It might be worthwhile to focus on the places where we use double to
> >represent a time value.
> >Please read through ceph_time.h and osdc to get a feel for it. If you think it
> >could use improvement, I'd be happy to hear your ideas.
> 
> You mean we could switch some of ceph_clock_now() calls to ceph_time ? 
> We could use coarse_mono_clock to perform that.
> 
> If you agree that switching code like the one I'm speaking is valuable, 
> I can work on it.

That would be great!

sage

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On 14/01/2016, Erwan Velu wrote:
> Thanks for your insights. 
> 
> I even wonder if the change of the daylight time saving could not trigger that bug I'm thinking about.
> Especially if the server time is in local time .... That's an horrible vision :(

That, at least, is ruled out completely. Outright setting the time won't affect
the monotonic clock.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Erwan Velu]

Before propagating the change to more code base and offer a PR with that, what do you think of this style ?

Is it aligned with the best-pratices of the project ?

https://gist.github.com/ErwanAliasr1/f74f7f046e9300ac2c30

Thanks,

----- Mail original -----
De: "Sage Weil" <sweil@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Adam C. Emerson" <aemerson@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Mercredi 13 Janvier 2016 14:48:00
Objet: Re: About ceph_clock_now()

On Wed, 13 Jan 2016, Erwan Velu wrote:
> >One concern with monotonic clocks, even for elapsed time, is that their epoch is
> >unspecified. You cannot send a monotonic time over the network to another
> >machine nor can you save it to a file and re-read it later. Anything to be
> >serialized must be a real time.
> 
> While speaking about ceph_clock_now(), I was more likely targeting lots of local code like
>   start = ceph_clock_now();
>   ... do_stuff ...
>   elapse_time = ceph_clock_now() - start
> 
> 
> I'm thinking of code like this in void OSD::tick_without_osd_lock():
> Starting at https://github.com/ceph/ceph/blob/master/src/osd/OSD.cc#L4009.
> 
> This code is trying to estimate if a mon is still alive by computing a difference of system time by
> doing a  "if (now - last_pg_stats_sent > max) {" where now & last_pg_stats_sent are the output of "ceph_clock_now()"
> 
> As ceph_clock_now() is using CLOCK_REALTIME, we could have cases where a simple drift caused by a NTP trigger that code.
> As we are only trying to compute a difference of absolute time (and not system time), using a MONOTONIC_* seems much more accurate at my taste.
> 
> I know that's pretty picky but that could exists.

That's a perfect example of where the monotonic time should be used. There 
are a zillion instances of this pattern that could be cleaned up... :)

> >I find it interesting. If you want to change more subsystems to use ceph_time
> >and use monotonic and coarse clocks in appropriate places I think that would be
> >wonderful. It might be worthwhile to focus on the places where we use double to
> >represent a time value.
> >Please read through ceph_time.h and osdc to get a feel for it. If you think it
> >could use improvement, I'd be happy to hear your ideas.
> 
> You mean we could switch some of ceph_clock_now() calls to ceph_time ? 
> We could use coarse_mono_clock to perform that.
> 
> If you agree that switching code like the one I'm speaking is valuable, 
> I can work on it.

That would be great!

sage

--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On 19/01/2016, Erwan Velu wrote:
> Before propagating the change to more code base and offer a PR with that, what do you think of this style ?
> 
> Is it aligned with the best-pratices of the project ?
> 
> https://gist.github.com/ErwanAliasr1/f74f7f046e9300ac2c30

One minor thing, you shouldn't use anything in time_detail directly. It SHOULD
be ceph::coarse_mono_clock::now() ('detail' or leading underscores signal a
'private' namespace.)

Other than that, this looks very good.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Erwan Velu]

Thanks for the review & insights.
I'll propagate the change over the code.

----- Mail original -----
De: "Adam C. Emerson" <aemerson@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Mardi 19 Janvier 2016 17:29:03
Objet: Re: About ceph_clock_now()

On 19/01/2016, Erwan Velu wrote:
> Before propagating the change to more code base and offer a PR with that, what do you think of this style ?
> 
> Is it aligned with the best-pratices of the project ?
> 
> https://gist.github.com/ErwanAliasr1/f74f7f046e9300ac2c30

One minor thing, you shouldn't use anything in time_detail directly. It SHOULD
be ceph::coarse_mono_clock::now() ('detail' or leading underscores signal a
'private' namespace.)

Other than that, this looks very good.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Erwan Velu]

Hi,

I'm sadly not at full time on Ceph but you can find my work in progress on this PR in my personal branch.
I'm thinking of making a single commit explaining this change and propagating it to the whole codebase: 
https://github.com/ErwanAliasr1/ceph/commit/00daa403831c9e0dfffb6f31a48723ca3cc3b569

Note this is WIP and I'm currently hiting obvious places where ceph_clock_now() can be replaced. I don't know a lot about the coebase so I'll ask my questions later about the locations where I have doubts.

Cheers,
Erwan

----- Mail original -----
De: "Adam C. Emerson" <aemerson@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Mardi 19 Janvier 2016 17:29:03
Objet: Re: About ceph_clock_now()

On 19/01/2016, Erwan Velu wrote:
> Before propagating the change to more code base and offer a PR with that, what do you think of this style ?
> 
> Is it aligned with the best-pratices of the project ?
> 
> https://gist.github.com/ErwanAliasr1/f74f7f046e9300ac2c30

One minor thing, you shouldn't use anything in time_detail directly. It SHOULD
be ceph::coarse_mono_clock::now() ('detail' or leading underscores signal a
'private' namespace.)

Other than that, this looks very good.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Erwan Velu]

Hey,

I've been able to continue this work and updated by branch accordingly.
I understand the benefit of using the ceph_time work but I feel that it makes the change pretty verbose for a not a so big change (CLOCK_REALTIME vs CLOCK_MONO).

This imply a change of all utime_t and makes the computations a little bit more complex to read.

Does it worth the time spent on it ? If yes, I don't have any issue continuing this way.
I'm pretty new to the project and would like to make the best PR as possible.
So your insights on the under-work patch would be very helpful.

https://github.com/ErwanAliasr1/ceph/tree/evelu-monotonic

Thanks,

----- Mail original -----
De: "Erwan Velu" <evelu@redhat.com>
À: "Adam C. Emerson" <aemerson@redhat.com>
Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Jeudi 21 Janvier 2016 20:22:56
Objet: Re: About ceph_clock_now()

Hi,

I'm sadly not at full time on Ceph but you can find my work in progress on this PR in my personal branch.
I'm thinking of making a single commit explaining this change and propagating it to the whole codebase: 
https://github.com/ErwanAliasr1/ceph/commit/00daa403831c9e0dfffb6f31a48723ca3cc3b569

Note this is WIP and I'm currently hiting obvious places where ceph_clock_now() can be replaced. I don't know a lot about the coebase so I'll ask my questions later about the locations where I have doubts.

Cheers,
Erwan

----- Mail original -----
De: "Adam C. Emerson" <aemerson@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Mardi 19 Janvier 2016 17:29:03
Objet: Re: About ceph_clock_now()

On 19/01/2016, Erwan Velu wrote:
> Before propagating the change to more code base and offer a PR with that, what do you think of this style ?
> 
> Is it aligned with the best-pratices of the project ?
> 
> https://gist.github.com/ErwanAliasr1/f74f7f046e9300ac2c30

One minor thing, you shouldn't use anything in time_detail directly. It SHOULD
be ceph::coarse_mono_clock::now() ('detail' or leading underscores signal a
'private' namespace.)

Other than that, this looks very good.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

On 22/01/2016, Erwan Velu wrote:
> Hey,
> 
> I've been able to continue this work and updated by branch accordingly.
> I understand the benefit of using the ceph_time work but I feel that it makes the change pretty verbose for a not a so big change (CLOCK_REALTIME vs CLOCK_MONO).
>
> This imply a change of all utime_t and makes the computations a little bit more complex to read.

There are a few changes one can use to make it less verbose. If you're not in a
header file (or inside a function), you might like to do:

using ceph::coarse_mono_clock;

(Don't do that in headers outside of functions, though.)

Then you can just do things like

auto now = mono_clock_coarse::now();

Which should be a bit less verbose.

Or even:

using cm_clock = ceph::coarse_mono_clock;

auto now = cm_clock::now();

> Does it worth the time spent on it ? If yes, I don't have any issue continuing this way.
> I'm pretty new to the project and would like to make the best PR as possible.
> So your insights on the under-work patch would be very helpful.

I think so. We definitely wnat to use monotonic clocks where appropriate. And we
can get a performance benefit from using coarse clocks where appropriate, too.
And we want to switch to the ceph_time stuff anyway just because it rules out a
whole class of bugs and integrates with standard and third-party libraries.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Erwan Velu]

Hey Adam,

I updated my branch based on your recommendations. Thanks.
I'm continuing my edit.

----- Mail original -----
De: "Adam C. Emerson" <aemerson@redhat.com>
À: "Erwan Velu" <evelu@redhat.com>
Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
Envoyé: Vendredi 22 Janvier 2016 18:35:22
Objet: Re: About ceph_clock_now()

On 22/01/2016, Erwan Velu wrote:
> Hey,
> 
> I've been able to continue this work and updated by branch accordingly.
> I understand the benefit of using the ceph_time work but I feel that it makes the change pretty verbose for a not a so big change (CLOCK_REALTIME vs CLOCK_MONO).
>
> This imply a change of all utime_t and makes the computations a little bit more complex to read.

There are a few changes one can use to make it less verbose. If you're not in a
header file (or inside a function), you might like to do:

using ceph::coarse_mono_clock;

(Don't do that in headers outside of functions, though.)

Then you can just do things like

auto now = mono_clock_coarse::now();

Which should be a bit less verbose.

Or even:

using cm_clock = ceph::coarse_mono_clock;

auto now = cm_clock::now();

> Does it worth the time spent on it ? If yes, I don't have any issue continuing this way.
> I'm pretty new to the project and would like to make the best PR as possible.
> So your insights on the under-work patch would be very helpful.

I think so. We definitely wnat to use monotonic clocks where appropriate. And we
can get a performance benefit from using coarse clocks where appropriate, too.
And we want to switch to the ceph_time stuff anyway just because it rules out a
whole class of bugs and integrates with standard and third-party libraries.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 22-1-2016 17:00, Erwan Velu wrote:
> Hey,
> 
> I've been able to continue this work and updated by branch accordingly.
> I understand the benefit of using the ceph_time work but I feel that it makes the change pretty verbose for a not a so big change (CLOCK_REALTIME vs CLOCK_MONO).
> 
> This imply a change of all utime_t and makes the computations a little bit more complex to read.
> 
> Does it worth the time spent on it ? If yes, I don't have any issue continuing this way.
> I'm pretty new to the project and would like to make the best PR as possible.
> So your insights on the under-work patch would be very helpful.

Erwan,

It would be real useful if references to CLOCK_(MONOTONIC|REALTIME)_*
are centralised in one place (or at least as little as possible).
And perhaps wrapped in simple function.

Reason is that CLOCK_*_COARSE are linux specific.

POSIX only has CLOCK_*, without the COARSE.
FreeBSD has the CLOCK_*_FAST variant which equals the COARSE objective.
(less accuracy, more speed)

So I've wrapped the code in a
#if defined(__linux__)
#	USE the COARSE variant
#elif defined(__FreeBSD__)
#	USE the FAST variant
#else
# 	Use the POSIX version as fallback
#endif

Thanx,
--WjW

Re: About ceph_clock_now()

[Matt Benjamin]

Hi Willem,

Agree.  The c++ notations and our declarations using them do centralize all this.  I don't know if we're actually defining anything equivalent to the coarse/fast clocks (it seems like a good idea, and I assumed we would), and it's straightforward to do.

(Adam may have, already.)

Matt

----- Original Message -----
> From: "Willem Jan Withagen" <wjw@digiware.nl>
> To: "Erwan Velu" <evelu@redhat.com>, "Adam C. Emerson" <aemerson@redhat.com>
> Cc: "Sage Weil" <sweil@redhat.com>, "The Sacred Order of the Squid Cybernetic" <ceph-devel@vger.kernel.org>
> Sent: Saturday, January 23, 2016 6:49:21 AM
> Subject: Re: About ceph_clock_now()
> 
> On 22-1-2016 17:00, Erwan Velu wrote:
> > Hey,
> > 
> > I've been able to continue this work and updated by branch accordingly.
> > I understand the benefit of using the ceph_time work but I feel that it
> > makes the change pretty verbose for a not a so big change (CLOCK_REALTIME
> > vs CLOCK_MONO).
> > 
> > This imply a change of all utime_t and makes the computations a little bit
> > more complex to read.
> > 
> > Does it worth the time spent on it ? If yes, I don't have any issue
> > continuing this way.
> > I'm pretty new to the project and would like to make the best PR as
> > possible.
> > So your insights on the under-work patch would be very helpful.
> 
> Erwan,
> 
> It would be real useful if references to CLOCK_(MONOTONIC|REALTIME)_*
> are centralised in one place (or at least as little as possible).
> And perhaps wrapped in simple function.
> 
> Reason is that CLOCK_*_COARSE are linux specific.
> 
> POSIX only has CLOCK_*, without the COARSE.
> FreeBSD has the CLOCK_*_FAST variant which equals the COARSE objective.
> (less accuracy, more speed)
> 
> So I've wrapped the code in a
> #if defined(__linux__)
> #	USE the COARSE variant
> #elif defined(__FreeBSD__)
> #	USE the FAST variant
> #else
> # 	Use the POSIX version as fallback
> #endif
> 
> Thanx,
> --WjW
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-707-0660
fax.  734-769-8938
cel.  734-216-5309

Re: About ceph_clock_now()

[Adam C. Emerson]

On 23/01/2016, Matt Benjamin wrote:
> Hi Willem,
> 
> Agree.  The c++ notations and our declarations using them do centralize all this.  I don't know if we're actually defining anything equivalent to the coarse/fast clocks (it seems like a good idea, and I assumed we would), and it's straightforward to do.
> 
> (Adam may have, already.)

We do. We can switch out ceph::coarse_mono_clock for BSD's FAST MONO CLOCK or on
systems that don't have a coarse/fast option, just fall back to CLOCK_MONOTONIC
easily.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 23-1-2016 22:55, Adam C. Emerson wrote:
> On 23/01/2016, Matt Benjamin wrote:
>> Hi Willem,
>>
>> Agree.  The c++ notations and our declarations using them do centralize all this.  I don't know if we're actually defining anything equivalent to the coarse/fast clocks (it seems like a good idea, and I assumed we would), and it's straightforward to do.
>>
>> (Adam may have, already.)
> 
> We do. We can switch out ceph::coarse_mono_clock for BSD's FAST MONO CLOCK or on
> systems that don't have a coarse/fast option, just fall back to CLOCK_MONOTONIC
> easily.
> 
Cool,

Last time I tried a testrun after catching up to HEAD, I got quite a few
compiletime errors.
Guess I've got to catch up again.

--WjW

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 24-1-2016 12:53, Willem Jan Withagen wrote:
> On 23-1-2016 22:55, Adam C. Emerson wrote:
>> On 23/01/2016, Matt Benjamin wrote:
>>> Hi Willem,
>>>
>>> Agree.  The c++ notations and our declarations using them do centralize all this.  I don't know if we're actually defining anything equivalent to the coarse/fast clocks (it seems like a good idea, and I assumed we would), and it's straightforward to do.
>>>
>>> (Adam may have, already.)
>>
>> We do. We can switch out ceph::coarse_mono_clock for BSD's FAST MONO CLOCK or on
>> systems that don't have a coarse/fast option, just fall back to CLOCK_MONOTONIC
>> easily.
>>
> Cool,
> 
> Last time I tried a testrun after catching up to HEAD, I got quite a few
> compiletime errors.
> Guess I've got to catch up again.

Not to irritate, but I just updated my copy of ceph:master and I could
not find any trace of a reference to CLOCK_MONOTONIC_FAST, so it has to
be in any of the WIPs ??

--WjW

Re: About ceph_clock_now()

[Adam C. Emerson]

On 24/01/2016, Willem Jan Withagen wrote:
> Not to irritate, but I just updated my copy of ceph:master and I could
> not find any trace of a reference to CLOCK_MONOTONIC_FAST, so it has to
> be in any of the WIPs ??

I had meant that there was an abstraction there, rather than that we had
CLOCK_MONOTONIC_FAST already. But, does:

https://github.com/ceph/ceph/pull/7340

Work under your system/seem a sane way of doing it?

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 25-1-2016 01:03, Adam C. Emerson wrote:
> On 24/01/2016, Willem Jan Withagen wrote:
>> Not to irritate, but I just updated my copy of ceph:master and I could
>> not find any trace of a reference to CLOCK_MONOTONIC_FAST, so it has to
>> be in any of the WIPs ??
> 
> I had meant that there was an abstraction there, rather than that we had
> CLOCK_MONOTONIC_FAST already. But, does:
> 
> https://github.com/ceph/ceph/pull/7340
> 
> Work under your system/seem a sane way of doing it?

That is 99,5% of what I had in my WIP.
The difference is that I matched it on __linux__ and  __FreeBSD__, but
yours is definitely even better, since it if-then-else on what is really
under observation.
I would comment the last section: the #else with a remark that you
select POSIX if nothing else, but that POSIX is more/unneeded accurate
and slow.

--WjW

Re: About ceph_clock_now()

[Erwan Velu]

>I would comment the last section: the #else with a remark that you
>select POSIX if nothing else, but that POSIX is more/unneeded accurate
>and slow.

I totally agree with your comment.
I'm pretty boring on that topic but keeping a well documented commit (in the commit message) that explains the following is very useful when reading a patch later.
- what was the issue
- how we fixed it

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 25-1-2016 10:58, Erwan Velu wrote:
>> I would comment the last section: the #else with a remark that you
>> select POSIX if nothing else, but that POSIX is more/unneeded accurate
>> and slow.
>
> I totally agree with your comment.
> I'm pretty boring on that topic but keeping a well documented commit (in the commit message) that explains the following is very useful when reading a patch later.
> - what was the issue
> - how we fixed it

I would even keep it in the code so that somebody looking at it understands
that the system default for a reason to slower POSIX.
Especially in porting circumstances this is important.
And even more so, now it is almost impossible to detect this compiletime.

I ran into it bacause only one option was there: *COARSE, so my build 
bombed out.
Now it it will select the slow POSIX if I don not have *COARSE or *FAST, 
and
merrily compile on.

So you'd only find this then running benchmarks and notice significant 
slowdown.

Come to think of it, it might even warrant a #warning line in the POSIX 
case,
to suggest to look into the 'man clock_gettime' page and see if the new 
OS has
something better.

--WjW

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On 25/01/2016, Willem Jan Withagen wrote:
> I would even keep it in the code so that somebody looking at it understands
> that the system default for a reason to slower POSIX.
> Especially in porting circumstances this is important.
> And even more so, now it is almost impossible to detect this compiletime.
[snip]
> Come to think of it, it might even warrant a #warning line in the POSIX
> case,
> to suggest to look into the 'man clock_gettime' page and see if the new OS
> has
> something better.

All right, I have pushed another version with comments and warnings.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 25-1-2016 19:00, Matt Benjamin wrote:
> Hi Willem,
> 
> This commit is by Adam.
Hi Matt,

Sorry about that, I'll send my personal remaks agin to him.

The point you raise here is a more generic question:
 -- How to wrap OS specifics into more generic code?

Sometimes overloading would be usefull. But in this case the function is
exactly oke, only the names of const values differ, and as such that
will not work. So a ceph::clock_gettime() would still have the same
problem.
Alternative is make different functions, but that would a lot of
functions, just to work around the naming.

> I have in OTHER PROJECTS mapped Linux CLOCK_MONOTONIC_COARSE to
> FreeBSD's CLOCK_MONOTONIC_FAST, but that's it.  In my Ceph code, I've
> been using clock_gettime() explicitly, but also
> CLOCK_MONOTONIC_COARSE, on the expectation to have this change in
> future.

'mmm sort of lost on what part of the code I then triggered and ended up
commenting on your code..

1)
Currently the code in Adam's looks like:
#if defined(CLOCK_MONOTONIC_COARSE)
        // Linux systems have _COARSE clocks.
        clock_gettime(CLOCK_MONOTONIC_COARSE, &ts);
#elif defined(CLOCK_MONOTONIC_FAST)
        // BSD systems have _FAST clocks.
        clock_gettime(CLOCK_MONOTONIC_FAST, &ts);
#else
        // And if we find neither, you may wish to consult your system's
        // documentation.
#warning Falling back to CLOCK_MONOTONIC, may be slow.
        clock_gettime(CLOCK_MONOTONIC, &ts);
#endif

And this is repeated in a few more locations.
Moreover, it will be required to do that a few times everytime
clock_getime is used.

Perhaps Matt solution is crude, but more convenient, which I think went
like:
#if defined(__FreeBSD__)
#define CLOCK_MONOTONIC_COARSE CLOCK_MONOTONIC_FAST
#endif
I keeps the code looking very Linux like, which works for this case.
But there could very well be that there is more difference than just
nameing, and tricky details of the implementation could bite.
It would also not work when there is more that just a nameing difference.

Put this in the compat.h or ceph_time.h file and this can tackle this
specific problem

> If you think the clock definition should be different, please suggest
> corrections (but again, I'm not the author of this proposed change).

I'm at the middle of the road of which version I like best.
The first one is 100% clear and obvious, but also very verbose.
The second one shines in elegance, but is not very transparent value wise.

If the second solution was put in ceph_time.h I think that that would
have my preference

--WjW

Re: About ceph_clock_now()

[Adam C. Emerson]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On 25/01/2016, Willem Jan Withagen wrote:
> 1)
> Currently the code in Adam's looks like:
> #if defined(CLOCK_MONOTONIC_COARSE)
>         // Linux systems have _COARSE clocks.
>         clock_gettime(CLOCK_MONOTONIC_COARSE, &ts);
> #elif defined(CLOCK_MONOTONIC_FAST)
>         // BSD systems have _FAST clocks.
>         clock_gettime(CLOCK_MONOTONIC_FAST, &ts);
> #else
>         // And if we find neither, you may wish to consult your system's
>         // documentation.
> #warning Falling back to CLOCK_MONOTONIC, may be slow.
>         clock_gettime(CLOCK_MONOTONIC, &ts);
> #endif
> 
> And this is repeated in a few more locations.
> Moreover, it will be required to do that a few times everytime
> clock_getime is used.

I was originally thinking of just specifying the time constant, but I figured
that somehow somewhere someone might want to compile on a system without
clock_gettime. (Windows? I don't know.) Or use the fasttime library or something
where just switching out the constant wouldn't work.

I wouldn't worry about people having to do this every tie they call
clock_gettime, though, since they really ought to be going through the C++ Clock
abstraction. (At least, in the context of the Ceph codebase.)

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 603 bytes --]

Re: About ceph_clock_now()

[Matt Benjamin]

inline

----- Original Message -----
> From: "Adam C. Emerson" <aemerson@redhat.com>
> To: "Willem Jan Withagen" <wjw@digiware.nl>
> Cc: "Matt Benjamin" <mbenjamin@redhat.com>, "Erwan Velu" <evelu@redhat.com>, "Ceph Development"
> <ceph-devel@vger.kernel.org>, "Sage Weil" <sweil@redhat.com>
> Sent: Monday, January 25, 2016 3:14:51 PM
> Subject: Re: About ceph_clock_now()
> 
> On 25/01/2016, Willem Jan Withagen wrote:
> > 1)
> > Currently the code in Adam's looks like:
> > #if defined(CLOCK_MONOTONIC_COARSE)
> >         // Linux systems have _COARSE clocks.
> >         clock_gettime(CLOCK_MONOTONIC_COARSE, &ts);
> > #elif defined(CLOCK_MONOTONIC_FAST)
> >         // BSD systems have _FAST clocks.
> >         clock_gettime(CLOCK_MONOTONIC_FAST, &ts);
> > #else
> >         // And if we find neither, you may wish to consult your system's
> >         // documentation.
> > #warning Falling back to CLOCK_MONOTONIC, may be slow.
> >         clock_gettime(CLOCK_MONOTONIC, &ts);
> > #endif
> > 
> > And this is repeated in a few more locations.
> > Moreover, it will be required to do that a few times everytime
> > clock_getime is used.
> 
> I was originally thinking of just specifying the time constant, but I figured
> that somehow somewhere someone might want to compile on a system without
> clock_gettime. (Windows? I don't know.) Or use the fasttime library or
> something
> where just switching out the constant wouldn't work.
> 
> I wouldn't worry about people having to do this every tie they call
> clock_gettime, though, since they really ought to be going through the C++
> Clock
> abstraction. (At least, in the context of the Ceph codebase.)

exactly, this.

> 
> --
> Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
> IRC: Aemerson@{RedHat, OFTC, Freenode}
> 0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9
> 

-- 
-- 
Matt Benjamin
Red Hat, Inc.
315 West Huron Street, Suite 140A
Ann Arbor, Michigan 48103

http://www.redhat.com/en/technologies/storage

tel.  734-707-0660
fax.  734-769-8938
cel.  734-216-5309

Re: About ceph_clock_now()

[Willem Jan Withagen]

On 25-1-2016 21:14, Adam C. Emerson wrote:
> On 25/01/2016, Willem Jan Withagen wrote:
>> 1)
>> Currently the code in Adam's looks like:
>> #if defined(CLOCK_MONOTONIC_COARSE)
>>         // Linux systems have _COARSE clocks.
>>         clock_gettime(CLOCK_MONOTONIC_COARSE, &ts);
>> #elif defined(CLOCK_MONOTONIC_FAST)
>>         // BSD systems have _FAST clocks.
>>         clock_gettime(CLOCK_MONOTONIC_FAST, &ts);
>> #else
>>         // And if we find neither, you may wish to consult your system's
>>         // documentation.
>> #warning Falling back to CLOCK_MONOTONIC, may be slow.
>>         clock_gettime(CLOCK_MONOTONIC, &ts);
>> #endif
>>
>> And this is repeated in a few more locations.
>> Moreover, it will be required to do that a few times everytime
>> clock_getime is used.
> 
> I was originally thinking of just specifying the time constant, but I figured
> that somehow somewhere someone might want to compile on a system without
> clock_gettime. (Windows? I don't know.) Or use the fasttime library or something
> where just switching out the constant wouldn't work.
> 
> I wouldn't worry about people having to do this every tie they call
> clock_gettime, though, since they really ought to be going through the C++ Clock
> abstraction. (At least, in the context of the Ceph codebase.)
> 
Well that is the essential part of all of this. If other parts of the
Ceph code are expected to go through the ceph_time abstraction, then
this is the only place we need to care for OS differences. And the more
clear and obvious, the better it is.

And we should stick with your version.

--WjW

Re: About ceph_clock_now()

[Adam C. Emerson]

On 13/01/2016, Erwan Velu wrote:
[snip]
> You consider the MONOTONIC clock drifting a lot ?
> I mean an NTP adjustement can make a serious jump forward or even worse backward.

So, CLOCK_MONOTONIC will never jump backward (thus the name). And there
is a system-defined limit on how much it will ever jump forward at any
given time, though I don't know what that value is offhand. On a
well-synchronized system, at least it should give more accurate durations than
CLOCK_MONOTONIC_RAW. That said, if you're coming up from a cold boot and you're
out of sync, it could be jumpier. As I said, I am not a Timelord, this is just
what I've gathered as received wisdom. 

[snip]
> You mean we could switch some of ceph_clock_now() calls to ceph_time ? 
> We could use coarse_mono_clock to perform that.
> 
> If you agree that switching code like the one I'm speaking is valuable, I can work on it.

I think that would be very valuable, thank you.

-- 
Senior Software Engineer           Red Hat Storage, Ann Arbor, MI, US
IRC: Aemerson@{RedHat, OFTC, Freenode}
0x80F7544B90EDBFB9 E707 86BA 0C1B 62CC 152C  7C12 80F7 544B 90ED BFB9