Closed Bug 1014064 Opened 5 years ago Closed 5 years ago

Implement LoadInfo::UpdateSystemLoad() on BSDs

Categories

(Core :: WebRTC, defect)

x86_64
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: jbeich, Assigned: jbeich)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
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
Attachment #8426326 - Flags: review?(pkerr)
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)
Attachment #8426326 - Flags: review?(pkerr) → review-
Attached patch v1.1Splinter Review
(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.
Attachment #8426326 - Attachment is obsolete: true
Attachment #8426413 - Flags: review?(gpascutto)
Attachment #8426413 - Flags: review?(gpascutto) → review+
Tested the small snippet on openbsd, doesnt break :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/34482f7b45b6
Keywords: checkin-needed
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.
Attachment #8426576 - Flags: review?(gpascutto)
https://hg.mozilla.org/mozilla-central/rev/34482f7b45b6
Assignee: nobody → jbeich
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8426576 - Flags: review?(gpascutto) → review+
Depends on: 1016631
You need to log in before you can comment on or make changes to this bug.