Re: [PATCH RFC: linux-next 1/2] irq: Add CPU mask affinity hint callback framework

[Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>]

On Wed, 2010-04-28 at 09:45 -0700, Thomas Gleixner wrote:
> B1;2005;0cPeter,
> 
> On Tue, 27 Apr 2010, Peter P Waskiewicz Jr wrote:
> > On Tue, 27 Apr 2010, Thomas Gleixner wrote:
> > > On Sun, 18 Apr 2010, Peter P Waskiewicz Jr wrote:
> > > > +/**
> > > > + * struct irqaffinityhint - per interrupt affinity helper
> > > > + * @callback:	device driver callback function
> > > > + * @dev:	reference for the affected device
> > > > + * @irq:	interrupt number
> > > > + */
> > > > +struct irqaffinityhint {
> > > > +	irq_affinity_hint_t callback;
> > > > +	void *dev;
> > > > +	int irq;
> > > > +};
> > > 
> > > Why do you need that extra data structure ? The device and the irq
> > > number are known, so all you need is the callback itself. So no need
> > > for allocating memory ....
> > 
> > When I register the function callback with the interrupt layer, I need to
> > know what device structures to reference back in the driver.  In other words,
> > if I call into an underlying driver with just an interrupt number, then I
> > have no way at getting at the dev structures (netdevice for me, plus my
> > private adapter structures), unless I declare them globally (yuck).
> 
> Grr, I knew that I missed something. That'll teach me to review
> patches before the coffee has reached my brain cells :)
> 
> > I had a different approach before this one where I assumed the device from
> > the irq handler callback was safe to use for the device in this new callback.
> > I didn't feel really great about that, since it's an implicit assumption that
> > could cause things to go sideways really quickly.
> >
> > Let me know what you think either way.  I'm certainly willing to make a
> > change, I just don't know at this point what's the safest approach from what
> > I currently have.
> 
> So you need a reference to your device, so what about the following:
> 
> struct irq_affinity_hint;
> 
> struct irq_affinity_hint {
>        unsigned int (*callback)(unsigned int irq, struct irq_affinity_hint *hint,
> 				cpumask_var_t *mask);
> }
> 
> Now you embed that struct into your device private data structure and
> you get the reference to it back in the callback function. No extra
> kmalloc/kfree, less code.

Good idea!  I'll roll that into my new version.

> One other thing I noticed, but forgot to comment on:
> 
> > +static int irq_affinity_hint_proc_show(struct seq_file *m, void *v)
> > +{
> > +	struct irq_desc *desc = irq_to_desc((long)m->private);
> > +	struct cpumask mask;
> > +	unsigned int ret = 0;
> 
>  Why do we return 0, when there is no callback and no hint available ?

I initialized it to 0 to remove a compiler warning; I can put more
thought into it and assign a more appropriate return value.

> > +
> 
>   We don't want to have cpumask enforced on stack. Please make that:
> 
>      	cpumask_var_t mask;
> 
> 	if (!alloc_cpumask_var(&mask, GFP_KERNEL))
> 	       return -ENOMEM;

I'll roll this into my next version.

> > +	if (desc->hint && desc->hint->callback) {
> 
>   The access to desc-> needs to be protected with
>   desc->lock. Otherwise you might race with a callback unregister.

Good point.  I'll fix this.

> > +		ret = desc->hint->callback(&mask, (long)m->private,
> > +		                           desc->hint->dev);
> > +		if (!ret)
> > +			seq_cpumask(m, &mask);
> > +	}
> > +
> > +	seq_putc(m, '\n');
> > +	return ret;
> > +}
> 
> Thanks,
> 

Thanks for the feedback.  I'll have the updated patches for review soon.

-PJ