Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Michael S. Tsirkin]

On Fri, Dec 17, 2010 at 05:32:46PM +0100, Thomas Gleixner wrote:
 On Fri, 17 Dec 2010, Jan Kiszka wrote:
  Am 17.12.2010 16:25, Thomas Gleixner wrote:
   Your aproach with disable_irq_nosync() is completely flawed, simply
   because you try to pretend that your interrupt handler is done, while
   it is not done at all. The threaded interrupt handler is done when
   user space completes. Everything else is just hacking around the
   problem and creates all that nasty transitional problems.
  
  disable_irq_nosync is the pattern currently used in KVM, it's nothing
  new in fact.
 
 That does not make it less flawed :)
 
  The approach looks interesting but requires separate code for
  non-PCI-2.3 devices, i.e. when we have no means to mask at device level.
 
 Why? You can have the same code, you just can't request COND_ONESHOT
 handlers for it, so it needs unshared ONESHOT or it won't work at all,
 no matter what approach you chose. No device level mask, no sharing,
 it's that simple.
 
  Further drawbacks - unless I missed something on first glance:
  
  - prevents any future optimizations that would work without IRQ thread
ping-pong (ie. once we allow guest IRQ injection from hardirq context
for selected but typical setups)
  - two additional, though light-weight, context switches on each
interrupt completion
 
 The drawback of these two points is way less than the horror which you
 need to introduce to do the whole async handler disable, userspace
 enable dance. Robust and simple solutions really a preferred over
 complex and fragile horror which has a questionable runtime benefit.

I'd like to note that the overhead of involving the scheduler in
interrupt injection for an assigned device should be easily measurable:
just make the MSI handlers threaded and see what the result is.

In the case of emulated devices, when we had an extra thread
involved in MSI handling, the vcpu thread and the
interrupt injection thread were competing for cpu with strange
fluctuations in performance as the result
(i.e. sometimes we would get good speed as threading would introduce
a kind of interrupt coalescing, sometimes we would get huge latency).

  - continuous polling if user space decides to leave the interrupt
