From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH 1/4] kvm tools: Add ioeventfd support
Date: Fri, 27 May 2011 12:54:50 +0200
Message-ID: <20110527105450.GA25909@elte.hu>
References: <1306492621-10208-1-git-send-email-levinsasha928@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: penberg@kernel.org, john@jfloren.net, kvm@vger.kernel.org,
	asias.hejun@gmail.com, gorcunov@gmail.com, prasadjoshi124@gmail.com
To: Sasha Levin <levinsasha928@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx3.mail.elte.hu ([157.181.1.138]:57778 "EHLO mx3.mail.elte.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750839Ab1E0Kyy (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 27 May 2011 06:54:54 -0400
Content-Disposition: inline
In-Reply-To: <1306492621-10208-1-git-send-email-levinsasha928@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Sasha Levin <levinsasha928@gmail.com> 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?

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'?

> +
> +	struct list_head	list_used;

just 'list' is enough i think - it's obvious that ioevent->list is a 
list of ioevents, right?

> +	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.
> +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?

(This might have relevance, see the big-reader-lock mail i sent 
earlier today.)

Thanks,

	Ingo