1014064 - Implement LoadInfo::UpdateSystemLoad() on BSDs

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan Beich]

Attachment: v1

This uses kern.cp_time to sum ticks in cpu_times and total_times. OpenBSD doesn't have sysctlbyname() while DragonFly/FreeBSD lack KERN_CP_TIME mib macro.

CP_INTR is excluded as noise after looking at /proc/stat emulation on NetBSD and FreeBSD. And unlike FreeBSD I don't try to convert from ticks to jiffies because OS X doesn't either.

http://bxr.su/NetBSD/sys/miscfs/procfs/procfs_linux.c
http://bxr.su/FreeBSD/sys/compat/linprocfs/linprocfs.c

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8426326 [details] [diff] [review]
v1

Review of attachment 8426326 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I have two small nits. Can you put an updated patch?

::: content/media/webrtc/LoadMonitor.cpp
@@ +44,5 @@
> +#include <sys/sched.h>
> +#endif
> +
> +#ifdef KERN_CPTIME
> +#define KERN_CP_TIME KERN_CPTIME

Is this needed for a specific BSD? If so move it into the corresponding ifdef.

@@ +438,5 @@
> +  }
> +
> +  const uint64_t cpu_times = cp_time[CP_NICE]
> +                           + cp_time[CP_SYS]
> +                           // + cp_time[CP_INTR]

Remove this or explain why it's commented out (like you did in the bug)

Comment 2

[Jan Beich]

Attachment: v1.1

(In reply to Gian-Carlo Pascutto [:gcp] from comment #1)
> ::: content/media/webrtc/LoadMonitor.cpp
> @@ +44,5 @@
> > +#include <sys/sched.h>
> > +#endif
> > +
> > +#ifdef KERN_CPTIME
> > +#define KERN_CP_TIME KERN_CPTIME
> 
> Is this needed for a specific BSD? If so move it into the corresponding
> ifdef.

Replaced with |#if defined(__OpenBSD__)| to know who has inconsistent underscores in mib macros in sys/sysctl.h. ;)

> 
> @@ +438,5 @@
> > +  }
> > +
> > +  const uint64_t cpu_times = cp_time[CP_NICE]
> > +                           + cp_time[CP_SYS]
> > +                           // + cp_time[CP_INTR]
> 
> Remove this or explain why it's commented out (like you did in the bug)

After looking at other userland consumers I'll just uncomment the line. Whether Linux distinguishes between interrupt time and system time is irrelevant here.

Comment 3

[Landry Breuil (:gaston)]

Tested the small snippet on openbsd, doesnt break :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/34482f7b45b6

Comment 4

[Jan Beich]

Attachment: uint64_t for NetBSD

I've missed NetBSD uses uint64_t for kern.cp_time -> 32bit archs would fall through to NS_ERROR_FAILURE -> harmless.

Probably too late to reindent the ifdef spaghetti.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34482f7b45b6

Comment 6

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfd0c53bdf28

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dfd0c53bdf28