unhandled (e.g. because the virtual IRQ line is masked)
  
 That should be a solvable problem.
 
 Thanks,
 
   tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 16.12.2010 21:26, Jan Kiszka wrote:
 Am 16.12.2010 14:13, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 +   if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 +   !(desc-irq_data.drv_status  IRQS_SHARED)) {
 +   /*
 +* Signal the old handler that is has to switch to shareable
 +* handling mode. Disable the line to avoid any conflict with
 +* a real IRQ.
 +*/
 +   disable_irq(irq);

 This is weird, really. I thought you wanted to avoid waiting for the
 threaded handler to finish if it's on the fly. So this should be
 disable_irq_nosync() or did you change your mind ?
 
 No, I did not. I wanted to avoid that we set MAKE_SHAREABLE while there
 might be another IRQ in flight. The handler that is called due to a real
 IRQ might misinterpret MAKE_SHAREABLE as there is no real event and
 perform the wrong steps (at least the current implementation for KVM would).

Actually, the requirement we have to fulfill here is to avoid that the
hardirq handler sees !SHARED while the threaded one reads SHARED. To
achieve this without disabling the line, I'm still searching for a way
to couple the sharing state of associated hard and threaded handler runs
- but I think there is no reliable association, is there?

Jan



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 16.12.2010 21:26, Jan Kiszka wrote:
  Am 16.12.2010 14:13, Thomas Gleixner wrote:
  On Mon, 13 Dec 2010, Jan Kiszka wrote:
  + if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
  + !(desc-irq_data.drv_status  IRQS_SHARED)) {
  + /*
  +  * Signal the old handler that is has to switch to shareable
  +  * handling mode. Disable the line to avoid any conflict with
  +  * a real IRQ.
  +  */
  + disable_irq(irq);
 
  This is weird, really. I thought you wanted to avoid waiting for the
  threaded handler to finish if it's on the fly. So this should be
  disable_irq_nosync() or did you change your mind ?
  
  No, I did not. I wanted to avoid that we set MAKE_SHAREABLE while there
  might be another IRQ in flight. The handler that is called due to a real
  IRQ might misinterpret MAKE_SHAREABLE as there is no real event and
  perform the wrong steps (at least the current implementation for KVM would).
 
 Actually, the requirement we have to fulfill here is to avoid that the
 hardirq handler sees !SHARED while the threaded one reads SHARED. To
 achieve this without disabling the line, I'm still searching for a way
 to couple the sharing state of associated hard and threaded handler runs
 - but I think there is no reliable association, is there?

Unfortunately not. So the only way to solve that is disabling the
interrupt which makes sure that all handlers have completed.

OTOH, if we have to disable anyway, then we could simply keep it
disabled across the installation of a new handler. That would make the
notification business go away, wouldn't it ?

Thanks,

tglx


--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 17.12.2010 11:23, Thomas Gleixner wrote:
 On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 16.12.2010 21:26, Jan Kiszka wrote:
 Am 16.12.2010 14:13, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 + if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 + !(desc-irq_data.drv_status  IRQS_SHARED)) {
 + /*
 +  * Signal the old handler that is has to switch to shareable
 +  * handling mode. Disable the line to avoid any conflict with
 +  * a real IRQ.
 +  */
 + disable_irq(irq);

 This is weird, really. I thought you wanted to avoid waiting for the
 threaded handler to finish if it's on the fly. So this should be
 disable_irq_nosync() or did you change your mind ?

 No, I did not. I wanted to avoid that we set MAKE_SHAREABLE while there
 might be another IRQ in flight. The handler that is called due to a real
 IRQ might misinterpret MAKE_SHAREABLE as there is no real event and
 perform the wrong steps (at least the current implementation for KVM would).

 Actually, the requirement we have to fulfill here is to avoid that the
 hardirq handler sees !SHARED while the threaded one reads SHARED. To
 achieve this without disabling the line, I'm still searching for a way
 to couple the sharing state of associated hard and threaded handler runs
 - but I think there is no reliable association, is there?
 
 Unfortunately not. So the only way to solve that is disabling the
 interrupt which makes sure that all handlers have completed.

Hmm, what a pity.

 
 OTOH, if we have to disable anyway, then we could simply keep it
 disabled across the installation of a new handler. That would make the
 notification business go away, wouldn't it ?

No, the notification is still necessary in case the registered handler
keeps the line off after returning from both hard and threaded handler.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 17.12.2010 11:23, Thomas Gleixner wrote:
  OTOH, if we have to disable anyway, then we could simply keep it
  disabled across the installation of a new handler. That would make the
  notification business go away, wouldn't it ?
 
 No, the notification is still necessary in case the registered handler
 keeps the line off after returning from both hard and threaded handler.

And how should that happen? If it is in oneshot mode then the line is
reenabled when the thread handler returns.

Thanks,

tglx

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 17.12.2010 11:41, Thomas Gleixner wrote:
 On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 17.12.2010 11:23, Thomas Gleixner wrote:
 OTOH, if we have to disable anyway, then we could simply keep it
 disabled across the installation of a new handler. That would make the
 notification business go away, wouldn't it ?

 No, the notification is still necessary in case the registered handler
 keeps the line off after returning from both hard and threaded handler.
 
 And how should that happen? If it is in oneshot mode then the line is
 reenabled when the thread handler returns.

disable_irq_nosync is called by the handler before returning. And it's
the handler's job to revert this, properly synchronizing it internally.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Fri, 17 Dec 2010, Jan Kiszka wrote:

 Am 17.12.2010 11:41, Thomas Gleixner wrote:
  On Fri, 17 Dec 2010, Jan Kiszka wrote:
  Am 17.12.2010 11:23, Thomas Gleixner wrote:
  OTOH, if we have to disable anyway, then we could simply keep it
  disabled across the installation of a new handler. That would make the
  notification business go away, wouldn't it ?
 
  No, the notification is still necessary in case the registered handler
  keeps the line off after returning from both hard and threaded handler.
  
  And how should that happen? If it is in oneshot mode then the line is
  reenabled when the thread handler returns.
 
 disable_irq_nosync is called by the handler before returning. And it's
 the handler's job to revert this, properly synchronizing it internally.

disable_irq_nosync() is really the worst thing to do. That's simply
not going to work without a lot of fuglyness.

What about the following:

primary_handler()
{
if (!shared)
return IRQ_WAKE_THREAD;

spin_lock(dev-irq_lock);

if (from_my_device() || dev-irq_thread_waiting) {
mask_dev();
dev-masked = true;
ret = IRQ_WAKE_THREAD;
} else
ret = IRQ_NONE;

spin_unlock(dev-irq_lock);
return ret;
}

check_timeout()
{
if (dev-irq_active  wait_longer())
return IRQ_WAKE_THREAD;
return IRQ_HANDLED;
}

unmask_dev_if_necessary()
{
if (dev-masked  dev-irq_active)
umask_dev();
}

threaded_handler()
{
if (!dev-irq_thread_waiting) {
spin_lock_irq(dev-irq_lock);
wake_user = do_magic_stuff_with_the_dev();
dev-irq_thread_waiting = wake_user;
spin_unlock(dev-irq_lock);
if (wake_user)
wake_up(user);
}

if (!dev-irq_thread_waiting) {
spin_lock_irq(dev-irq_lock);
unmask_dev_if_necessary();
spin_unlock(dev-irq_lock);
return IRQ_HANDLED;
}

/*
 * Wait for user space to complete. Timeout is to
 * avoid starvation of the irq line when
 * something goes wrong
 */
wait_for_completion_timeout(dev-compl, SENSIBLE_TIMEOUT);

spin_lock_irq(dev-irq_lock);
if (timedout) {
mask_dev();
dev-masked = true;
/*
 * Leave dev-irq_thread_waiting untouched and let
 * the core code reschedule us when check_timeout
 * decides it's worth to wait. In any case we leave
 * the device masked at the device level, so we don't
 * cause an interrupt storm.
 */
ret = check_timeout();
} else {
unmask_dev_if_necessary();
dev-irq_thread_waiting = false;
ret = IRQ_HANDLED;
}
spin_unlock(dev-irq_lock);
return ret;
}

userspace_complete()
{
complete(dev-irq_compl);
}

Your aproach with disable_irq_nosync() is completely flawed, simply
because you try to pretend that your interrupt handler is done, while
it is not done at all. The threaded interrupt handler is done when
user space completes. Everything else is just hacking around the
problem and creates all that nasty transitional problems.

The above code does not have them at all. The threaded handler does
not care at all about the dev_id shared state encoding and the state
transitions. It merily cares about the device internal status
dev-masked. Everything else is handled by the genirq code and the
litte status check in the primary handler.

Neither does the user space completion care about it. It just
completes and is completely oblivious of the irq line state/mode. And
really, the user space part should not care at all. It can set some
status before calling complete(), but that's driver specific stuff and
has nothing to do with the irq handling magic.

It requires a few not too intrusive modifications to the genirq code:

   - Rescheduling the thread handler on IRQ_WAKE_THREAD
   - Changing the oneshot finalizing a bit
   - Adding the status manipulations for request/free_irq

Now you might argue that the timeout is ugly, but I don't think it's
ugly at all. You need it anyway in case user space failed
completely. And coming back after 100ms to let the genirq code handle
a disable_irq() or synchronize_irq() is a reasonable request, it's the
error/corner case at all. If there is code which installs/removes an
interrupt handler on the same line every 5ms, then this code becomes
rightfully blocked out for 100ms or such.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 17.12.2010 16:25, Thomas Gleixner wrote:
 On Fri, 17 Dec 2010, Jan Kiszka wrote:
 
 Am 17.12.2010 11:41, Thomas Gleixner wrote:
 On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 17.12.2010 11:23, Thomas Gleixner wrote:
 OTOH, if we have to disable anyway, then we could simply keep it
 disabled across the installation of a new handler. That would make the
 notification business go away, wouldn't it ?

 No, the notification is still necessary in case the registered handler
 keeps the line off after returning from both hard and threaded handler.

 And how should that happen? If it is in oneshot mode then the line is
 reenabled when the thread handler returns.

 disable_irq_nosync is called by the handler before returning. And it's
 the handler's job to revert this, properly synchronizing it internally.
 
 disable_irq_nosync() is really the worst thing to do. That's simply
 not going to work without a lot of fuglyness.
 
 What about the following:
 
 primary_handler()
 {
   if (!shared)
   return IRQ_WAKE_THREAD;
 
   spin_lock(dev-irq_lock);
 
   if (from_my_device() || dev-irq_thread_waiting) {
   mask_dev();
   dev-masked = true;
   ret = IRQ_WAKE_THREAD;
   } else
   ret = IRQ_NONE;
 
   spin_unlock(dev-irq_lock);
   return ret;
 }
 
 check_timeout()
 {
   if (dev-irq_active  wait_longer())
   return IRQ_WAKE_THREAD;
   return IRQ_HANDLED;
 }
 
 unmask_dev_if_necessary()
 {
   if (dev-masked  dev-irq_active)
   umask_dev();
 }
 
 threaded_handler()
 {
   if (!dev-irq_thread_waiting) {
   spin_lock_irq(dev-irq_lock);
   wake_user = do_magic_stuff_with_the_dev();
   dev-irq_thread_waiting = wake_user;
   spin_unlock(dev-irq_lock);
   if (wake_user)
   wake_up(user);
   }
 
   if (!dev-irq_thread_waiting) {
   spin_lock_irq(dev-irq_lock);
   unmask_dev_if_necessary();
   spin_unlock(dev-irq_lock);
   return IRQ_HANDLED;
   }
 
   /*
* Wait for user space to complete. Timeout is to
* avoid starvation of the irq line when
* something goes wrong
*/
   wait_for_completion_timeout(dev-compl, SENSIBLE_TIMEOUT);
 
   spin_lock_irq(dev-irq_lock);
   if (timedout) {
   mask_dev();
   dev-masked = true;
   /*
* Leave dev-irq_thread_waiting untouched and let
* the core code reschedule us when check_timeout
* decides it's worth to wait. In any case we leave
* the device masked at the device level, so we don't
* cause an interrupt storm.
*/
   ret = check_timeout();
   } else {
   unmask_dev_if_necessary();
   dev-irq_thread_waiting = false;
   ret = IRQ_HANDLED;
   }
   spin_unlock(dev-irq_lock);
   return ret;
 }
 
 userspace_complete()
 {
   complete(dev-irq_compl);
 }
 
 Your aproach with disable_irq_nosync() is completely flawed, simply
 because you try to pretend that your interrupt handler is done, while
 it is not done at all. The threaded interrupt handler is done when
 user space completes. Everything else is just hacking around the
 problem and creates all that nasty transitional problems.

disable_irq_nosync is the pattern currently used in KVM, it's nothing
new in fact.

The approach looks interesting but requires separate code for
non-PCI-2.3 devices, i.e. when we have no means to mask at device level.
Further drawbacks - unless I missed something on first glance:

- prevents any future optimizations that would work without IRQ thread
  ping-pong (ie. once we allow guest IRQ injection from hardirq context
  for selected but typical setups)
- two additional, though light-weight, context switches on each
  interrupt completion
- continuous polling if user space decides to leave the interrupt
  unhandled (e.g. because the virtual IRQ line is masked)

Maybe the latter can be solved in a nicer way, but I don't think we can
avoid the first two. I'm not saying yet that they are killing this
approach, we just need to asses their relevance.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Fri, 17 Dec 2010, Jan Kiszka wrote:
 Am 17.12.2010 16:25, Thomas Gleixner wrote:
  Your aproach with disable_irq_nosync() is completely flawed, simply
  because you try to pretend that your interrupt handler is done, while
  it is not done at all. The threaded interrupt handler is done when
  user space completes. Everything else is just hacking around the
  problem and creates all that nasty transitional problems.
 
 disable_irq_nosync is the pattern currently used in KVM, it's nothing
 new in fact.

That does not make it less flawed :)

 The approach looks interesting but requires separate code for
 non-PCI-2.3 devices, i.e. when we have no means to mask at device level.

Why? You can have the same code, you just can't request COND_ONESHOT
handlers for it, so it needs unshared ONESHOT or it won't work at all,
no matter what approach you chose. No device level mask, no sharing,
it's that simple.

 Further drawbacks - unless I missed something on first glance:
 
 - prevents any future optimizations that would work without IRQ thread
   ping-pong (ie. once we allow guest IRQ injection from hardirq context
   for selected but typical setups)
 - two additional, though light-weight, context switches on each
   interrupt completion

The drawback of these two points is way less than the horror which you
need to introduce to do the whole async handler disable, userspace
enable dance. Robust and simple solutions really a preferred over
complex and fragile horror which has a questionable runtime benefit.

 - continuous polling if user space decides to leave the interrupt
   unhandled (e.g. because the virtual IRQ line is masked)
 
That should be a solvable problem.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Mon, 13 Dec 2010, Jan Kiszka wrote:
 + if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 + !(desc-irq_data.drv_status  IRQS_SHARED)) {
 + /*
 +  * Signal the old handler that is has to switch to shareable
 +  * handling mode. Disable the line to avoid any conflict with
 +  * a real IRQ.
 +  */
 + disable_irq(irq);

This is weird, really. I thought you wanted to avoid waiting for the
threaded handler to finish if it's on the fly. So this should be
disable_irq_nosync() or did you change your mind ?

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 16.12.2010 14:13, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 +if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 +!(desc-irq_data.drv_status  IRQS_SHARED)) {
 +/*
 + * Signal the old handler that is has to switch to shareable
 + * handling mode. Disable the line to avoid any conflict with
 + * a real IRQ.
 + */
 +disable_irq(irq);
 
 This is weird, really. I thought you wanted to avoid waiting for the
 threaded handler to finish if it's on the fly. So this should be
 disable_irq_nosync() or did you change your mind ?

No, I did not. I wanted to avoid that we set MAKE_SHAREABLE while there
might be another IRQ in flight. The handler that is called due to a real
IRQ might misinterpret MAKE_SHAREABLE as there is no real event and
perform the wrong steps (at least the current implementation for KVM would).

However, I will rebase my patch over your series now and try to re-think
this. The question is what could go wrong if we do not guarantee that
MAKE_SHAREABLE and ordinary IRQ will always be distinguishable. If there
is really nothing, specifically for the KVM scenario, we could even drop
the disable/enable_irq. That would be also be nicer when thinking about
potential delays of the already registered handler during this
transitional phase.

Jan



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 15.12.2010 09:05, Thomas Gleixner wrote:
 On Wed, 15 Dec 2010, Jan Kiszka wrote:
 
 Am 14.12.2010 22:46, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 From: Jan Kiszka jan.kis...@siemens.com
chip_bus_lock(desc);
retval = __setup_irq(irq, desc, action);
chip_bus_sync_unlock(desc);
  
 -  if (retval)
 +  if (retval) {
 +  if (desc-action  !desc-action-next)
 +  desc-irq_data.drv_status = ~IRQS_SHARED;

 This is redundant. IRQS_SHARED gets set in a code path where all
 checks are done already.

 Nope, it's also set before entry of __setup_irq in case we call an
 IRQF_ADAPTIVE handler.

 We need to set it that early as we may race with IRQ events for the
 already registered handler happening between the sharing notification
 and the actual registration of the second handler.
 
 Hmm, ok. Though the MAKE_SHAREABLE flag should be sufficient to do the
 notification.

For notification, yes. But we need SHARED once we reenable the line
after the notification.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Wed, 15 Dec 2010, Jan Kiszka wrote:
 Am 15.12.2010 09:05, Thomas Gleixner wrote:
  On Wed, 15 Dec 2010, Jan Kiszka wrote:
  
  Am 14.12.2010 22:46, Thomas Gleixner wrote:
  On Mon, 13 Dec 2010, Jan Kiszka wrote:
  From: Jan Kiszka jan.kis...@siemens.com
   chip_bus_lock(desc);
   retval = __setup_irq(irq, desc, action);
   chip_bus_sync_unlock(desc);
   
  -if (retval)
  +if (retval) {
  +if (desc-action  !desc-action-next)
  +desc-irq_data.drv_status = ~IRQS_SHARED;
 
  This is redundant. IRQS_SHARED gets set in a code path where all
  checks are done already.
 
  Nope, it's also set before entry of __setup_irq in case we call an
  IRQF_ADAPTIVE handler.
 
  We need to set it that early as we may race with IRQ events for the
  already registered handler happening between the sharing notification
  and the actual registration of the second handler.
  
  Hmm, ok. Though the MAKE_SHAREABLE flag should be sufficient to do the
  notification.
 
 For notification, yes. But we need SHARED once we reenable the line
 after the notification.

Darn. Will think more about it.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Wed, 15 Dec 2010, Jan Kiszka wrote:
 Am 14.12.2010 21:54, Thomas Gleixner wrote:
  On Mon, 13 Dec 2010, Jan Kiszka wrote:
  @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, 
  void *dev_id)
 /* Make sure it's not being used on another CPU: */
 synchronize_irq(irq);
   
  +  if (single_handler)
  +  desc-irq_data.drv_status = ~IRQS_SHARED;
  +
  
  What's the reason to clear this flag outside of the desc-lock held
  region.
 
 We need to synchronize the irq first before clearing the flag.
 
 The problematic scenario behind this: An IRQ started in shared mode,
 this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
 before calling into the threaded handler. And that handler may now think
 that the line is still masked as IRQS_SHARED is set.

That should read not set I guess. Hmm, needs more thoughts :(
 
  I need this status for other purposes as well, where I
  definitely need serialization.
 
 Well, two options: wrap all bit manipulations with desc-lock
 acquisition/release or turn drv_status into an atomic. I don't know what
 your plans with drv_status are, so...

Some bits for irq migration and other stuff, which allows us to avoid
fiddling with irqdesc in the drivers.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 15.12.2010 14:04, Thomas Gleixner wrote:
 On Wed, 15 Dec 2010, Jan Kiszka wrote:
 Am 14.12.2010 21:54, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, 
 void *dev_id)
/* Make sure it's not being used on another CPU: */
synchronize_irq(irq);
  
 +  if (single_handler)
 +  desc-irq_data.drv_status = ~IRQS_SHARED;
 +

 What's the reason to clear this flag outside of the desc-lock held
 region.

 We need to synchronize the irq first before clearing the flag.

 The problematic scenario behind this: An IRQ started in shared mode,
 this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
 before calling into the threaded handler. And that handler may now think
 that the line is still masked as IRQS_SHARED is set.
 
 That should read not set I guess.

Can't remember who wrote this, but that guy might have been too tired
for clear sentences: Yes, of course, we could run into troubles, if
IRQS_SHARED was _not_ set while the IRQ line is unmasked between hard
and threaded handler.

 Hmm, needs more thoughts :(

Be warned, might be painful.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Wed, 15 Dec 2010, Jan Kiszka wrote:

 Am 15.12.2010 14:04, Thomas Gleixner wrote:
  On Wed, 15 Dec 2010, Jan Kiszka wrote:
  Am 14.12.2010 21:54, Thomas Gleixner wrote:
  On Mon, 13 Dec 2010, Jan Kiszka wrote:
  @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int 
  irq, void *dev_id)
   /* Make sure it's not being used on another CPU: */
   synchronize_irq(irq);
   
  +if (single_handler)
  +desc-irq_data.drv_status = ~IRQS_SHARED;
  +
 
  What's the reason to clear this flag outside of the desc-lock held
  region.
 
  We need to synchronize the irq first before clearing the flag.
 
  The problematic scenario behind this: An IRQ started in shared mode,
  this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
  before calling into the threaded handler. And that handler may now think
  that the line is still masked as IRQS_SHARED is set.
  
  That should read not set I guess.
 
 Can't remember who wrote this, but that guy might have been too tired
 for clear sentences: Yes, of course, we could run into troubles, if
 IRQS_SHARED was _not_ set while the IRQ line is unmasked between hard
 and threaded handler.

Right.

As a side note, the current implementation requires that you lookup
irq_data.drv_status for every invocation of the handler or have a
reference to irq_data.drv_status somewhere locally, which I don't like
either.

I have an evil and nasy idea how to avoid that, need to look how ugly
it gets. Worst case we need to go back to that notification thing
which I wanted really avoid in the first place.
 
Though I like the register_mutex idea which came out of this a lot as
it allows us to reduce desc-lock held and interrupt disabled regions
quite nicely.

/me goes back to stare at the code

  Hmm, needs more thoughts :(
 
 Be warned, might be painful.

Bah, my brain became pain resistant when I started hacking that code.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Wed, 15 Dec 2010, Jan Kiszka wrote:

 Am 15.12.2010 14:04, Thomas Gleixner wrote:
  On Wed, 15 Dec 2010, Jan Kiszka wrote:
  Am 14.12.2010 21:54, Thomas Gleixner wrote:
  On Mon, 13 Dec 2010, Jan Kiszka wrote:
  @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int 
  irq, void *dev_id)
   /* Make sure it's not being used on another CPU: */
   synchronize_irq(irq);
   
  +if (single_handler)
  +desc-irq_data.drv_status = ~IRQS_SHARED;
  +
 
  What's the reason to clear this flag outside of the desc-lock held
  region.
 
  We need to synchronize the irq first before clearing the flag.
 
  The problematic scenario behind this: An IRQ started in shared mode,
  this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
  before calling into the threaded handler. And that handler may now think
  that the line is still masked as IRQS_SHARED is set.
  
  That should read not set I guess.
 
 Can't remember who wrote this, but that guy might have been too tired
 for clear sentences: Yes, of course, we could run into troubles, if
 IRQS_SHARED was _not_ set while the IRQ line is unmasked between hard
 and threaded handler.
 
  Hmm, needs more thoughts :(
 
 Be warned, might be painful.

Talking about headache. Your solution above does not prevent that
scenario.

 CPU 0  CPU 1

 synchronize_irq();
hard irq comes in sees shared and unmasks
 clear IRQS_SHARED
thread handler runs and sees !SHARED

Same scenario, just moved by a few lines :)

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 15.12.2010 16:41, Thomas Gleixner wrote:
 On Wed, 15 Dec 2010, Jan Kiszka wrote:
 
 Am 15.12.2010 14:04, Thomas Gleixner wrote:
 On Wed, 15 Dec 2010, Jan Kiszka wrote:
 Am 14.12.2010 21:54, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int 
 irq, void *dev_id)
  /* Make sure it's not being used on another CPU: */
  synchronize_irq(irq);
  
 +if (single_handler)
 +desc-irq_data.drv_status = ~IRQS_SHARED;
 +

 What's the reason to clear this flag outside of the desc-lock held
 region.

 We need to synchronize the irq first before clearing the flag.

 The problematic scenario behind this: An IRQ started in shared mode,
 this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
 before calling into the threaded handler. And that handler may now think
 that the line is still masked as IRQS_SHARED is set.

 That should read not set I guess.

 Can't remember who wrote this, but that guy might have been too tired
 for clear sentences: Yes, of course, we could run into troubles, if
 IRQS_SHARED was _not_ set while the IRQ line is unmasked between hard
 and threaded handler.

 Hmm, needs more thoughts :(

 Be warned, might be painful.
 
 Talking about headache. Your solution above does not prevent that
 scenario.
 
  CPU 0  CPU 1
   
  synchronize_irq();
   hard irq comes in sees shared and unmasks

Nope, IRQ_ONESHOT is already cleared at that point.

  clear IRQS_SHARED
   thread handler runs and sees !SHARED
 
 Same scenario, just moved by a few lines :)

The same, just the other way around - and mostly harmless, I hope. :)

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Wed, 15 Dec 2010, Jan Kiszka wrote:
 Am 15.12.2010 16:41, Thomas Gleixner wrote:
  Talking about headache. Your solution above does not prevent that
  scenario.
  
   CPU 0  CPU 1
  
   synchronize_irq();
  hard irq comes in sees shared and unmasks
 
 Nope, IRQ_ONESHOT is already cleared at that point.

