Hi - I have a few follow up comments, see below.
Thanks, Barret > From 6105697c7a3ed895c06d1265b7db79d9ecf1c015 Mon Sep 17 00:00:00 2001 > From: Davide Libenzi <[email protected]> > Date: Fri, 27 Nov 2015 10:57:19 -0800 > Subject: Migrated devarch MSR access to new MSR API > @@ -362,22 +363,28 @@ static long archread(struct chan *c, void *a, long n, > int64_t offset) > + msr_set_values(&msrv, values, num_cores); > + > + err = msr_cores_read(&cset, &msra, &msrv); > + > + n = -1; > + if (likely(!err)) { > + if (n >= num_cores * sizeof(uint64_t)) { > + if (!memcpy_to_user_errno(current, a, values, > + num_cores * sizeof(uint64_t))) > + n = num_cores * sizeof(uint64_t); > } else { This didn't seem to get fixed or clarified. Here's my original comment: What is going on here? n == -1, then we see if it is great enough to cover the whole range of our output (which is always positive)? Oh, I found it later in commit f326ef1adcc. Please apply the fixups to the originally incorrect patch. Other than preserving a working git history, it saves your reviewers time. > From 99ac80e47013f91452d8e5c439ba0937a48576c0 Mon Sep 17 00:00:00 2001 > > From: Davide Libenzi <[email protected]> > > Date: Mon, 14 Dec 2015 13:45:25 -0800 > > Subject: Added ROS_STATIC_ASSERT to common.h and using that in assert.h > > > > Added ROS_STATIC_ASSERT to common.h and using that in assert.h. > We don't need this in ros/common.h, since userspace already has their own. It's parlib_static_assert(), added by Dan in 4402cbebfec6 ("Rename static_assert to avoid conflicting with C++'11 (and later) (XCC)") > From f5e38ab6bac9077cebedf147758cb3034156106b Mon Sep 17 00:00:00 2001 > From: Davide Libenzi <[email protected]> > Date: Mon, 16 Nov 2015 07:13:13 -0800 > Subject: Added perfmon interrupt handling to allow overflow based profiling > +static uint64_t perfmon_get_fixevent_mask(const struct perfmon_event *pev, > + > int eventno, uint64_t base) > +{ > + uint64_t m = 0; > + > + if (pev->u.b.inten) > + m |= 1 << 3; > + if (pev->u.b.os) > + m |= (1 << 0); > + if (pev->u.b.usr) > + m |= (1 << 1); This shouldn't compile on this commit. You change perfmon_event to use the bitfield macros, but didn't change this. I guess you change it in a later commit, since the final branch builds. There are other places in this file that need the same fixup. > From 4d09b83f75fef0da023624d6ba504480511fb646 Mon Sep 17 00:00:00 2001 > From: Davide Libenzi <[email protected]> > Date: Sat, 12 Dec 2015 13:35:31 -0800 > Subject: Enable the PFM sampling to pass an 64bit info value > @@ -55,13 +56,13 @@ static void perfmon_read_cpu_caps(struct perfmon_cpu_caps > *pcc) > > cpuid(0x0a, 0, &a, &b, &c, &d); > > - ZERO_DATA(*pcc); > - pcc->perfmon_version = a & 0xff; > pcc->proc_arch_events = a >> 24; > pcc->bits_x_counter = (a >> 16) & 0xff; > pcc->counters_x_proc = (a >> 8) & 0xff; > pcc->bits_x_fix_counter = (d >> 5) & 0xff; > pcc->fix_counters_x_proc = d & 0x1f; > + wmb_f(); > + pcc->perfmon_version = a & 0xff; > } What is going on here? Is this an attempt to make it race-free? If anything this should be a fixup for the original commit, but I'm not even sure what it's doing. > static void perfmon_enable_event(int event, bool enable) > @@ -96,11 +97,11 @@ static uint64_t perfmon_get_fixevent_mask(const struct > perfmon_event *pev, > { > uint64_t m = 0; > > - if (pev->u.b.inten) > + if (PMEV_GET_EN(pev->event)) > m |= 1 << 3; > - if (pev->u.b.os) > + if (PMEV_GET_OS(pev->event)) > m |= (1 << 0); > - if (pev->u.b.usr) > + if (PMEV_GET_USR(pev->event)) > m |= (1 << 1); > m <<= eventno * FIXCNTR_NBITS; Ah, these (and others) should fixup commit #17 that changed the bitfields. > --- a/kern/include/ros/profiler_records.h > +++ b/kern/include/ros/profiler_records.h > @@ -7,9 +7,22 @@ > > #include <sys/types.h> > > +#define PROF_DOM_SHIFT (8 * sizeof(uint64_t) - 4) > +#define PROF_INFO_MASK (((uint64_t) 1 << PROF_DOM_SHIFT) - 1) This is a little confusing. The DOM shift is just 60, right? Is that value special (hence the 8 * sizeof - 4)? > struct proftype_kern_trace64 { > + uint64_t info; > uint64_t tstamp; > uint16_t cpu; > uint16_t num_traces; Subject for a future patch: are these structures copied in native endianness, or in an endian-independent format? > @@ -490,7 +511,7 @@ void profiler_notify_mmap(struct proc *p, uintptr_t addr, > size_t size, int prot, > int flags, struct file *f, > size_t offset) > { > if (kref_get_not_zero(&profiler_kref, 1)) { > - if (f && (prot & PROT_EXEC) && profiler_percpu_ctx && tracing) { > + if (f && (prot & PROT_EXEC) && profiler_percpu_ctx) { > char path_buf[PROFILER_MAX_PRG_PATH]; > char *path = file_abs_path(f, path_buf, > sizeof(path_buf)); > > @@ -504,7 +525,7 @@ void profiler_notify_mmap(struct proc *p, uintptr_t addr, > size_t size, int prot, > void profiler_notify_new_process(struct proc *p) > { > if (kref_get_not_zero(&profiler_kref, 1)) { > - if (profiler_percpu_ctx && tracing && p->binary_path) > + if (profiler_percpu_ctx && p->binary_path) > profiler_push_new_process(p); > kref_put(&profiler_kref); > } So now we're emitting trace info on all mmaps and process creation, even if the profiler is turned off? > From d2e4bae5a18d21205bf5de243450794a91674386 Mon Sep 17 00:00:00 2001 > From: Davide Libenzi <[email protected]> > Date: Sun, 13 Dec 2015 13:58:20 -0800 > Subject: Emit build information file into KFS /etc/build.info > +KERNEL_ELF_PATH=$(abspath $(KERNEL_OBJ))-64b This -64b is arch specific. We'll want to get this variable from the arch/Makefile. > +create-build-file: > +ifneq ($(INVARIANT_BUILD),1) Can drop the INVARIANT_BUILD, esp since we don't have a CONFIG_ var for it. > + @echo "KernelPath: $(KERNEL_ELF_PATH)" > kern/kfs/etc/build.info > + @echo "KernelSize: $(shell stat -c %s $(KERNEL_ELF_PATH))" >> \ > + kern/kfs/etc/build.info > + @echo "Date: `date`" >> kern/kfs/etc/build.info > + @echo "Host: `hostname`" >> kern/kfs/etc/build.info Use $(FIRST_KFS_PATH) instead of kern/kfs for all of these. (or rather use $(FIRST_KFS_PATH) to create KFS_BUILD_INFO_PATH or something). If you're removing this in an upcoming patchset (the #version stuff), then don't worry about any of this stuff. On 2015-12-14 at 14:27 "'Davide Libenzi' via Akaros" <[email protected]> wrote: > Done. > > > On Mon, Dec 14, 2015 at 2:22 PM, Davide Libenzi <[email protected]> > wrote: > > > Adding the "pa" thing now. Hold one ... > > > > > > On Mon, Dec 14, 2015 at 2:20 PM, Davide Libenzi > > <[email protected]> wrote: > > > >> Yes, it's a Linux perf record injected. > >> Plus, we would need something, if we plan to implement uname. > >> > >> > >> On Mon, Dec 14, 2015 at 2:19 PM, Barret Rhoden > >> <[email protected]> wrote: > >> > >>> On 2015-12-14 at 14:18 "'Davide Libenzi' via Akaros" > >>> <[email protected]> wrote: > >>> > Currently the knowledge of the Linux perf file format is kept > >>> > at a minimum. Injecting that information at Linux side would > >>> > require heavier changes to the Linux perf tool, which I > >>> > intentionally kept at a minimum, to minimize conflicts in > >>> > applying to new versions. Similarly, if I have to crack open > >>> > the perf file at Linux side, we will have to have another tools > >>> > on the Linux side. > >>> > >>> Makes sense. So it sounds like there is a perf record or > >>> something with the kernel binary location? > >>> > >>> -- > >>> You received this message because you are subscribed to the Google > >>> Groups "Akaros" group. > >>> To unsubscribe from this group and stop receiving emails from it, > >>> send an email to [email protected]. > >>> To post to this group, send email to [email protected]. > >>> For more options, visit https://groups.google.com/d/optout. > >>> > >> > >> > > > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
