On Tue, Jul 28, 2020 at 10:57 AM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote: > > On Mon, Jul 27, 2020 at 10:01 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: >> >> On Mon, Jul 27, 2020 at 4:35 PM <bimmy.pujari@xxxxxxxxx> wrote: >> > >> > From: Ashkan Nikravesh <ashkan.nikravesh@xxxxxxxxx> >> > >> > 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. > >> >> 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. > >> >> 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@xxxxxxxxx> >> > Signed-off-by: Bimmy Pujari <bimmy.pujari@xxxxxxxxx> >> > --- >> > 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
