Message-ID: <4BFC800D.2090309@tilera.com>
Date:	Tue, 25 May 2010 21:57:33 -0400
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] arch/tile: new multi-core architecture for Linux

Thomas, thanks for your feedback.  If I don't comment on something you
said it's because you were obviously right and I applied a suitable fix. :-)

On 5/25/2010 4:12 PM, Thomas Gleixner wrote:
> +/**
> + * tile_request_irq() - Allocate an interrupt handling instance.
> [...]
>
>   Why are you implementing your private interrupt handling
>   infrastructure ? What's wrong with the generic interrupt handling
>   code ? Why is each device driver forced to call tile_request_irq()
>   which makes it incompatible to the rest of the kernel and therefor
>   unshareable ?
>   

Our interrupt management code has evolved as we have developed this
code, so I won't present arguments as to why it's perfect the way it is,
but just why it IS the way it is.  :-)

The tile irq.c does not replace the generic Linux IRQ management code,
but instead provides a very limited set of virtual interrupts that are
only used by our para-virtualized device drivers, and delivered to Linux
via a single hypervisor downcall that atomically sets "virtual
interrupt" bits in a bitmask.  The PCI root complex driver reserves four
of these bits (i.e. irqs) to map real PCI interrupts; they are then fed
forward into the regular Linux IRQ system to manage all "standard"
devices.  The other tile-specific para-virtualized drivers that use this
interface are the PCI endpoint code, xgbe network driver, ATA-over-GPIO
driver, and the IPI layer.  None of these para-virtualized drivers are
actually shareable with other Linux architectures in any case.

We have an outstanding enhancement request in our bug tracking system to
switch to using the Linux generic IRQs directly, and plan to implement
it prior to our next major release.  But we haven't done it yet, and
this code, though somewhat crufty, is reasonably stable.  I'm also not
the primary maintainer of this particular piece of code, so I'd rather
wait until that person frees up and have him do it, instead of trying to
hack it myself.

In any case, I'll add commentary material (probably just an edited
version of the explanatory paragraph above) into irq.c so at least it's
clear what's going on.

> +void tile_free_irq(int index)
> +[...]
>
>   That code lacks any kind of protection and serialization.
>   

Interesting point.  As it happens, these calls are all made during boot,
so they are serialized that way.  But in principle we could use the xgbe
driver as a module, at least, so you're right; I'll add a spinlock.

> [...]
>
>   You check desc->handler, but you happily call the handler while
>   dev_id might be still NULL. See above.
>   

Assuming we spinlock the irq request/free routines, I think this is
safe, since the unlock memory fence will guarantee visibility of the
fields prior to any attempt to use them.  We always allocate the
interrupt, then tell the hypervisor to start delivering them; on device
unload we tell the hypervisor to stop delivering interrupts, then free
it.  The "tell the hypervisor" steps use on_each_cpu() and wait, so are
fully synchronous.

> +/*
> + * Generic, controller-independent functions:
> + */
> +
> +int show_interrupts(struct seq_file *p, void *v)
> +[...]
>
>   So that prints which interrupts ? Now you refer to the generic code,
>   while above you require that tile specific one. -ENOSENSE.
>   

Yes, this is confusing.  This routine is required by procfs, and it
shows just the PCI interrupts, not the tile irqs.  I'll add a comment,
and try to segregate the file into "generic irqs" and "tile irqs" more
obviously, for now.  The routine itself will be more sensible once we
integrate our tile_irqs into the generic system.

> +/* How many cycles per second we are running at. */
> +static cycles_t cycles_per_sec __write_once;
> +static u32 cyc2ns_mult __write_once;
> +#define cyc2ns_shift 30
>
>   Please do not use fixed shift values. Use the generic functions to
>   calculate the optimal shift/mult pairs instead. 
>   

Thanks; I wasn't aware of these.  I'll switch the code over to use them,
and the other helper functions you pointed out.

> +#if CHIP_HAS_SPLIT_CYCLE()
>
>   That should be a CONFIG_TILE_HAS_SPLIT_CYCLE and not a function like
>   macro define somewhere in a header file.
>   

This is not a configurable option.  The <arch/chip.h> header (which is
not a Linux header per-se, but one of our "core architecture" headers
that can be used in any programming context) provides a set of
CHIP_xxx() macros.  We use a functional macro style because we saw too
many instances of "#ifdef CHIP_FOO_MISSPELLED" where the misspelling
wasn't noticed until much later.

> +/*
> + * Provide support for effectively turning the timer interrupt on and
> + * off via the interrupt mask.  Make sure not to unmask it while we are
> + * running the timer interrupt handler, to avoid recursive timer
> + * interrupts; these may be OK in some cases, but it's generally cleaner
> + * to reset the kernel stack before starting the next timer interrupt.
>
>   Which would already be guaranteed by the generic interrupt code ....
>   The clockevent callbacks are already called with interrupts
>   disabled, so why all this magic ?
>   

The code was written so that it would be robust in the face of the timer
interrupt-path code potentially enabling interrupts, since I couldn't
convince myself it didn't.  I'll rip out all that code and replace it
with a couple of BUG() checks instead.  Thanks, that's a nice cleanup.

And thanks again for the feedback.  It's very helpful.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists