Message-ID: <CAEf4Bzb9KA=mzYo_x42ExRoZjm=dF6up1DxrUL_eqkDYs9+UUg@mail.gmail.com>
Date:   Tue, 28 Jul 2020 22:09:52 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     "Nikravesh, Ashkan" <ashkan.nikravesh@...el.com>
Cc:     Maciej Żenczykowski <maze@...gle.com>,
        "Pujari, Bimmy" <bimmy.pujari@...el.com>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>,
        "Alvarez, Daniel A" <daniel.a.alvarez@...el.com>
Subject: Re: [PATCH] bpf: Add bpf_ktime_get_real_ns

On Tue, Jul 28, 2020 at 1:00 PM Nikravesh, Ashkan
<ashkan.nikravesh@...el.com> wrote:
>
>
>
> ________________________________
> From: Andrii Nakryiko <andrii.nakryiko@...il.com>
> Sent: Tuesday, July 28, 2020 11:28 AM
> To: Maciej Żenczykowski <maze@...gle.com>
> Cc: Pujari, Bimmy <bimmy.pujari@...el.com>; bpf <bpf@...r.kernel.org>; Networking <netdev@...r.kernel.org>; mchehab@...nel.org <mchehab@...nel.org>; Alexei Starovoitov <ast@...nel.org>; Daniel Borkmann <daniel@...earbox.net>; Martin Lau <kafai@...com>; Nikravesh, Ashkan <ashkan.nikravesh@...el.com>
> Subject: Re: [PATCH] bpf: Add bpf_ktime_get_real_ns
>
> On Tue, Jul 28, 2020 at 10:57 AM Maciej Żenczykowski <maze@...gle.com> wrote:
> >
> > On Mon, Jul 27, 2020 at 10:01 PM Andrii Nakryiko <andrii.nakryiko@...il.com> wrote:
> >>
> >> On Mon, Jul 27, 2020 at 4:35 PM <bimmy.pujari@...el.com> wrote:
> >> >
> >> > From: Ashkan Nikravesh <ashkan.nikravesh@...el.com>
> >> >
> >> > The existing bpf helper functions to get timestamp return the time
> >> > elapsed since system boot. This timestamp is not particularly useful
> >> > where epoch timestamp is required or more than one server is involved
> >> > and time sync is required. Instead, you want to use CLOCK_REALTIME,
> >> > which provides epoch timestamp.
> >> > Hence add bfp_ktime_get_real_ns() based around CLOCK_REALTIME.
> >> >
> >>
> >> This doesn't seem like a good idea.
> >
> >
> > I disagree.
> >
> >>
> >> With time-since-boot it's very
> >> easy to translate timestamp into a real time on the host.
> >
> >
> > While it's easy to do, it's annoying, because you need to hardcode the offset into the bpf program
> > (which has security implications) or use another bpf array lookup/read (perf implications).
>
> There is no array lookup/read if you are using a global variable for this.
>
> [AN] Yes, as Maciej mentioned, converting since-boot-time to epoch in kernel has performance implication when the epoch timestamp/offset is provided by user-space and hard coding the offset doesn't seem to be a good idea. I am not sure what you refer by using a global variable.
>

This is global variable:


/* this gets initialized from user-space,
 * see tools/bpf/runqslower for examples,
 * or many of selftests doing this
 */
const volatile __u64 boot_to_wall_off_ns = 0;

SEC("...")
int my_handler(...)
{
    u64 wallclock_ns = bpf_ktime_get_boot_ns() + boot_to_wall_off_ns;

    ...
}

There is no map lookup involved, boot_to_wall_off_ns is accessed directly.

