Prev: ehea: error handling improvement
Next: p9auth: add CAP_GRANT_ID to authorize use of /dev/caphash
From: Thomas Gleixner on 28 Apr 2010 12:50
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.

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 ?

> +

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;

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

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

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter P Waskiewicz Jr on 30 Apr 2010 13:30
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter P Waskiewicz Jr on 30 Apr 2010 13:50
On Thu, 2010-04-29 at 13:39 -0700, Thomas Gleixner wrote:
> On Thu, 29 Apr 2010, Peter P Waskiewicz Jr wrote:
> > On Thu, 2010-04-29 at 12:48 -0700, Thomas Gleixner wrote:
> > > Thinking more about it, I wonder whether you have a cpu_mask in your
> > > driver/device private data anyway. I bet you have :)
> >
> > Well, at this point we don't, but nothing says we can't.
>
> Somewhere you need to store that information in your driver, right ?

Yes. But right now, storing a cpu_mask for an interrupt wouldn't buy us
anything since we have no mechanism to make use of it today. :-)

I'll be putting the cpu_mask entry in our q_vector structure, which is
our abstraction of the MSI-X vector (it's where I have the hint struct
right now in patch 2/2 for the ixgbe driver). It's a simple place to
stick it.

> > > So it should be sufficient to set a pointer to that cpu_mask in
> > > irq_desc and get rid of the callback completely.
> >
> > So "register" would just assign the pointer, and "unregister" would make
> > sure to NULL the mask pointer out. I like it. It'll sure clean things
> > up too.
>
> Yep, that'd be like the set_irq_chip() function. Just assign the
> pointer under desc->lock.
>
> Thanks,
>
> tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Peter P Waskiewicz Jr on 30 Apr 2010 14:30
On Thu, 2010-04-29 at 12:48 -0700, Thomas Gleixner wrote:
> B1;2005;0cPeter,
>
> On Thu, 29 Apr 2010, Peter P Waskiewicz Jr wrote:
> > On Wed, 2010-04-28 at 09:45 -0700, Thomas Gleixner wrote:
> > > 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.
>
> Thinking more about it, I wonder whether you have a cpu_mask in your
> driver/device private data anyway. I bet you have :)

Well, at this point we don't, but nothing says we can't.

> So it should be sufficient to set a pointer to that cpu_mask in
> irq_desc and get rid of the callback completely.

So "register" would just assign the pointer, and "unregister" would make
sure to NULL the mask pointer out. I like it. It'll sure clean things
up too.

> Any access to desc->affinity_hint needs to be protected by desc->lock.
> For setting the pointer to a real mask resp. NULL that's fine. The
> copy which you need to do in the proc-read function is not going to
> introduce huge latencies either.

Right.

> Thanks,
>
> tglx

Thanks for the additional inputs. Patches coming soon.

-PJ

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Thomas Gleixner on 30 Apr 2010 15:00
On Thu, 29 Apr 2010, Peter P Waskiewicz Jr wrote:
> On Thu, 2010-04-29 at 12:48 -0700, Thomas Gleixner wrote:
> > Thinking more about it, I wonder whether you have a cpu_mask in your
> > driver/device private data anyway. I bet you have :)
>
> Well, at this point we don't, but nothing says we can't.

Somewhere you need to store that information in your driver, right ?

> > So it should be sufficient to set a pointer to that cpu_mask in
> > irq_desc and get rid of the callback completely.
>
> So "register" would just assign the pointer, and "unregister" would make
> sure to NULL the mask pointer out. I like it. It'll sure clean things
> up too.

Yep, that'd be like the set_irq_chip() function. Just assign the
pointer under desc->lock.

Thanks,

tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  | 
Pages: 1 2
Prev: ehea: error handling improvement
Next: p9auth: add CAP_GRANT_ID to authorize use of /dev/caphash