On Mon, Dec 14, 2015 at 10:22 AM, Barret Rhoden <[email protected]>
wrote:
> > 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
>
I need to fix my pre-commit, as otherwise I will be for sure forgetting to
run it.
Have a big NO at commit time is much better for 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.
>
Should I amend the commit, or add a README into the library (or both)?
> 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.
>
Well, multiple call from different tasks can come in, and the operations
against the per CPU counters are definitely not atomic.
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;
>
While that code is running, the "pa" is not exposed anywhere.
Once the "pa" is out there, nobody will be changing it.
>
> 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.
>
Should I add?
>
> > > 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.
>
Let me leave the bitfield, and try to add a static assert to catch if GCC
decide to go sideways.
> > 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.
>
I will revise that with Makefrag-user-app idea, until we decide for the
future layout.
--
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.
