Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

Avi Kivity
2010-11-28T13:14:21Z

On 11/28/2010 01:44 PM, Michael S. Tsirkin wrote:
> On Sun, Nov 28, 2010 at 11:54:26AM +0200, Avi Kivity wrote:
> > On 11/28/2010 11:50 AM, Michael S. Tsirkin wrote:
> > >> >
> > >> >Another problem is that there seem to be two memory allocations and a
> > >> >copy here, apparently just to simplify error handling. It might be fine
> > >> >for this test but won't scale for when performance matters.
> > >>
> > >> When it matters, we can fix it. I don't see msr read/write becoming
> > >> a hot path.
> > >
> > >It will be very painful to fix it.
> >
> > Why?
>
> Because the API returns a vector.

Returning an object does not involve a copy (return value optimization).

> > >>
> > >> The compiler should optimize it away completely.
> > >
> > >Should as opposed to does. Want me to try a simple test?
> >
> > Please.
>
> Just for fun: optimize for size, and compare code sizes.
> The C++ code is yours but I have removed all use of STL to make
> it more of an even fight. I checked by object and executable size.
> Note that this is shared library build: a C++ executable
> will pull in a large C++ library, a C executable won't.
> If you are interested in an STL based example let me know.
> You can take it from here and make it more real if you like,
> or look at the assembler output.
>
> ------------------------------
> [mst@tuck ~]$ cat test.c
> #include<sys/ioctl.h>
> #include<sys/types.h>
> #include<sys/stat.h>
> #include<fcntl.h>
> #include<unistd.h>
> #include<errno.h>
>
> int main(int argc, char **argv)
> {
> int fd = open("/dev/kvm", O_RDWR);
> int r;
> if (fd< 0)
> goto open_err;
> r = ioctl(fd, 0, 0);
> if (r< 0)
> goto ioctl_err;
> return 0;
> ioctl_err:
> close(fd);
> open_err:
> return -1;
> }

This code is not reusable. Everywhere you use an fd, you have to repeat
this code.

> [mst@tuck ~]$ gcc -c -Os test.c
> [mst@tuck ~]$ size test.o
> text data bss dec hex filename
> 97 0 0 97 61 test.o
> [mst@tuck ~]$ gcc -Os test.c
> [mst@tuck ~]$ size a.out
> text data bss dec hex filename
> 1192 260 8 1460 5b4 a.out
> [mst@tuck ~]$ wc -l test.c
> 22 test.c
> ------------------------------
> [mst@tuck ~]$ cat kvmxx.cpp
> extern "C" {
> #include<sys/ioctl.h>
> #include<sys/types.h>
> #include<sys/stat.h>
> #include<fcntl.h>
> #include<unistd.h>
> #include<errno.h>
> }
>
> namespace kvm {
>
> class fd {
> public:
> explicit fd(const char *path, int flags);
> ~fd() { ::close(_fd); }
> long ioctl(unsigned nr, long arg);
> private:
> int _fd;
> };
>
> };
>
> namespace kvm {
>
> static long check_error(long r)
> {
> if (r == -1) {
> throw errno;
> }
> return r;
> }
>
> fd::fd(const char *device_node, int flags)
> : _fd(::open(device_node, flags))
> {
> check_error(_fd);
> }
>
>
> long fd::ioctl(unsigned nr, long arg)
> {
> return check_error(::ioctl(_fd, nr, arg));
> }
>
> }
>
> int main(int ac, char **av)
> {
> try {
> kvm::fd fd("/dev/kvm", O_RDWR);
> fd.ioctl(0, 0);
> } catch (...) {
> return -1;
> }
> return 0;
> }

class kvm::fd is reusable, if you embed it in another object you don't
have to worry about errors any more (as long as the object's methods are
exception safe).

> [mst@tuck ~]$ g++ -c -Os kvmxx.cpp
> [mst@tuck ~]$ size kvmxx.o
> text data bss dec hex filename
> 529 0 0 529 211 kvmxx.o
> [mst@tuck ~]$ g++ -Os kvmxx.cpp
> [mst@tuck ~]$ size a.out
> text data bss dec hex filename
> 2254 308 16 2578 a12 a.out
> [mst@tuck ~]$ wc kvmxx.cpp
> 56 kvmxx.cpp
> ------------------------------
>
>
> One interesting thing is that the object size grew
> faster than linked executable size.
> This might mean that the compiler generated some
> dead code that the linker then threw out.
> It's also interesting that C++ managed to use up
> more data/bss storage.

C++ will have much larger data and code sizes because it uses DWARF
tables for unwinding and generates stack unwinding code. These are all
out of the hot path.

> > >> There's been a lot
> > >> of work in gcc on that.
> > >>
> > >> About compile times, I don't care much.
> > >
> > >I do. You will too when we have codebase that can be built as fast as
> > >we commit things, so buildbot breaks.
> > >This is common in C++ based projects.
> >
> > If kvm-unit-tests.git takes to long to compile, I'll be very happy.
>
> If the claim is "it's so small it does not matter what it's written in"
> then I guess don't mind. But then - why do we care about
> error handling so much? For the test, it's probably ok to just assert
> after each ioctl/malloc and be done with it.

Yes, all the correctness is more or less pointless here. Like I said,
this is an experiment to see what things look like. I guess each side
will it as proving its claims.

--
error compiling committee.c: too many arguments to function