Re: [Xen-devel] [RFC PATCH] Adding support for coverage informations

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-01-29 at 12:02 +0000, Frediano Ziglio wrote:
> On Tue, 2013-01-29 at 10:56 +0000, Ian Campbell wrote:
> > On Mon, 2013-01-28 at 21:16 +0000, Frediano Ziglio wrote:
> > > From: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> > > 
> > > This patch introduce coverage support to Xen.
> > > Currently it allows to compile Xen with coverage support but there is no 
> > > way
> > > to extract them.
> > > 
> > > The declarations came from Linux source files (as you can see from file
> > > headers).
> > > 
> > > It also add a new hypercall (can somebody reserve a number for this 
> > > stuff?).
> > 
> > You can simply patch xen/include/public/xen.h to declare the new
> > __HYPERVISOR_foo_op. (which I now see you have done in this patch!). If
> > you just want to reserve the number it is also allowed to send just that
> > hunk to reserve a number pending the implementation.
> > 
> > BTW I'd suggest leaving the stub implementation out of the patch until
> > you've decided what it will look like, so it returns ENOSYS instead of
> > EINVAL (or change your stub to return ENOSYS).
> > 
> 
> Easy to change. Actually I discovered that my patch does not even
> compile if you disable TEST_COVERAGE (and it should be disabled by
> default too). EINVAL say that the hypercall is present but the value
> passed in invalid.
> 
> The reason why adding a new hypercall instead of a new sysctl is simply
> because is easier to have a zero cost if you disable coverage
> informations. The best thing would be redirect do_coverage_op to
> do_ni_hypercall using linker options but even two small stub would do
> (these stubs will return ENOSYS instead).

I don't think you need to worry about performance of the NULL
do_coverage_op when coverage is disabled, that's not a hot path..

> 
> > > The idea is to have some operations mainly
> > > - get filename
> > 
> > This is get a list of filenames (i.e. input for the next two)?
> > 
> 
> The idea was just to pass the index and get a filename or a ENOENT error
> (or a ENOBUF if buffer is too small). Actually you should scan the
> entire list to get a proper filename so this end up in a O(N^2)
> operation but as N is constant and not that big it should not be a
> problem. If it's a problem I can implement a tree or simply reuse
> constructor space to store a simple array of gcov_info*.

What access pattern do you expect? Do you expect most users to just want
to get all the files (i.e. one by one, cycling through the indexes) or
would they be interested in a subset of the files?

I'd expect most users to scan the indexes at start of day to get the
list of filenames and then cache it and use it for the remainder of the
run.

> > > - get count informations for a file
> > > - reset counter for a specific file
> > 
> > How atomic do these need to be? Will it be necessary to obtain
> > consistent snapshots of the counters for multiple files? Even for a
> > single file I assume there will be many counters so getting a consistent
> > snapshot at the file level might even be tricky?
> > 
> > Or perhaps since you are measuring coverage you only care about one bit
> > per basic block (rather than a counter as such) and if you race then, oh
> > well, you'll see it next time.
> > 
> 
> They cannot be so atomic anyway as the hypervisor is running, I'd not
> use a lock or similar. These informations are mainly statistical one.
> It's not possible to disable incrementing any counter and also the
> increment is done without locking (so it's possible to loose some
> number). Unless you pause all machines and stop any userspace program

Yes, that's what I was wondering -- if it would be necessary to quiesce
the system (which probably means holding all of the pCPUS in some known
good state briefly, similar to Linux's stop_machine() stuff) while
reading the counters. Fortunately it sounds like this won't be
necessary.

> that is doing some activities with Xen you'll have to consider some
> dirty counter. Perhaps would be better to have an operation to
> query/reset more filenames at a time.

Read and reset for a single file might be sufficient? Really all this
depends on what the userspace side needs -- which is something I think
only you can answer.

>  But even in Linux you have to
> "cat" different files to get informations.

OK.

> > > - reset all counters
> > > 
> > > The op parameter in the hypercall will be the operation.
> > > The idx parameter is the index to the file.
> > > The arg1 is the argument (a buffer pointer for filename or counters).
> > > The arg2 probably will be the size of previous buffer.
> > 
> > Often the best way to deal with arg1/arg2 would be to pass a union of
> > the types specific to each operation.
> > 
> 
> In this case arg1 if the buffer while arg2 would be mainly the length of
> arg1 so they cannot be an union.
> 
> > But how does the caller determine the size of the buffer? I guess it
> > various from file to file? Is this what comes from "get count
> > informations for a file"?
> > 
> 
> Well.. pass a large enough buffer and handle ENOBUF correctly.

I think it would be much better if you provided a way to query the
required buffer size (for each file if necessary). It would make sense
to me to do this in the same interface as the one which returns the
filename.

> > > Do you think I should pack these parameters and pass a single pointer?
> > > 
> > > Linux use a file system to export these information. I would use the same
> > > format as Linux use so to make it easy to reuse tools from Linux (like 
> > > lcov).
> > > That the reason why I split get filename from get counter informations.
> > 
> > You could also synthesize this at the tools layer. I don't know enough
> > of how this stuff works to say what makes sense at the h/v layer.
> > 
> > What form does this information take internally?
> > 
> 
> They are binary files. The format is defined by gcc. On userspace you
> get a single .gcda file for each compile unit. This file is updated when
> any program exit (multiple execution update the files). Gcc generate
> also .gcno files. You need both .gcda and .gcno at the end to handle
> coverage information.

I meant what form does the data take internally to Xen.

My point was that since you need a Xen specific userspace tool to
extract the information from the hypervisor *it* could be the thing
which provides the same format as Linux uses, which need not necessarily
imply that the representation inside the hypervisor or at the hypercall
API looks the same. It might be easier for the hypervisor to provide a
relatively unfiltered view on the internal data and have the tools fix
it up into something which looks like the same format as Linux.

> > > Do anybody know why I had to exclude %.init.o files to make it compile?
> > > Are these files used before Xen start?
> > 
> > They are used during start of day bring up and then discarded once Xen
> > is up and running. I'm not sure what that would affect the link,
> > although it would obviously be disastrous if you were to touch the
> > counters associated with those files again after they were thrown away.
> > 
> 
> The problem would be reading (or resetting) these counter if discarded,
> probably would lead to some buffer overwriting which is not good.
> Actually these modules have no such informations so it's not a problem.

If you cared about boot time coverage then I suppose you could save off
the counters just before the point at which Xen frees the init sections
-- after that point they certainly won't be changing.

But I think skipping them until someone wants to profile that bit is OK
too.

> > We might have some checks in the tree to ensure that non-init code
> > cannot reference init code by mistake -- maybe that's what broke the
> > build for you?
> > 
> 
> Yes, I think it's safer to disable for them (as it actually).
> 
> Are TEST_COVERAGE a good names or would be better to have something like
> CONFIG_ENABLE_COVERAGE (do you use any prefix for configure stuff?) ?

I'd model it on the perfc stuff in xen/Rules.mk or the debug stuff in
Config.mk I think, so you would do "make dist-xen coverage=y" and this
would turn into -DCOVERAGE or something. The CONFIG_* stuff tends to
come from xen/include/asm-*/config.h (TBH I'm not sure it's terribly
consistent, or maybe there's a rule/pattern I don't know about ...)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel