linux-kernel - Re: [RFC][PATCH] irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 25 Jun 2010 11:30:45 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Huang Ying <ying.huang@...el.com>
Cc:	Ingo Molnar <mingo@...e.hu>, "H.PeterA" <"nvin hpa"@zytor.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [RFC][PATCH] irq_work

On Fri, 2010-06-25 at 17:17 +0800, Huang Ying wrote:
> On Fri, 2010-06-25 at 15:48 +0800, Peter Zijlstra wrote:
> > On Fri, 2010-06-25 at 10:12 +0800, Huang Ying wrote:
> > > 
> > > It is better to add "void *data" field in this struct to allow same
> > > function can be used for multiple struct irq_work. 
> > 
> > No, simply do:
> > 
> > struct my_foo {
> >   struct irq_work work;
> >   /* my extra data */
> > }
> > 
> > void my_func(struct irq_work *work)
> > {
> >   struct my_foo *foo = container_of(work, struct my_foo, work);
> > 
> >   /* tada! */
> > }
> 
> Yes. This works too. But Adding "void *data" field is helpful if you do
> not embed struct irq_work into another struct.

No, embedding is the normal way we do this in the kernel.

> > > And I think IRQ is the implementation detail here, so irq_work is
> > > probably not a good name. nmi_return_notifier or nmi_callback is better?
> > 
> > Well, its ran in hard-irq context, so its an irq work. There's nothing
> > that says it can only be used from NMI context.
> 
> It may be run in other contexts on some system (without APIC). 

I would consider that a BUG. Use a random IRQ source to process the
callbacks on these broken platforms.

> And I
> don't think it is useful to others except NMI handler. I think this is a
> choice between naming after implementation and purpose.

There is, although I'm sure people will yell at me for even proposing
this. You can raise the IPI from an IRQ disabled section to get
something done right after it.

> We can use another flag to signify whether it is executing. For example
> the bit 0 of entry->next.

There's no point.

> > I think clearing after the function is done executing is the only sane
> > semantics (and yes, I should fix the current perf code).
> > 
> > You can always miss an NMI since it can always happen before the
> > callback gets done, and allowing another enqueue before the callback is
> > complete is asking for trouble.
> 
> If we move entry->next = NULL before entry->func(entry), we will not
> miss the NMI. Can you show how to miss it in this way?

<NMI>
  ...
  irq_work_queue(&my_work, func);
  ...
<EOI>
<IPI>
  irq_work_run()

  <NMI>
    irq_work_queue(&my_work, func); <FAIL>
  <EOI>

   my_func.next = NULL;
<EOI>

Really not that hard. Now imagine wrapping irq_work in some state and
you reusing the state while the function is still running..
--
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/