Re: 2.6.26.3-rt3

["John Kacur" <jkacur@gmail.com>]

On Sat, Aug 23, 2008 at 3:36 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Fri, 22 Aug 2008, Steven Rostedt wrote:
>
>>
>>
>> On Sat, 23 Aug 2008, John Kacur wrote:
>> >
>> > One more patch that was missed - it was discussed here
>> > http://marc.info/?l=linux-rt-users&m=121846031913931&w=2
>> >
>> > I am resending it, please consider for -rt4.
>> > Without it I continue to get the following type of message.
>> >
>>
>> Actually this was left out intentionally. The quick fix would be the one
>> that Gregory Haskins suggested about not reporting when the task is bound
>> to one CPU. This bothers me a little, but is probably OK for now. A task
>> can still migrate from one cpu to another by the user, and this can cause
>> havic if a smp_processor_id is used.

Hmnn, the e-mail thread died out at that point and moved to IRC after
we released that
debug_smp_processor_id(void)
ALREADY had a check to see if the kernel thread was bound to a single
cpu. (see below)

	/*
	 * Kernel threads bound to a single CPU can safely use
	 * smp_processor_id():
	 */
	this_mask = cpumask_of_cpu(this_cpu);

	if (cpus_equal(current->cpus_allowed, this_mask))
		goto out;

I'm inclined to believe that the test is correct and we really were
calling smp_processor_id in preemptible code which is why I offered my
patch up again.

>>
>> I needed to examine this a bit closer before coming up with a proper fix.
>
>
> This patch below should be sufficient. I just changed your local_irqs_save
> to preempt_disabled: I have this queued for -rt4, but that is where I also

Ok, good, I'm running with your lighter weight patch to make sure that
it is sufficient. Thanks.

> plan on adding the latest ftrace updates so it may take a bit to get it
> out.
>
> -- Steve
>
>
> =======
> From: Steven Rostedt <srostedt@redhat.com>
> Subject: suppress warning of smp_processor_id use.
>
> John Kacur pointed out that the get_cpu_var used in net/sched/sch_generic.c
> would trigger warnings. This was happing on a statistic variable and
> by a softirq which is bound to a single thread.
>
> John sent a patch that used local_irq_save which is a little bit of
> overkill. This version uses preempt disable, but we still need to create
> a preempt_disable_rt API that is only activated when PREEMPT_RT is configured.
>

Could you perhaps leave in my original Signed-off-by: or at least convert it to
Debugged-by: John Kacur <jkacur at gmail dot com>

> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  net/sched/sch_generic.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> Index: linux-2.6.26.3-rt3/net/sched/sch_generic.c
> ===================================================================
> --- linux-2.6.26.3-rt3.orig/net/sched/sch_generic.c     2008-08-22 21:28:50.000000000 -0400
> +++ linux-2.6.26.3-rt3/net/sched/sch_generic.c  2008-08-22 21:29:38.000000000 -0400
> @@ -112,7 +112,9 @@ static inline int handle_dev_cpu_collisi
>                 * Another cpu is holding lock, requeue & delay xmits for
>                 * some time.
>                 */
> +               preempt_disable(); /* FIXME: we need an _rt version of this */

I think you should drop the FIXME comment. The discussion was about
the debug version of smp_processor_id that is called via
__get_cpu_var() below, and it appears that it doesn't need any
fix-ups.
(Of course there is the chance that I misunderstood and you really do
mean some changes to preempt_disable() - let me know pls)

>                __get_cpu_var(netdev_rx_stat).cpu_collision++;
> +               preempt_enable();
>                ret = dev_requeue_skb(skb, dev, q);
>        }
>
>