Errm ? It's set. Could you please stop to increase my confusion ? :)

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Mon, 13 Dec 2010, Jan Kiszka wrote:
 @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, 
 void *dev_id)
   /* Make sure it's not being used on another CPU: */
   synchronize_irq(irq);
  
 + if (single_handler)
 + desc-irq_data.drv_status = ~IRQS_SHARED;
 +

What's the reason to clear this flag outside of the desc-lock held
region. I need this status for other purposes as well, where I
definitely need serialization.

 + mutex_lock(register_lock);
 +
 + old_action = desc-action;
 + if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 + !(desc-irq_data.drv_status  IRQS_SHARED)) {
 + /*
 +  * Signal the old handler that is has to switch to shareable
 +  * handling mode. Disable the line to avoid any conflict with
 +  * a real IRQ.
 +  */
 + disable_irq(irq);
 + local_irq_disable();
 +
 + desc-irq_data.drv_status |= IRQS_SHARED | IRQS_MAKE_SHAREABLE;

Unserialized access as well. Will think about it.

 + old_action-handler(irq, old_action-dev_id);
 + desc-irq_data.drv_status = ~IRQS_MAKE_SHAREABLE;

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Thomas Gleixner]

On Mon, 13 Dec 2010, Jan Kiszka wrote:
 From: Jan Kiszka jan.kis...@siemens.com
   chip_bus_lock(desc);
   retval = __setup_irq(irq, desc, action);
   chip_bus_sync_unlock(desc);
  
 - if (retval)
 + if (retval) {
 + if (desc-action  !desc-action-next)
 + desc-irq_data.drv_status = ~IRQS_SHARED;

This is redundant. IRQS_SHARED gets set in a code path where all
checks are done already.

To make that more obvious we can set it right before

   raw_spin_unlock_irqrestore(desc-lock, flags);

conditionally on (shared).

That way we can also move the kfree out of the mutex locked section.

Thanks,

tglx
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 14.12.2010 21:54, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 @@ -943,6 +950,9 @@ static struct irqaction *__free_irq(unsigned int irq, 
 void *dev_id)
  /* Make sure it's not being used on another CPU: */
  synchronize_irq(irq);
  
 +if (single_handler)
 +desc-irq_data.drv_status = ~IRQS_SHARED;
 +
 
 What's the reason to clear this flag outside of the desc-lock held
 region.

We need to synchronize the irq first before clearing the flag.

The problematic scenario behind this: An IRQ started in shared mode,
this the line was unmasked after the hardirq. Now we clear IRQS_SHARED
before calling into the threaded handler. And that handler may now think
that the line is still masked as IRQS_SHARED is set.

 I need this status for other purposes as well, where I
 definitely need serialization.

Well, two options: wrap all bit manipulations with desc-lock
acquisition/release or turn drv_status into an atomic. I don't know what
your plans with drv_status are, so...

 
 +mutex_lock(register_lock);
 +
 +old_action = desc-action;
 +if (old_action  (old_action-flags  IRQF_ADAPTIVE) 
 +!(desc-irq_data.drv_status  IRQS_SHARED)) {
 +/*
 + * Signal the old handler that is has to switch to shareable
 + * handling mode. Disable the line to avoid any conflict with
 + * a real IRQ.
 + */
 +disable_irq(irq);
 +local_irq_disable();
 +
 +desc-irq_data.drv_status |= IRQS_SHARED | IRQS_MAKE_SHAREABLE;
 
 Unserialized access as well. Will think about it.
 
 +old_action-handler(irq, old_action-dev_id);
 +desc-irq_data.drv_status = ~IRQS_MAKE_SHAREABLE;
 
 Thanks,
 
   tglx

Jan



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 2/4] genirq: Inform handler about line sharing state

[Jan Kiszka]

Am 14.12.2010 22:46, Thomas Gleixner wrote:
 On Mon, 13 Dec 2010, Jan Kiszka wrote:
 From: Jan Kiszka jan.kis...@siemens.com
  chip_bus_lock(desc);
  retval = __setup_irq(irq, desc, action);
  chip_bus_sync_unlock(desc);
  
 -if (retval)
 +if (retval) {
 +if (desc-action  !desc-action-next)
 +desc-irq_data.drv_status = ~IRQS_SHARED;
 
 This is redundant. IRQS_SHARED gets set in a code path where all
 checks are done already.

Nope, it's also set before entry of __setup_irq in case we call an
IRQF_ADAPTIVE handler.

We need to set it that early as we may race with IRQ events for the
already registered handler happening between the sharing notification
and the actual registration of the second handler.

Jan



signature.asc
Description: OpenPGP digital signature