Re: [PATCH] Add irqdevice interface + userint implementation

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Sun, Apr 8, 2007 at  5:58 AM, in message
<20070408095853.GA31284-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
wrote: 
>>  diff -- git a/drivers/kvm/kvm_irqdevice.h b/drivers/kvm/kvm_irqdevice.h
>> new file mode 100644
>> index 0000000..9c91d15
>> ---  /dev/null
>> +++ b/drivers/kvm/kvm_irqdevice.h
>> @@ - 0,0 +1,206 @@
>> +/*
>> + * kvm_irqdevice.h
> 
> No need to mention the file name in the top of file comment.  Quite 
> contrary,
> this comment can easily get out of sync and thus is not encouraged.

Ack

> 
>> + *
>> + * Defines an interface for an abstract interrupt controller.  The model 
>> + * consists of a unit with an arbitrary number of input lines (IRQ0- N), an
>> + * output line (INTR), and methods for completing an interrupt- acknowledge
>> + * cycle (INTA).  A particular implementation of this model will define
>> + * various policies, such as irq- to- vector translation, INTA/auto- EOI policy,
>> + * etc.  
>> + * 
>> + * In addition, the INTR callback mechanism allows the unit to be "wired" 
> to
>> + * an interruptible source in a very flexible manner. For instance, an 
>> + * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another 
>> + * interrupt controller (ala cascaded i8259s)
>> + *
>> + * Copyright (C) 2007 Novell
>> + *
>> + * Authors:
>> + *   Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along 
> with
>> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
>> + * Place -  Suite 330, Boston, MA 02111- 1307 USA.
> 
> It would be nice if you could follow the top of file comment style used
> in the rest of the kvm code instead of using a very different one.

I actually copied this header from one of the KVM files, but I will take a deeper look to see if I can figure out what you are talking about.  Perhaps you are referring to how I described the file, rather than the other details like the GPL section, etc.

> 
>> +/*
>> + * kvm_irq_init()
>> + *
>> + * Description:
>> + *    Initialized the kvm_irqdevice for use.  Should be called before 
> calling
>> + *    any derived implementation init functions
>> + * 
>> + * Parameters:
>> + *    struct kvm_irqdevice *dev: This device
>> + *
>> + * Returns: (void)
>> + */
> 
> Please use kernel- doc style comments that can be used to auto- generate
> documentation.

I can do this if its the right thing to do...but isnt that for general kernel API?  This stuff is really just private to KVM.  Please advise. 

> 
>> +static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
>> +{
>> +	return dev- >pending(dev, flags);
>> +}

I respectfully disagree with these next set of objections related to the wrappers.  I will try to detail the reasons why

> 
> This wrapper is not needed at all, just call the method directly.

I would agree with you if were were talking about disparate pointers: e.g. kvm_arch_ops for function context and kvm_vcpu for data context.  However, in this case we have the same pointer for both (e.g. we are emulating the "implicit this" from C++).  The two *always* being the same is an invariant that this wrapper helps to enforce.  Lack of this wrapper is just asking for a bug, IMHO.

> 
>> +static inline int kvm_irq_read_vector(struct kvm_irqdevice *dev, int flags)
>> +{
>> +	return dev- >read_vector(dev, flags);
>> +}
> 
> ditto
> 
>> +static inline int kvm_irq_set_pin(struct kvm_irqdevice *dev, int pin, 
>> +				  int level, int flags)
>> +{
>> +	return dev- >set_pin(dev, pin, level, flags);
>> +}
> 
> ditto
> 
>> +static inline int kvm_irq_summary(struct kvm_irqdevice *dev, void *data)
>> +{
>> +	return dev- >summary(dev, data);
>> +}
> 
> ditto.
> 
>> +static inline void kvm_irq_register_sink(struct kvm_irqdevice *dev, 
>> +					 const struct kvm_irqsink *sink)
>> +{
>> +	dev- >sink = *sink;
>> +}
> 
> Completely useless wrapper, just do the assignment in the caller.

