From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH 1/4] kvm tools: Add ioeventfd support Date: Fri, 27 May 2011 14:02:56 +0300 Message-ID: <1306494176.3217.16.camel@lappy> References: <1306492621-10208-1-git-send-email-levinsasha928@gmail.com> <20110527105450.GA25909@elte.hu> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: penberg@kernel.org, john@jfloren.net, kvm@vger.kernel.org, asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com To: Ingo Molnar Return-path: Received: from mail-wy0-f174.google.com ([74.125.82.174]:33841 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750839Ab1E0LDQ (ORCPT ); Fri, 27 May 2011 07:03:16 -0400 Received: by wya21 with SMTP id 21so1155282wya.19 for ; Fri, 27 May 2011 04:03:15 -0700 (PDT) In-Reply-To: <20110527105450.GA25909@elte.hu> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, 2011-05-27 at 12:54 +0200, Ingo Molnar wrote: > * Sasha Levin wrote: > > > ioeventfd is way provided by KVM to receive notifications about > > reads and writes to PIO and MMIO areas within the guest. > > > > Such notifications are usefull if all we need to know is that > > a specific area of the memory has been changed, and we don't need > > a heavyweight exit to happen. > > > > The implementation uses epoll to scale to large number of ioeventfds. > > Nice! :-) > > > +struct ioevent { > > + u64 start; > > + u8 len; > > If that's an mmio address then it might be worth naming it > ioevent->mmio_addr, ioevent->mmio_end. > > > + void (*ioevent_callback_fn)(struct kvm *kvm, void *ptr); > > Please only 'fn', we already know this is an ioevent. > > > + struct kvm *kvm; > > + void *ptr; > > what is the purpose of the pointer? > > AFAICS it the private data of the callback function. In such cases > please name them in a harmonizing fashion, such as: > > void (*fn)(struct kvm *kvm, void *data); > struct kvm *fn_kvm; > void *fn_data; > > Also, will tools/kvm/ ever run with multiple 'struct kvm' instances > present? I'm not sure. We pass it around to all our functions instead of using a global, so I assumed we might have several guests under one process. > A sidenote: i think 'struct kvm *kvm' was a naming mistake - it's way > too aspecific, it tells us nothing. What is a 'kvm'? > > A much better name would be 'struct machine *machine', hm? Even if > everyone agrees this would be a separate patch, obviously. > > Also, can ioevent->kvm *ever* be different from the kvm that the > mmio-event receiving vcpu thread is associated with? If not then the > fn_kvm field is really superfluous - we get the machine from the mmio > handler and can pass it down to the callback function. > > > + int event_fd; > > 'fd' > > > + u64 datamatch; > > what's a datamatch? 'cookie'? 'key'? The kernel-side ioeventfd matches the value written to the PIO port and signals the event only if both values match. It's named this way in the kernel code so I wanted to be consistent. > > > + > > + struct list_head list_used; > > just 'list' is enough i think - it's obvious that ioevent->list is a > list of ioevents, right? > We might have a list of free ioevents if we ever decide to scale it beyond the max 20 event limit we currently have, so I would rather be specific with the list names. > > + kvm_ioevent = (struct kvm_ioeventfd) { > > + .addr = ioevent->start, > > + .len = ioevent->len, > > Do you see how confusing the start/len naming is? Here you are > assigning a 'start' field to an 'addr' and the reviewer is kept > wondering whether that's right. If it was ->mmio_addr then it would > be a lot more obvious what is going on. Yes, I'll rename them to addr/len to match with KVM naming. > > +static void *ioeventfd__thread(void *param) > > +{ > > + for (;;) { > > + int nfds, i; > > + > > + nfds = epoll_wait(epoll_fd, events, IOEVENTFD_MAX_EVENTS, -1); > > + for (i = 0; i < nfds; i++) { > > + u64 tmp; > > + struct ioevent *ioevent; > > + > > + ioevent = events[i].data.ptr; > > + > > + if (read(ioevent->event_fd, &tmp, sizeof(tmp)) < 0) > > + die("Failed reading event"); > > + > > + ioevent->ioevent_callback_fn(ioevent->kvm, ioevent->ptr); > > + } > > + } > > + > > + return NULL; > > +} > > + > > +void ioeventfd__start(void) > > +{ > > + pthread_t thread; > > + > > + pthread_create(&thread, NULL, ioeventfd__thread, NULL); > > +} > > Shouldnt this use the thread pool, so that we know about each and > every worker thread we have started, in one central place? > Our thread pool currently responds to events - it runs a callback if it has received a notification to do so. It doesn't manage threads which have to run all the time like in this case. Though once we return from epoll_wait() here we do minimal work before sending the IO event into the thread pool. > (This might have relevance, see the big-reader-lock mail i sent > earlier today.) > > Thanks, > > Ingo -- Sasha.