From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs Date: Sun, 28 Nov 2010 15:14:17 +0200 Message-ID: <4CF255A9.6030502@redhat.com> References: <1290595933-13122-1-git-send-email-avi@redhat.com> <1290595933-13122-3-git-send-email-avi@redhat.com> <20101126101625.GA3657@redhat.com> <4CF0CB9A.5060403@redhat.com> <20101128085833.GA3330@redhat.com> <4CF2215D.9060201@redhat.com> <20101128095056.GA4499@redhat.com> <4CF226D2.6030102@redhat.com> <20101128114424.GB4499@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:36338 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752693Ab0K1NOV (ORCPT ); Sun, 28 Nov 2010 08:14:21 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oASDELUT021123 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 28 Nov 2010 08:14:21 -0500 In-Reply-To: <20101128114424.GB4499@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > #include > #include > #include > #include > #include > > 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 > #include > #include > #include > #include > #include > } > > 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