This has to do with encapsulation.  The fact that the data is stored in dev->sink is an implementation detail that I prefer to see hidden by the interface.  For instance, what if someone changes their mind about the current policy of a singleton callback (e.g. allow multiple callback registrations)?  Or what if someone wants to add error checking to make sure a callback is registered only once? 

> Also the convention of copying the operation vectors is rather unnatural
> for linux, just set the pointer (and make sure the operations regustired
> are file- scope not local- scope as in your current code)

Ack.

> 
>> +static inline void kvm_irq_raise_intr(struct kvm_irqdevice *dev)
>> +{
>> +	struct kvm_irqsink *sink = &dev- >sink;
>> +	if (sink- >raise_intr)
>> +		sink- >raise_intr(sink, dev);
>> +}
> 
> Similarly this is rather pointless and could be opencoded in the only 
> caller.

Keep in mind that this one patch from a series.  There are other consumers downstream in my queue that will use this as well.  Writing this code to check if there is a callback over and over is a pain and unnecessary.  I am trying to model a pub/sub interface where the publisher need not care about the presence of subscribers.  In addition, this touches upon my first objection where the invariant enforcement of sink->raise_int(sink, ...) is desirable.  

> 
>> +/*----------------------------------------------------------------------
>> + * optimized bitarray object -  works like bitarrays in bitops, but uses 
>> + * a summary field to accelerate lookups.  Assumes external locking 
>> + *--------------------------------------------------------------------- */
>> +
>> +struct bitarray {
>> +	unsigned long summary; /* 1 per word in pending */
>> +	unsigned long pending[NR_IRQ_WORDS];
>> +};
>> +
>> +static inline int bitarray_pending(struct bitarray *this)
>> +{
>> +	return this- >summary ? 1 : 0;	
>> +}
>> +
>> +static inline int bitarray_findhighest(struct bitarray *this)
>> +{
>> +	if (!this- >summary)
>> +		return - 1;
>> +	else {
>> +	    int word_index = __ffs(this- >summary);
>> +	    int bit_index  = __ffs(this- >pending[word_index]);
>> +
>> +	    return word_index * BITS_PER_LONG + bit_index;	
>> +	}
>> +}
>> +
>> +static inline void bitarray_set(struct bitarray *this, int nr)
>> +{
>> +	__set_bit(nr, &this- >pending);
>> +	__set_bit(nr / BITS_PER_LONG, &this- >summary); 
>> +} 
>> +
>> +static inline void bitarray_clear(struct bitarray *this, int nr)
>> +{
>> +	int word = nr / BITS_PER_LONG;
>> +
>> +	__clear_bit(nr, &this- >pending);
>> +	if (!this- >pending[word])
>> +		__clear_bit(word, &this- >summary);
>> +}
>> +
>> +static inline int bitarray_test(struct bitarray *this, int nr)
>> +{
>> +	return test_bit(nr, &this- >pending);
>> +}
> 
> This should go into a separate header, probably even in include/linux/

Avi has already addressed this

> 
>> +/*----------------------------------------------------------------------
>> + * userint interface -  provides the actual kvm_irqdevice implementation
>> + *--------------------------------------------------------------------- */
>> +
>> +typedef struct {
>> +	spinlock_t      lock;
>> +	struct bitarray irq_pending;
>> +	int             nmi_pending;
>> +}kvm_userint;
> 
> Please just use a struct type instead of the typedef.

Ack

> 
>> +int kvm_userint_init(struct kvm_irqdevice *dev)
>> +{
>> +	kvm_userint *s;
>> +
>> +	s = (kvm_userint *)kzalloc(sizeof(kvm_userint), GFP_KERNEL);
> 
> No need to case the kzalloc return value.  But you need to check
> the return value for NULL and handle the error.

Ack

Thanks for the review.  Greatly appreciated.

Regards,
-Greg



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV