Hi -
On 2015-12-11 at 12:09 "'Davide Libenzi' via Akaros"
<[email protected]> wrote:
> I left how it was. But I do have checkpatch in my pre-commit hook,
> and I wonder why it did not trigger.
> This is the pre-commit I have (given to me by Kevin):
>
> https://gist.github.com/dlibenzi/6fbcd64c2e81eb430226
>
> WARNING: Do not use whitespace before Signed-off-by:
>
> Hmm, this is git-s doing that.
> I fixed the others.
Not sure about why the pre-commit hooks don't work. They also fail to
checkpatch the commit message (sometimes they catch typos and lack of
signed-off tags). I have a script that I run to checkpatch on a
revision list:
$ cat ~/bin/git-checkpatch
#!/bin/bash
TMPDIR=/tmp/fp-cp
usage()
{
echo "$0 rev..list"
exit -1
}
if [ $# -ne 1 ]
then
usage
fi
mkdir -p $TMPDIR
git format-patch -o $TMPDIR $1
./scripts/checkpatch.pl $TMPDIR/*.patch
rm $TMPDIR/*patch
> > > Added libpfm4 library support. The libpfm4 library provides a
> > > database of counters available on different Intel platforms.
> >
> > Where did this library come from? (URL, package name, etc).
> >
>
> Apparently from a guy sitting next to me π
> Did not explicitly add the links, as there is little confusion:
>
> https://www.google.com/search?q=libpfm4
We've been a little lax with adding code and dealing with licenses in
the past. I'd like it to be clear from the commit message where code
came from. I take it then that you added libpfm4-4.6.0 or something
from http://perfmon2.sourceforge.net/, possibly from
git://git.code.sf.net/p/perfmon2/libpfm4.
> The library has no dependencies.
> The code was literally taken (removed some Linux pref specific files)
> and added to the perfmon folder with its own Makefile.
Good. That's good to know too for the future, in case we need to bring
in an updated version of libpfm.
> > Mostly curious, does the declaration also need the attribute? I
> > tried with and without on a PERCPU that I dropped in init.c and it
> > didn't seem to care one way or the other.
> >
>
> Apparently it doesn't.
> I would slightly prefer to leave it there, in case GCC might decide
> to care later on π
Sounds good to me. =)
> > Should the enabling of the event happen before or after setting the
> > event? If
> > it happens before (as is here) would we collect perf events from
> > the old setting
> > of the fixed counter? Same goes for down below in the non-fixed
> > counter. perfmon_do_cores_free() looks okay (disables the event
> > before changing the fields).
> >
>
> It does not change much either ways, as the real programming comes
> when we write the CTRL register.
sounds good. this line from the doc cleared it up for me:
"Each enable bit in IA32_PERF_GLOBAL_CTRL is ANDβed with
the enable bits for all privilege levels in the respective
IA32_PERFEVTSELx or IA32_PERF_FIXED_CTR_CTRL MSRs to start/stop
the counting of respective counters.
> > What is the lock protecting? (Both here and in the other
> > perfmon_do_ functions). I imagine we need to disable interrupts,
> > in case a PMI IRQ fires,
> > but do we need to serialize these accesses across the entire
> > machine?
> >
>
> The could be per-CPU locks, but given that this is not an extremely
> hot path, I chose to not do that.
> Turned into per-CPU locks.
That cuts down on the lock contention, but I'm still curious what the
lock is protecting. If you're just trying to protect from an
interrupt causing someone to muck with the state, then a lock isn't
needed.
Still, I'm fine with the lock (per cpu or global). I (and
perhaps whoever reads the code in the future) just want to know if
there's something to be worried about.
> > Is 'pa' basically read-only once we install_session_alloc in
> > perfmon_open_event?
> > If not, how do we protect from having multiple users?
> >
> > If that's true and it's the invariant that keeps us from clobbering
> > pa's state,
> > then please put that in the comments somewhere. Otherwise someone
> > in the future
> > will modify this and enter a nightmare.
> >
>
> Does not matter. perf alloc (pa) are immutable and
> perfmon_alloc_get() gets a reference on them.
OK. So they are immutable after all of the cores run
perfmon_do_cores_alloc() (due to this):
pa->cores_counters[core_id()] = (counter_t) i;
which is before perfmon_install_session_alloc().
That's the sort of thing that could catch someone in the future, even
if it is unlikely. I wasn't asking for the code to change; I was just
saying that a comment would have made it more clear both for me and for
future developers. We ought to be explicit about any of the invariants
about locking or concurrent access.
> > If you can avoid the bitfields in the future, that'd be nice. We
> > try to avoid
> > using them where possible, in favor of masks and shifts. If this
> > is hard to
> > change here, then don't bother.
> >
>
> It makes code much cleaner IMO.
> You had to create macros (or open code them) for getting/setting every
> one of them.
> Which is pretty tedious when you have that many.
I agree that they look nice. My concern with bitfields is that the
compiler might not do what we want with them. I'm not a language
expert here, but here's a sample of some of the issues:
http://stackoverflow.com/questions/6043483/why-bit-endianness-is-an-issue-in-bitfields/6044223#6044223
Again, if this is hard to change here, then don't bother. It's not a
huge deal.
> > Minor thing, but if you read *kptr and incremented it, like you do
> > later with
> > the get_le_ helpers, you wouldn't need to remember to +1 it the
> > first time you
> > use it in every case {}.
> >
>
> I would have loved that, if you (and Kevin) corrected me previously in
> doing *ptr++ things π
> Changed to sane-c-code now.
I think our concern was that *ptr++ can be a little confusing for
people not intimately familiar with the order of operations. We had a
bug a while back where *ptr++ was confused with (*ptr)++. Or was it
confused with *(ptr++)? =)
Either way, reading the cmd and advancing the pointer seems sane. =)
> > Not that we actually support more than 2^16 cores yet, but
> > num_cores is an int.
> >
>
> Yeah, I figured, by the time we hit 65K cores, Intel might have
> changed the perf API 10 times, so this code might need revisioning
> anyway π
Haha, absolutely.
> > Added new perf utility to access CPU counters from userspace.
> >
> > Like the other tools such as snc and kprof2perf, this can't be
> > built by cd-ing
> > into the directory and running make. I'd like to fix that before
> > it gets out of
> > hand, though maybe the cure is worse than the symptoms for now.
> >
>
> I asked the question a few times, about the revised build system. Got
> no answers.
I guess coming up with the right answer for a build system is a little
more involved. There are ways in which we can use the current system
and still have the app Makefiles know about the CC, but I'll look into
it on my own.
> > As an FYI, the process will only run on the provisioned cores once
> > it becomes an
> > MCP. If you are running an app that is an SCP, then it will never
> > run on those
> > cores. If it will be an MCP, you won't capture the events from
> > when it was on
> > core 0 either.
> >
>
> So, are there better options?
Nope. I just wanted to make sure everyone was aware of that limitation.
> Changes have been posted in the same branch.
Thanks!
Barret
--
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.
