[Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3036 bytes --]

Hi,

I came across a few things in latest 2.6.19-i386-1.6-01 patch:

The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
Linux lock here), and I do not see any volatile data it could protect anyway. So let's
remove it.

--- linux-2.6.19-ipipe.orig/kernel/ipipe/core.c
+++ linux-2.6.19-ipipe/kernel/ipipe/core.c
@@ -1251,8 +1251,6 @@ static int __ipipe_common_info_proc(char
 	unsigned irq;
 	int len;
 
-	spin_lock(&__ipipe_pipelock);
-
 	p += sprintf(p, "       +----- Handling ([A]ccepted, [G]rabbed, [W]ired, [D]iscarded)\n");
 	p += sprintf(p, "       |+---- Sticky\n");
 	p += sprintf(p, "       ||+--- Locked\n");
@@ -1336,8 +1334,6 @@ static int __ipipe_common_info_proc(char
 	else
 		p += sprintf(p, "priority=%d\n", ipd->priority);
 
-	spin_unlock(&__ipipe_pipelock);
-
 	len = p - page;
 
 	if (len <= off + count)



The hard IRQ state of the tracer output got inverted during recent restructuring. This
fixes it (and indents a few labels correctly).

--- linux-2.6.19-ipipe.orig/kernel/ipipe/tracer.c
+++ linux-2.6.19-ipipe/kernel/ipipe/tracer.c
@@ -279,7 +279,7 @@ __ipipe_trace(enum ipipe_trace_type type
 	local_irq_save_hw_notrace(flags);
 
 	cpu_id = ipipe_processor_id();
-restart:
+ restart:
 	tp = old_tp = &trace_paths[cpu_id][active_path[cpu_id]];
 
 	/* here starts a race window with NMIs - catched below */
@@ -322,7 +322,7 @@ restart:
 
 	/* store all trace point data */
 	point->type = type;
-	point->flags = raw_irqs_disabled_flags(flags) ? 0 : IPIPE_TFLG_HWIRQ_OFF;
+	point->flags = raw_irqs_disabled_flags(flags) ? IPIPE_TFLG_HWIRQ_OFF : 0;
 	point->eip = eip;
 	point->parent_eip = parent_eip;
 	point->v = v;
@@ -367,7 +367,7 @@ restart:
 		/* store the path's end (i.e. excluding post-trace) */
 		tp->end = WRAP_POINT_NO(pos - post_trace + tp->post_trace);
 
-enforce_end:
+ enforce_end:
 		if (tp->flags & IPIPE_TFLG_FREEZING)
 			tp = __ipipe_trace_freeze(cpu_id, tp, pos);
 		else
@@ -411,7 +411,7 @@ static unsigned long __ipipe_global_path
 	spin_lock_irqsave(&global_path_lock, flags);
 
 	cpu_id = ipipe_processor_id();
-restart:
+ restart:
 	tp = &trace_paths[cpu_id][active_path[cpu_id]];
 
 	/* here is small race window with NMIs - catched below */



And finally, ipipe_init_proc can be moved to the __init section.

--- linux-2.6.19-ipipe.orig/kernel/ipipe/core.c
+++ linux-2.6.19-ipipe/kernel/ipipe/core.c
@@ -1362,7 +1362,7 @@ void __ipipe_remove_domain_proc(struct i
 	remove_proc_entry(ipd->name,ipipe_proc_root);
 }
 
-void ipipe_init_proc(void)
+void __init ipipe_init_proc(void)
 {
 	ipipe_proc_root = create_proc_entry("ipipe",S_IFDIR, 0);
 	create_proc_read_entry("version",0444,ipipe_proc_root,&__ipipe_version_info_proc,NULL);



There is more cleanup stuff in my pipe, one of it (type-based spinlock selection)
could be helpful for follow-up work on other archs. It's almost done, I will try to
prepare patch series over the next days.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Philippe Gerum]

On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
> Hi,
> 
> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
> 
> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
> remove it.

The interrupt status word, and whether any virtual interrupt is
allocated or not, are the volatile data protected by this lock on a SMP
system. Since this is a common spinlock with no interrupt control
required which is only used over the Linux domain (/proc handler), you
don't need to go for the _hw() form of it.

[...]

> 
> The hard IRQ state of the tracer output got inverted during recent restructuring. This
> fixes it (and indents a few labels correctly).
> 

Ack. Merged.

[...]

> And finally, ipipe_init_proc can be moved to the __init section.
> 
> --- linux-2.6.19-ipipe.orig/kernel/ipipe/core.c
> +++ linux-2.6.19-ipipe/kernel/ipipe/core.c
> @@ -1362,7 +1362,7 @@ void __ipipe_remove_domain_proc(struct i
>  	remove_proc_entry(ipd->name,ipipe_proc_root);
>  }
>  
> -void ipipe_init_proc(void)
> +void __init ipipe_init_proc(void)
>  {
>  	ipipe_proc_root = create_proc_entry("ipipe",S_IFDIR, 0);
>  	create_proc_read_entry("version",0444,ipipe_proc_root,&__ipipe_version_info_proc,NULL);
> 

Ack. Merged.

> 
> 
> There is more cleanup stuff in my pipe, one of it (type-based spinlock selection)
> could be helpful for follow-up work on other archs. It's almost done, I will try to
> prepare patch series over the next days.

Thanks.

-- 
Philippe.

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Philippe Gerum]

On Sun, 2006-12-03 at 15:43 +0100, Philippe Gerum wrote:
> On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
> > Hi,
> > 
> > I came across a few things in latest 2.6.19-i386-1.6-01 patch:
> > 
> > The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
> > Linux lock here), and I do not see any volatile data it could protect anyway. So let's
> > remove it.
> 
> The interrupt status word, and whether any virtual interrupt is
> allocated or not, are the volatile data protected by this lock on a SMP
> system. Since this is a common spinlock with no interrupt control
> required which is only used over the Linux domain (/proc handler), you
> don't need to go for the _hw() form of it.
> 

M, actually, the problem is elsewhere: we should disable hw IRQs here,
so that other domains could not compete with us on this lock.

-- 
Philippe.

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]

Philippe Gerum wrote:
> On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
>> Hi,
>>
>> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
>>
>> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
>> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
>> remove it.
> 
> The interrupt status word, and whether any virtual interrupt is
> allocated or not, are the volatile data protected by this lock on a SMP
> system. Since this is a common spinlock with no interrupt control
> required which is only used over the Linux domain (/proc handler), you
> don't need to go for the _hw() form of it.

As far as I see, nothing prevents the other users of __ipipe_pipelock to
be executed over non-root domain (IRQ registration in Xenomai context is
allowed, no?).

But I have to re-check what data for __ipipe_common_info_proc actually
can be released (I'm not considering inconsistency a problem here). I
didn't see anything on first review.

Still, this kind of merging of _hw with non-_hw spinlock operations is
fishy in my eyes.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Philippe Gerum]

On Sun, 2006-12-03 at 16:05 +0100, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
> >> Hi,
> >>
> >> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
> >>
> >> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
> >> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
> >> remove it.
> > 
> > The interrupt status word, and whether any virtual interrupt is
> > allocated or not, are the volatile data protected by this lock on a SMP
> > system. Since this is a common spinlock with no interrupt control
> > required which is only used over the Linux domain (/proc handler), you
> > don't need to go for the _hw() form of it.
> 
> As far as I see, nothing prevents the other users of __ipipe_pipelock to
> be executed over non-root domain (IRQ registration in Xenomai context is
> allowed, no?).
> 

Indeed, this is also the sense of my second reply:
https://mail.gna.org/public/adeos-main/2006-12/msg00004.html

Which means that our problem is more an issue regarding preemption by
interrupts.

> But I have to re-check what data for __ipipe_common_info_proc actually
> can be released (I'm not considering inconsistency a problem here). I
> didn't see anything on first review.
> 
> Still, this kind of merging of _hw with non-_hw spinlock operations is
> fishy in my eyes.
> 

It's not without interrupt control, provided you accept the possible
side-effects regarding kernel preemption, which /proc handlers do. What
would have been really problematic is a mismatch between
spin_lock_irqsave_hw and spinlock_irqsave forms, and what is really a
bug is the current lack of protection wrt interrupt.

> Jan
> 
> _______________________________________________
> Adeos-main mailing list
> Adeos-main@domain.hid
> https://mail.gna.org/listinfo/adeos-main
-- 
Philippe.

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

Philippe Gerum wrote:
> On Sun, 2006-12-03 at 16:05 +0100, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
>>>> Hi,
>>>>
>>>> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
>>>>
>>>> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
>>>> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
>>>> remove it.
>>> The interrupt status word, and whether any virtual interrupt is
>>> allocated or not, are the volatile data protected by this lock on a SMP
>>> system. Since this is a common spinlock with no interrupt control
>>> required which is only used over the Linux domain (/proc handler), you
>>> don't need to go for the _hw() form of it.
>> As far as I see, nothing prevents the other users of __ipipe_pipelock to
>> be executed over non-root domain (IRQ registration in Xenomai context is
>> allowed, no?).
>>
> 
> Indeed, this is also the sense of my second reply:
> https://mail.gna.org/public/adeos-main/2006-12/msg00004.html
> 
> Which means that our problem is more an issue regarding preemption by
> interrupts.
> 
>> But I have to re-check what data for __ipipe_common_info_proc actually
>> can be released (I'm not considering inconsistency a problem here). I
>> didn't see anything on first review.
>>
>> Still, this kind of merging of _hw with non-_hw spinlock operations is
>> fishy in my eyes.
>>
> 
> It's not without interrupt control, provided you accept the possible
> side-effects regarding kernel preemption, which /proc handlers do. What
> would have been really problematic is a mismatch between
> spin_lock_irqsave_hw and spinlock_irqsave forms, and what is really a
> bug is the current lack of protection wrt interrupt.

I re-checked the code, and I can only repeat that I see ZERO need for
this lock here. All we are reading is the control mask from static IRQ
arrays + some bits from the related I-pipe domain.

If there is actually something to protect, than it should be calling
this proc handler vs. unregistering the domain (and it's proc entry) -
but that is only feasible with something like synchronize_sched, i.e.
waiting a grace period after unregistering so that all handlers are
through. A really uncritical race which exists with a lot of /proc code.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Philippe Gerum]

On Sun, 2006-12-03 at 19:01 +0100, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Sun, 2006-12-03 at 16:05 +0100, Jan Kiszka wrote:
> >> Philippe Gerum wrote:
> >>> On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
> >>>> Hi,
> >>>>
> >>>> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
> >>>>
> >>>> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
> >>>> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
> >>>> remove it.
> >>> The interrupt status word, and whether any virtual interrupt is
> >>> allocated or not, are the volatile data protected by this lock on a SMP
> >>> system. Since this is a common spinlock with no interrupt control
> >>> required which is only used over the Linux domain (/proc handler), you
> >>> don't need to go for the _hw() form of it.
> >> As far as I see, nothing prevents the other users of __ipipe_pipelock to
> >> be executed over non-root domain (IRQ registration in Xenomai context is
> >> allowed, no?).
> >>
> > 
> > Indeed, this is also the sense of my second reply:
> > https://mail.gna.org/public/adeos-main/2006-12/msg00004.html
> > 
> > Which means that our problem is more an issue regarding preemption by
> > interrupts.
> > 
> >> But I have to re-check what data for __ipipe_common_info_proc actually
> >> can be released (I'm not considering inconsistency a problem here). I
> >> didn't see anything on first review.
> >>
> >> Still, this kind of merging of _hw with non-_hw spinlock operations is
> >> fishy in my eyes.
> >>
> > 
> > It's not without interrupt control, provided you accept the possible
> > side-effects regarding kernel preemption, which /proc handlers do. What
> > would have been really problematic is a mismatch between
> > spin_lock_irqsave_hw and spinlock_irqsave forms, and what is really a
> > bug is the current lack of protection wrt interrupt.
> 
> I re-checked the code, and I can only repeat that I see ZERO need for
> this lock here. All we are reading is the control mask from static IRQ
> arrays + some bits from the related I-pipe domain.

We do want the IRQ descriptors to be in a stable state while dumping
their contents, wrt ipipe_virtualize_irq() and ipipe_control_irq(). It
could be seen as a conservative measure, but it's saner than assuming
that fields from the IRQ arrays will never relate to each other wrt what
is dumped by the /proc handler. However, this global spinlock should be
moved as per-domain lock, there is no need for global contention here.

> 
> If there is actually something to protect, than it should be calling
> this proc handler vs. unregistering the domain (and it's proc entry) -
> but that is only feasible with something like synchronize_sched, i.e.
> waiting a grace period after unregistering so that all handlers are
> through. A really uncritical race which exists with a lot of /proc code.
> 

This race could crash the box, would the descriptor from the
unregistered domain belong to a module being unloaded. Since
ipipe_register_domain() grabs the critical lock, masking IRQs in
the /proc handler would do the trick, but this is a fairly high price to
pay for running such a routine. 

> Jan
> 
-- 
Philippe.

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

Philippe Gerum wrote:
> On Sun, 2006-12-03 at 19:01 +0100, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Sun, 2006-12-03 at 16:05 +0100, Jan Kiszka wrote:
>>>> Philippe Gerum wrote:
>>>>> On Sun, 2006-12-03 at 15:13 +0100, Jan Kiszka wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I came across a few things in latest 2.6.19-i386-1.6-01 patch:
>>>>>>
>>>>>> The usage of __ipipe_pipelock in __ipipe_common_info_proc is broken (raw lock used as
>>>>>> Linux lock here), and I do not see any volatile data it could protect anyway. So let's
>>>>>> remove it.
>>>>> The interrupt status word, and whether any virtual interrupt is
>>>>> allocated or not, are the volatile data protected by this lock on a SMP
>>>>> system. Since this is a common spinlock with no interrupt control
>>>>> required which is only used over the Linux domain (/proc handler), you
>>>>> don't need to go for the _hw() form of it.
>>>> As far as I see, nothing prevents the other users of __ipipe_pipelock to
>>>> be executed over non-root domain (IRQ registration in Xenomai context is
>>>> allowed, no?).
>>>>
>>> Indeed, this is also the sense of my second reply:
>>> https://mail.gna.org/public/adeos-main/2006-12/msg00004.html
>>>
>>> Which means that our problem is more an issue regarding preemption by
>>> interrupts.
>>>
>>>> But I have to re-check what data for __ipipe_common_info_proc actually
>>>> can be released (I'm not considering inconsistency a problem here). I
>>>> didn't see anything on first review.
>>>>
>>>> Still, this kind of merging of _hw with non-_hw spinlock operations is
>>>> fishy in my eyes.
>>>>
>>> It's not without interrupt control, provided you accept the possible
>>> side-effects regarding kernel preemption, which /proc handlers do. What
>>> would have been really problematic is a mismatch between
>>> spin_lock_irqsave_hw and spinlock_irqsave forms, and what is really a
>>> bug is the current lack of protection wrt interrupt.
>> I re-checked the code, and I can only repeat that I see ZERO need for
>> this lock here. All we are reading is the control mask from static IRQ
>> arrays + some bits from the related I-pipe domain.
> 
> We do want the IRQ descriptors to be in a stable state while dumping
> their contents, wrt ipipe_virtualize_irq() and ipipe_control_irq(). It
> could be seen as a conservative measure, but it's saner than assuming
> that fields from the IRQ arrays will never relate to each other wrt what
> is dumped by the /proc handler. However, this global spinlock should be
> moved as per-domain lock, there is no need for global contention here.

/me still not convinced. Neither ipipe_virtualize_irq() nor
ipipe_control_irq() are used at a relevant rate so that user will see
inconsistent dumps here. But if you really see a need, let us please use
a sequence lock-like mechanism. It's not worth to stall anyone just for
the sake of debug /proc output.

> 
>> If there is actually something to protect, than it should be calling
>> this proc handler vs. unregistering the domain (and it's proc entry) -
>> but that is only feasible with something like synchronize_sched, i.e.
>> waiting a grace period after unregistering so that all handlers are
>> through. A really uncritical race which exists with a lot of /proc code.
>>
> 
> This race could crash the box, would the descriptor from the
> unregistered domain belong to a module being unloaded. Since
> ipipe_register_domain() grabs the critical lock, masking IRQs in
> the /proc handler would do the trick, but this is a fairly high price to
> pay for running such a routine. 
> 

Better no IRQ lock for this.

Well, also my synchronize_sched idea is not truly safe (unless the proce
handler would be called under rcu_read_lock - don't think this is the
case). I really can't tell how to safely cleanup proc entries that have
volatile data structures attached. I once asked on LKML for a correct
pattern but got no reply.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Adeos-main] [PATCHES] cleanup minor quirks for 1.6-01

[Philippe Gerum]

On Thu, 2006-12-07 at 00:38 +0100, Jan Kiszka wrote:

> > 
> >> If there is actually something to protect, than it should be calling
> >> this proc handler vs. unregistering the domain (and it's proc entry) -
> >> but that is only feasible with something like synchronize_sched, i.e.
> >> waiting a grace period after unregistering so that all handlers are
> >> through. A really uncritical race which exists with a lot of /proc code.
> >>
> > 
> > This race could crash the box, would the descriptor from the
> > unregistered domain belong to a module being unloaded. Since
> > ipipe_register_domain() grabs the critical lock, masking IRQs in
> > the /proc handler would do the trick, but this is a fairly high price to
> > pay for running such a routine. 
> > 
> 
> Better no IRQ lock for this.
> 

Clearly.

> Well, also my synchronize_sched idea is not truly safe (unless the proce
> handler would be called under rcu_read_lock - don't think this is the
> case). I really can't tell how to safely cleanup proc entries that have
> volatile data structures attached. I once asked on LKML for a correct
> pattern but got no reply.

Let's go for a simple mutex excluding the unregistration routine for
now. Not perfect, but still better than the previous situation. It's
been introduced in 1.6-02/x86.

-- 
Philippe.