On Tue, Dec 15, 2015 at 12:36 PM, Barret Rhoden <[email protected]>
wrote:
> > 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.
>
Sorry, I applied that to the head of that file.
Now it is kind of a mess to apply only those to an older commit, when the
changes have been merged into that one.
Do you have any recipe?
>
>
> > 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)")
>
Dropped the commit.
> > +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.
>
Again, sorry. Is there an easy way to apply only those back, when these
plus other changes are in a future commit?
>
> > @@ -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.
>
A CPU reading version != 0, could go ahead and use values which have not
been populated.
>
> > 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.
>
Yeah, sorry about that.
> > #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)?
>
Why confusing? It just leaves 4 bits for domain.
>
> > 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?
>
They are machine dependent (they assume machine).
We could make them independent, but I am not sure it is worth the effort at
this time.
>
> > @@ -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?
>
No. If the profiler is not started, percpu_ctx is NULL.
>
> > 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.
>
How?
>
> > +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.
>
Yeah, those are gone in #version.
The -64b path issue still remains.
--
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.
