lkml.org 
[lkml]   [1999]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch] SMP race fix [was Re: SMP lockup & 3c509 on 2.2.x [aka. the Deadly 'ping -f']]


On Thu, 6 May 1999, Andrea Arcangeli wrote:
>
> I thought the current semantic of synchronize_irq() is that
> synchronize_irq() simply flush the _running_ _irqhandler_ (not the
> raw-irq) and so is _far_ from making sure that there won't be a new irq
> after the call.

A pure synchronize_irq() yes.

But you missed the larger picture, which was that synchronize_irq() was
used at the end of "disable_irq()". And in that case, we know that there
can't be a new irq after the call, because we just disabled it (and we
don't depend on any magic hardware that we don't control directly for
disabling: the disabled state shows up in the irq descriptor data
structures, so we are _guaranteed_ that a new one cannot come in once we
have set those correctly).

So I again agree that there seems to be a race, I just disagree with the
approaches taken for that race, and especially patch#2 which I just think
is a no-op for the above reasons.

> I think it's _only_ disable_irq() that must make sure that no other irqX
> will run after the call. synchronize_irq() has not such semantic (at least
> according to me) and I don't consider it buggy (so it's not obvious that
> we want to make it bloated -> irqinprogress/irqdisabled aware).

Indeed. But the only real valid use for "synchronize_irq()" is some code
that looks something like this:

outw(DISABLE_IRQ, hardwareregister);
synchronize_irq();

where the point of synchronize_irq() is to make sure that we wait for any
pending irq that might be running now (or that might have _just_ been
raised before the outw).

However, the above use of synchronize_irq() may not actually be physically
possible (for the simple reason that the device and the CPU really aren't
synchronized ina very real sense, and there is nothing we can do at a low
interrupt controller layer to make them so), so I'm certainly willing to
entertain the notion of just making it a non-supported option.

> This also
> considering that cli() instead is perfectly safe for the underlined line:
>
> static inline void irq_enter(int cpu, unsigned int irq)
> {
> hardirq_enter(cpu);
> while (test_bit(0,&global_irq_lock)) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> /* nothing */;
> }
> }
>
> But if you then issue a sti() (as synchronize_irq() does) the pending irq
> will unblock (and it will be allowed to race in the disable_irq case)
> then.

Oh, agreed - but synchronize_irq() doesn't make sense on its own: it only
makes sense in conjunction with "disable_irq()" or a device-specific
disable operation.

Linus


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

\
 
 \ /
  Last update: 2005-03-22 13:51    [W:0.066 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site