Code Review: Added new #version device with kernel build information

[Davide Libenzi]

The new machinery create an info file like:

KernelPath: /usr/local/google/home/src/akaros/akaros/obj/kern/akaros-kernel-64b

Date: Mon Dec 14 20:21:26 PST 2015

Host: dlibenzi.mtv.corp.google.com

CommitID: 769cc0c093f9c9b9f41c6387d0272e317e0e62c3

And stick it into the kernel ELF file.

The new device reads the information out into separate files (path, date, host, commit).

This branch is based upon the devarch_msr_perf one.

https://github.com/dlibenzi/akaros/compare/devarch_msr_perf...dlibenzi:build_info

The following changes since commit b1e8b1a61a4e0493ef3605910477459fe30eaa43:

  Move Linux perf format conversion into perf tool, drop kprof2perf (2015-12-14 14:27:06 -0800)

are available in the git repository at:

  g...@github.com:dlibenzi/akaros build_info

for you to fetch changes up to 25c241765f4395cd9ff910688c5f4afc91b8c2d1:

  Added #version device (2015-12-14 20:23:39 -0800)

----------------------------------------------------------------

Davide Libenzi (3):

      Added makefile machinary to generate a build info ELF section

      Added memstr() API to allow to search strings in memory

      Added #version device

 Makefile                   |  39 +++++++----

 kern/drivers/dev/Kbuild    |   1 +

 kern/drivers/dev/version.c | 157 +++++++++++++++++++++++++++++++++++++++++++++

 kern/include/string.h      |   1 +

 kern/src/strstr.c          |  19 ++++++

 5 files changed, 203 insertions(+), 14 deletions(-)

 create mode 100644 kern/drivers/dev/version.c

[Davide Libenzi]

As far as Akaros perf, I will append a new commit there.

I needed the kernel size also, in theory, but then it's like the dog biting his tail 😀

I don't know it until I am creating it.

But, I don't really need that.

Linux perf wants to know which ELF to use to look for symbols at a given address, and I can assume from the kernel load address, up-a-while, only the kernel ELF is there.

So I can just pass a relatively big number as size, and be done with it.

[ron minnich]

This is a pretty sweet device but I have a suggestion. Would it be possible to turn those variables (kernelpath, commitid, and so on) into symbols, something *equivalent* to this (I'm not saying this is the only way to do it):

char *KernelPath  = "whatever"

and so on, i.e. just generate a C file full of such declarations, then in your device, you can, given the QID you get from the walk, on open, use QID.path to index into an array of pointers to those variables, then use the

readstr

func to copy the value out to user mode?

This would avoid the need to parse the variables. Just an idea.

If you're not liking the use of the QID.Path you could still maybe do this in a way that avoids searching for the : and so on.

ron

--
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 akaros+un...@googlegroups.com.
To post to this group, send email to aka...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Davide Libenzi]

You got it Ron! :)

[Davide Libenzi]

Branch updated for Ron's likings ☺

[Davide Libenzi]

One question. Should we emit newline on those version strings?

Currently, it is just the raw data.

Linux does emit newline, at least in the most I tried.

[Davide Libenzi]

Changed to emit NL. Cat to console is pretty ugly otherwise.

The raw data exported in build_info.c is raw though (no NL char at end).

[Davide Libenzi]

As it turns out, readstr/readmem are not only used to copy to user memory, hence I had to add a check if the destination address is or not, within user land, and use the proper copy API.

[Barret Rhoden]

On 2015-12-14 at 20:28 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> Added memstr() API to allow to search strings in memory

i haven't pulled the patchset down to look closely yet, but I think we
already have something for memstr()

void *sigscan(uint8_t *address, int length, char *signature);

in k/s/string.c

[Davide Libenzi]

You can drop that commit if you like. It is not used anymore.

I thought it useful to have it.

If that sigscan API does it, that's OK. It's name is not exactly telling it though ☺

[Davide Libenzi]

Hold on on this review, using the readstr, I have found another issue.

Also, I am adding a numeric version and name as well:

VERSION = 0

PATCHLEVEL = 1

SUBLEVEL = 0

EXTRAVERSION =

VERNAME = Nanwan

KERNELVERSION = $(VERSION)$(if $(PATCHLEVEL),.$(PATCHLEVEL)$(if $(SUBLEVEL),.$(SUBLEVEL)))$(EXTRAVERSION)

Question about that big line.

It is exceeding 80 chars, but if I break it with \, a space is added in any case to the resulting string.

Any idea?

[Davide Libenzi]

Does Akaros Kbuild support the clean-files directive?

I need to clean a file at "clean", but it does not seem to be picking it up.

[Davide Libenzi]

Fuggetaboudit. Found it ☺

[Davide Libenzi]

Branch is ready now:

/ $ ls -l \#version

-r--r--r--    2 0        0                0 Jan  1  1970 commitid

-r--r--r--    2 0        0                0 Jan  1  1970 date

-r--r--r--    2 0        0                0 Jan  1  1970 host

-r--r--r--    2 0        0                0 Jan  1  1970 kernel_path

-r--r--r--    2 0        0                0 Jan  1  1970 version

-r--r--r--    2 0        0                0 Jan  1  1970 version_name

