Date: Fri, 1 Jul 2016 20:39:48 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r302252 - head/sys/kern Message-ID: <20160701185743.Q1600@besplex.bde.org> In-Reply-To: <20160701031549.GV38613@kib.kiev.ua> References: <201606281643.u5SGhNsi061606@repo.freebsd.org> <20160629175917.O968@besplex.bde.org> <20160629145443.GG38613@kib.kiev.ua> <20160629153233.GI38613@kib.kiev.ua> <20160630040123.F791@besplex.bde.org> <20160629211953.GK38613@kib.kiev.ua> <20160701005401.Q1084@besplex.bde.org> <20160630180106.GU38613@kib.kiev.ua> <20160701031549.GV38613@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 1 Jul 2016, Konstantin Belousov wrote:
> On Thu, Jun 30, 2016 at 09:01:06PM +0300, Konstantin Belousov wrote:
>> Yes, timehands for bootimebin should be the solution, but
>> not in the scope of this patch. I will work on this right after the
>> current changeset lands in svn.
>
> Well, there is the move of boottimebin into timehands. I also reduced
> the number of timehands to two, this was discussed many times before.
> The feed-forward code is probably broken right now, I did not even
> compile it.
That was fast.
It seems simple and clean enough, but is too much during a re freeze.
I will only make some minor comments about style.
> diff --git a/sys/compat/linprocfs/linprocfs.c b/sys/compat/linprocfs/linprocfs.c
> index 56b2ade..a0dce47 100644
> --- a/sys/compat/linprocfs/linprocfs.c
> +++ b/sys/compat/linprocfs/linprocfs.c
> @@ -447,9 +447,11 @@ linprocfs_dostat(PFS_FILL_ARGS)
> struct pcpu *pcpu;
> long cp_time[CPUSTATES];
> long *cp;
> + struct timeval boottime;
> int i;
>
> read_cpu_time(cp_time);
> + getboottime(&boottime);
This is used surprisingly often by too many subsystems. With the value still
broken so that locking it doesn't help much, I would leave it as a global.
> diff --git a/sys/kern/kern_tc.c b/sys/kern/kern_tc.c
> index 0f015b3..1c2d562 100644
> --- a/sys/kern/kern_tc.c
> +++ b/sys/kern/kern_tc.c
> @@ -70,31 +70,36 @@ struct timehands {
> ...
> -static struct timehands th9 = { NULL, 0, 0, 0, {0, 0}, {0, 0}, {0, 0}, 0, &th0};
> ...
> +static struct timehands th1 = {
> + .th_counter = NULL,
> + .th_adjustment = 0,
> + .th_scale = 0,
> + .th_offset_count = 0,
> + .th_offset = {0, 0},
> + .th_microtime = {0, 0},
> + .th_nanotime = {0, 0},
> + .th_boottime = {0, 0},
> + .th_generation = 0,
> + .th_next = &th0
> +};
This shouldn't spell out all the 0 initializers. That was only needed to
reach the non-0 initializer at the end of the initializer.
> static struct timehands th0 = {
> - &dummy_timecounter,
> - 0,
> - (uint64_t)-1 / 1000000,
> - 0,
> - {1, 0},
> - {0, 0},
> - {0, 0},
> - 1,
> - &th1
> + .th_counter = &dummy_timecounter,
> + .th_adjustment = 0,
> + .th_scale = (uint64_t)-1 / 1000000,
> + .th_offset_count = 0,
> + .th_offset = {1, 0},
> + .th_microtime = {0, 0},
> + .th_nanotime = {0, 0},
> + .th_boottime = {0, 0},
> + .th_generation = 1,
> + .th_next = &th1
> };
> @@ -135,14 +138,22 @@ SYSCTL_PROC(_kern_timecounter, OID_AUTO, alloweddeviation,
> ...
> static int
> sysctl_kern_boottime(SYSCTL_HANDLER_ARGS)
> {
> + struct bintime boottimebin;
> + struct timeval boottime;
> +
> + binuptime1(NULL, &boottimebin);
> + bintime2timeval(&boottimebin, &boottime);
Use the wrapper function getboottime() if you keep it.
> @@ -342,8 +365,8 @@ fbclock_getmicrotime(struct timeval *tvp)
> } while (gen == 0 || gen != th->th_generation);
> }
> #else /* !FFCLOCK */
> -void
> -binuptime(struct bintime *bt)
> +static void
> +binuptime1(struct bintime *bt, struct bintime *boottimebin)
> {
> struct timehands *th;
> u_int gen;
> @@ -351,13 +374,24 @@ binuptime(struct bintime *bt)
> do {
> th = timehands;
> gen = atomic_load_acq_int(&th->th_generation);
> - *bt = th->th_offset;
> - bintime_addx(bt, th->th_scale * tc_delta(th));
> + if (bt != NULL) {
> + *bt = th->th_offset;
> + bintime_addx(bt, th->th_scale * tc_delta(th));
> + }
> + if (boottimebin != NULL)
> + *boottimebin = th->th_boottime;
> atomic_thread_fence_acq();
> } while (gen == 0 || gen != th->th_generation);
> }
>
> void
> +binuptime(struct bintime *bt)
> +{
> +
> + binuptime1(bt, NULL);
> +}
Uptime functions don't use boottimebin, so it is ugly for the general
binuptime1() function to return it.
This is also pessimal. Maybe the compiler can optimize away the
boottimebin == NULL case for the uptime functions, but none of the
functions is explicitly inlined. I like functions not being inlined
when this is not explicit. gcc only inlines ones that are static
and called once. I don't like this, and turn it off using
-fno-inline-functions-called-once. clang is too broken to support
this flag.
So maybe use a new general function that returns boottimebin for the
non-uptime functions only. Possibly it can add the offset directly.
> @@ -1116,8 +1170,10 @@ sysclock_snap2bintime(struct sysclock_snap *cs, struct bintime *bt,
> if (cs->delta > 0)
> bintime_addx(bt, cs->fb_info.th_scale * cs->delta);
>
> - if ((flags & FBCLOCK_UPTIME) == 0)
> + if ((flags & FBCLOCK_UPTIME) == 0) {
> + binuptime1(NULL, &boottimebin);
Perhaps use a more direct way to get boottimebin(). binuptime1() is
pessimized by null pointer checks for both its args. Use the new
function getboottimebin() even if is not direct.
> ...
> diff --git a/sys/net/bpf.c b/sys/net/bpf.c
> index 3b12cf4..4251f71 100644
> --- a/sys/net/bpf.c
> +++ b/sys/net/bpf.c
> @@ -2328,12 +2328,13 @@ bpf_hdrlen(struct bpf_d *d)
> static void
> bpf_bintime2ts(struct bintime *bt, struct bpf_ts *ts, int tstype)
> {
> - struct bintime bt2;
> + struct bintime bt2, boottimebin;
> struct timeval tsm;
> struct timespec tsn;
>
> if ((tstype & BPF_T_MONOTONIC) == 0) {
> bt2 = *bt;
> + getboottimebin(&boottimebin);
> bintime_add(&bt2, &boottimebin);
> bt = &bt2;
> }
This is still too chummy with the (mis)implementation. I think it is to
convert a relative CLOCK_MONOTONIC time to an absolute CLOCK_REALTIME
time. That should be done in the time subsystem. It is unclear if
we care about strict real time with leap seconds adjustments.
BPF_T_MONOTONIC is not documented in any man page or used in any library
or applicatiion in /usr/src. BPF_T_BINTIME is documented bpf(4) but not
used in any library or application in /usr/src. Similarly for all of
BPF_T_*. contrib/libpcap has 47 lines matching timeval, 2 matching
timespec and 0 matching bintime.
> diff --git a/sys/netpfil/ipfw/ip_fw_sockopt.c b/sys/netpfil/ipfw/ip_fw_sockopt.c
> index d186ba5..7faf99f 100644
> --- a/sys/netpfil/ipfw/ip_fw_sockopt.c
> +++ b/sys/netpfil/ipfw/ip_fw_sockopt.c
> @@ -395,6 +395,7 @@ swap_map(struct ip_fw_chain *chain, struct ip_fw **new_map, int new_len)
> static void
> export_cntr1_base(struct ip_fw *krule, struct ip_fw_bcounter *cntr)
> {
> + struct timeval boottime;
>
> cntr->size = sizeof(*cntr);
>
> @@ -403,21 +404,26 @@ export_cntr1_base(struct ip_fw *krule, struct ip_fw_bcounter *cntr)
> cntr->bcnt = counter_u64_fetch(krule->cntr + 1);
> cntr->timestamp = krule->timestamp;
> }
> - if (cntr->timestamp > 0)
> + if (cntr->timestamp > 0) {
> + getboottime(&boottime);
> cntr->timestamp += boottime.tv_sec;
> + }
> }
Looks like another home made conversion from CLOCK_MONOTONIC to
CLOCK_REALTIME. Easier to understand since it is actually used.
ipfw seems to use only getmicrouptime() for timestamping. There are
good reasons for using monotonic time for everything except presenting
the time to the user and the conversion for that should be more careful
than the above.
> diff --git a/sys/nfs/nfs_lock.c b/sys/nfs/nfs_lock.c
> index 7d11672..c84413e 100644
> --- a/sys/nfs/nfs_lock.c
> +++ b/sys/nfs/nfs_lock.c
> @@ -241,6 +241,7 @@ nfs_dolock(struct vop_advlock_args *ap)
> struct flock *fl;
> struct proc *p;
> struct nfsmount *nmp;
> + struct timeval boottime;
>
> td = curthread;
> p = td->td_proc;
> @@ -284,6 +285,7 @@ nfs_dolock(struct vop_advlock_args *ap)
> p->p_nlminfo = malloc(sizeof(struct nlminfo),
> M_NLMINFO, M_WAITOK | M_ZERO);
> p->p_nlminfo->pid_start = p->p_stats->p_start;
> + getboottime(&boottime);
> timevaladd(&p->p_nlminfo->pid_start, &boottime);
> }
> msg.lm_msg_ident.pid_start = p->p_nlminfo->pid_start;
Looks like most uses of boottime are for this buggy addition. I would
only trust it for telling the user the boot time, though it is broken
for that too.
> diff --git a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c b/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
> index 1d07943..0879299 100644
> --- a/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
> +++ b/sys/rpc/rpcsec_gss/svc_rpcsec_gss.c
> @@ -504,11 +504,13 @@ svc_rpc_gss_find_client(struct svc_rpc_gss_clientid *id)
> {
> struct svc_rpc_gss_client *client;
> struct svc_rpc_gss_client_list *list;
> + struct timeval boottime;
> unsigned long hostid;
>
> rpc_gss_log_debug("in svc_rpc_gss_find_client(%d)", id->ci_id);
>
> getcredhostid(curthread->td_ucred, &hostid);
> + getboottime(&boottime);
> if (id->ci_hostid != hostid || id->ci_boottime != boottime.tv_sec)
> return (NULL);
Here it is hopefully just a magic id, with the user being a remote system.
Any time that doesn't go backwards or forwards so far that it is in the
lieftime of an old or new boot instance works well for identifying the
boot instance.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160701185743.Q1600>