> >
> >>
> >> Having
> >> get_real_ns() variant might just encourage people to assume precise
> >> wall-clock timestamps that can be compared between within or even
> >> across different hosts.
> >
> >
> > In some environments they *can*.  Within a datacenter it is pretty common
> > to have extremely tightly synchronized clocks across thousands of machines.
>
> In some, yes, which also means that in some other they can't. So I'm
> still worried about misuses of REALCLOCK, within (internal daemons
> within the company) our outside (BCC tools and alike) of data centers.
> Especially if people will start using it to measure elapsed time
> between events. I'd rather not have to explain over and over again
> that REALCLOCK is not for measuring passage of time.
>
> [AN] Time synchronization fidelity or accuracy requirement is driven by the use-case.  A time synchronization is not the question here, you assume that your time synchronization accuracy is sufficient for your use-case.  You can always aim for better accuracy. For instance, the proposed helper function can be used as a mechanism to communicate the timestamp. For this, one use-case could be to measure e2e latency by adding egress timestamp to the packet. To do that, we sync the clock across all the hosts using NTP within a data center.

You are talking about use cases where you guys are conscious about
implications of using wallclock timestamps, and I'm talking about
general BPF users that might not know about any of these nuances, but
seeing bpf_ktime_get_real_ns() would be happy that it's "real" and
would just use it for everything.

> For instance, the proposed helper function can be used as a mechanism to communicate the timestamp.

See example above, can `bpf_ktime_get_boot_ns() + boot_to_wall_off_ns`
be used similarly for cross-host timestamp comparison?

>
> >
> >>
> >> REALCLOCK can jump around, you can get
> >> duplicate timestamps, timestamps can go back in time, etc.
> >
> >
> > That's only true if you allow it to.  On a server machine, once it's booted
>
> Right, but BPF is used so widely that it will be true at least in some cases.
>
> > up and time is synchronized, it no longer jumps around, only the frequency
> > is slowly adjusted to keep things in sync.
> >
> >>
> >> It's just not a good way to measure time.
> >
> >
> > It doesn't change the fact that some packets/protocols include real world timestamps.
> > We already have the since-boot time fetchers, so this doesn't prevent you from
> > using that - if that is what you want.
> >
> > Also one thing to remember is that a lot of time you only need ~1s precision.
> > In which case 'real' can simply be a lot easier to deal with.
> >
>
> Again, I'm worried about ensuing confusion by users trying to pick
> which variant to use. I'm not worried about those who understand the
> difference and can pick the right one for their needs, I'm worried
> about those that don't even give it a second thought. Given it's
> rather simple to convert boot time into wall-clock time, it doesn't
> seem like a necessary addition.
>
> >>
> >> Also, you mention the need for time sync. It's an extremely hard thing
> >> to have synchronized time between two different physical hosts, as
> >> anyone that has dealt with distributed systems will attest. Having
> >> this helper will just create a dangerous illusion that it is possible
> >> and will just cause more problems down the road for people.
> >
> >
> > It is possible.  But yes.  It is very very hard.
> > You can read up on Google TrueTime as an example real world implementation.
>
> Thank you, I did, though quite a while ago. Notice, I didn't say it's
> impossible, right? ;) But then Google TrueTime provides a range of
> time within confidence intervals, not a single seemingly-deterministic
> timestamp. And it needs custom hardware, so it's not realistic to
> expect to have it everywhere :)
>
> >
> >>
> >>
> >> > Signed-off-by: Ashkan Nikravesh <ashkan.nikravesh@...el.com>
> >> > Signed-off-by: Bimmy Pujari <bimmy.pujari@...el.com>
> >> > ---
> >> >  drivers/media/rc/bpf-lirc.c    |  2 ++
> >> >  include/linux/bpf.h            |  1 +
> >> >  include/uapi/linux/bpf.h       |  7 +++++++
> >> >  kernel/bpf/core.c              |  1 +
> >> >  kernel/bpf/helpers.c           | 14 ++++++++++++++
> >> >  kernel/trace/bpf_trace.c       |  2 ++
> >> >  tools/include/uapi/linux/bpf.h |  7 +++++++
> >> >  7 files changed, 34 insertions(+)
> >>
> >> [...]
> >
> >
> > Maciej Żenczykowski, Kernel Networking Developer @ Google

Powered by blists - more mailing lists