/ $ cat \#version/commitid

3d5b7f93f591466d50cc973dd41e151b663832df

/ $ cat \#version/date

Tue Dec 15 11:56:04 PST 2015

/ $ cat \#version/host

dlibenzi.mtv.corp.google.com

/ $ cat \#version/kernel_path

/usr/local/google/home/src/akaros/akaros/obj/kern/akaros-kernel-64b

/ $ cat \#version/version

0.1.0

/ $ cat \#version/version_name

Nanwan

[Davide Libenzi]

This branch has been rebased as well, due to new changed in devarch_msr_perf

[Barret Rhoden]

On 2015-12-15 at 11:14 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> Question about that big line.
> It is exceeding 80 chars, but if I break it with \, a space is added
> in any case to the resulting string.
> Any idea?

Let it go over 80 chars. If there's no nice way to avoid it, then we
can just go over. They do the same in Linux. One common example is
breaking up quoted strings in printk. If you break the "quoted string"
in printk, someone might not be able to grep for it.

[Davide Libenzi]

OK.

The ARCH export change has been done as well, in this branch.

[Barret Rhoden]

Hi -

On 2015-12-14 at 20:28 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> The new machinery create an info file like:
>
> KernelPath:
> /usr/local/google/home/src/akaros/akaros/obj/kern/akaros-kernel-64b
> Date: Mon Dec 14 20:21:26 PST 2015
> Host: dlibenzi.mtv.corp.google.com
> CommitID: 769cc0c093f9c9b9f41c6387d0272e317e0e62c3
>
> And stick it into the kernel ELF file.
> The new device reads the information out into separate files (path,
> date, host, commit).
> This branch is based upon the devarch_msr_perf one.
>
>
> https://github.com/dlibenzi/akaros/compare/devarch_msr_perf...dlibenzi:build_info

I can take this as is, minus these two commits:

c36986439eb4 ("Added memstr() API to allow to search strings in memory")
bf1c5c99e797 ("Made readstr and consequently readmem, to use
copy-to-user API")

The first isn't needed, as discussed in another email. The latter
isn't either, and I'd like for us to think that through more (and it's
not a priority).

There is also a minor issue with this. The commitid does not get
updated unless you do a make clean.

On Akaros:
/ $ cat \#version/commitid
345158006a1035496b721fb3946cc3b08839acac

On Linux:
$ git log -1
commit 2823074e41a98f840755bd7f396e9ed44b6df695

345158 was an old commit. I made a new commit and ran make.

The fix is probably to have the makefile detect a change in the commit
(possibly via a hidden file, e.g. obj/kern/.commitid) and remove the
old buildinfo.c file, triggering a rebuild.

It's not a big deal for now though. If you want to fix that up now,
let me know. o/w I can just merge what we've got, and we can fix it
later.

Barret

[Davide Libenzi]

OK dropping memstr(), but sigscan() is not it.

As for readstr(), currently is using memove() to write user memory. Given that we do have now safe copy to/from memory I don't see why that should be left as is. Especially when it's a 6 lines CL.

About the build info, yes, it is intetionally generate after a clean.

Not sure what to do there, w/out force-triggering a new compile+link every time.

If that's OK, I can do that (maybe in a future CL).

[Davide Libenzi]

What is that you had in mind for readstr/readmem? To have it error() if -EFAULT?

If that's the case, and assuming all the call sites expects for them to throw, we could add memcpy_{to,from}_user_error(), sibling of the _errno() ones.

I am not sure all the call sites are safe under throw though.

[Barret Rhoden]

On 2015-12-17 at 21:23 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> What is that you had in mind for readstr/readmem? To have it error()
> if -EFAULT?

More like the kernel trap handler would throw the error() on any PF on
a user address. There's no -EFAULT at all.

> If that's the case, and assuming all the call sites expects for them
> to throw, we could add memcpy_{to,from}_user_error(), sibling of the
> _errno() ones.

The point of the error() approach for PFs was that the kernel code does
not need any helper at all. memcpy works (including a memcpy that is
optimized for speed). CAS and atomics work. Regular dereferences work.
etc.

> I am not sure all the call sites are safe under throw though.

Me neither. That's why this is a bigger issue than playing
whack-a-mole with every function that might touch user memory. When
the time comes that we need to fix it (which I don't think is now), I'd
like to fix it completely. But for now, I'd like to just get an
application running.

Barret

[Barret Rhoden]

On 2015-12-17 at 15:00 "'Davide Libenzi' via Akaros"

<aka...@googlegroups.com> wrote:
> About the build info, yes, it is intetionally generate after a clean.
> Not sure what to do there, w/out force-triggering a new compile+link
> every time.
> If that's OK, I can do that (maybe in a future CL).

Sounds good; we can deal with this in the future.

I think there's a way to rebuild the buildinfo when we change commits
and also to not require a new total recompilation every time there's a
new commit. We can probably have a file that tracks the commit
version. If the git rev-parse != the contents of the file, we recreate
the file. Then buildinfo depends on that file.


Merged to master at 080cf1ff0e75..345158006a10 (from, to]

You can see the entire diff with 'git diff' or at
https://github.com/brho/akaros/compare/080cf1ff0e75...345158006a10


Barret