Preempt & Xfs Question

Preempt & Xfs Question

[Matthias-Christian Ott]

Hi!
I have a question: Why do I get such debug messages:

BUG: using smp_processor_id() in preemptible [00000001] code: khelper/892
caller is _pagebuf_lookup_pages+0x11b/0x362
 [<c03119c7>] smp_processor_id+0xa3/0xb4
 [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362
 [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362
 [<c02efe31>] xfs_buf_get_flags+0x106/0x141
 [<c02efeaa>] xfs_buf_read_flags+0x3e/0xae
 [<c02e0fd5>] xfs_trans_read_buf+0x18b/0x389
 [<c02ada32>] xfs_da_do_buf+0x735/0x88c
 [<c0127f46>] run_timer_softirq+0x12d/0x1c7
 [<c02adc1b>] xfs_da_read_buf+0x47/0x4b
 [<c02b1b22>] xfs_dir2_block_lookup_int+0x52/0x1b4
 [<c02b1b22>] xfs_dir2_block_lookup_int+0x52/0x1b4
 [<c011f189>] vprintk+0x133/0x16a
 [<c0101319>] kernel_thread_helper+0x5/0xb
 [<c0130253>] __kernel_text_address+0x28/0x36
 [<c02b1a10>] xfs_dir2_block_lookup+0x2f/0xef
 [<c02b0967>] xfs_dir2_isblock+0x2c/0x74
 [<c02aff4e>] xfs_dir2_lookup+0x176/0x186
 [<c01389d9>] __print_symbol+0x86/0xef
 [<c02e214e>] xfs_dir_lookup_int+0x4c/0x144
 [<c04cf6aa>] _spin_lock+0x16/0x7c
 [<c02e7cc8>] xfs_lookup+0x55/0x8a
 [<c02f4736>] linvfs_lookup+0x52/0x99
 [<c01698a2>] real_lookup+0xc2/0xe3
 [<c0169b2c>] do_lookup+0x96/0xa1
 [<c0169cd0>] link_path_walk+0x199/0xe3a
 [<c012a2f1>] do_notify_parent+0x108/0x214
 [<c016ac39>] path_lookup+0x97/0x15f
 [<c0166612>] open_exec+0x2f/0x102
 [<c011834b>] task_rq_lock+0x36/0x66
 [<c01676ed>] do_execve+0x42/0x201
 [<c0101cc1>] sys_execve+0x46/0x9c
 [<c010318b>] syscall_call+0x7/0xb
 [<c012e60e>] ____call_usermodehelper+0xa1/0xc0
 [<c012e56d>] ____call_usermodehelper+0x0/0xc0
 [<c0101319>] kernel_thread_helper+0x5/0xb

Does the XFS Module avoid preemption rules? If so, why?

Thanks
Matthias-Christian Ott

-- 
http://unixforge.org/~matthias-christian-ott/

Re: Preempt & Xfs Question

[Steve Lord]

Matthias-Christian Ott wrote:
> Hi!
> I have a question: Why do I get such debug messages:
> 
> BUG: using smp_processor_id() in preemptible [00000001] code: khelper/892
> caller is _pagebuf_lookup_pages+0x11b/0x362
> [<c03119c7>] smp_processor_id+0xa3/0xb4
> [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362
> [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362

.....

> 
> Does the XFS Module avoid preemption rules? If so, why?

It is probably coming from these macros which keep various statistics
inside xfs as per cpu variables.

in fs//xfs/linux-2.6/xfs_stats.h:

DECLARE_PER_CPU(struct xfsstats, xfsstats);

/* We don't disable preempt, not too worried about poking the
  * wrong cpu's stat for now */
#define XFS_STATS_INC(count)            (__get_cpu_var(xfsstats).count++)
#define XFS_STATS_DEC(count)            (__get_cpu_var(xfsstats).count--)
#define XFS_STATS_ADD(count, inc)       (__get_cpu_var(xfsstats).count += (inc))

So it knows about the fact that preemption can mess up the result of this,
but it does not really matter for the purpose it is used for here. The
stats are just informational but very handy for working out what is going
on inside xfs. Using a global instead of a per cpu variable would
lead to cache line contention.

If you want to make it go away on a preemptable kernel, then use the
alternate definition of the stat macros which is just below the
above code.

Steve

p.s. try running xfs_stats.pl -f which comes with the xfs-cmds source to
watch the stats.

Re: Preempt & Xfs Question

[Chris Wedgwood]

> BUG: using smp_processor_id() in preemptible [00000001] code:
> khelper/892

fixed in CVS, I guess it will hit mainline soon

Re: Preempt & Xfs Question

[Matthias-Christian Ott]

Chris Wedgwood wrote:

>>BUG: using smp_processor_id() in preemptible [00000001] code:
>>khelper/892
>>    
>>
>
>fixed in CVS, I guess it will hit mainline soon
>
>  
>
How did you fix it?

Matthias-Christian Ott

-- 
http://unixforge.org/~matthias-christian-ott/

Re: Preempt & Xfs Question

[Chris Wedgwood]

On Thu, Jan 27, 2005 at 05:46:54PM +0000, Matthias-Christian Ott wrote:

> How did you fix it?

I suggested:

===== fs/xfs/linux-2.6/xfs_stats.h 1.9 vs edited =====
Index: cw-current/fs/xfs/linux-2.6/xfs_stats.h
===================================================================
--- cw-current.orig/fs/xfs/linux-2.6/xfs_stats.h	2005-01-17 16:03:59.656946818 -0800
+++ cw-current/fs/xfs/linux-2.6/xfs_stats.h	2005-01-17 16:06:50.692361597 -0800
@@ -142,9 +142,9 @@
 
 /* We don't disable preempt, not too worried about poking the
  * wrong cpu's stat for now */
-#define XFS_STATS_INC(count)		(__get_cpu_var(xfsstats).count++)
-#define XFS_STATS_DEC(count)		(__get_cpu_var(xfsstats).count--)
-#define XFS_STATS_ADD(count, inc)	(__get_cpu_var(xfsstats).count += (inc))
+#define XFS_STATS_INC(count)		(per_cpu(xfsstats, __smp_processor_id()).count++)
+#define XFS_STATS_DEC(count)		(per_cpu(xfsstats, __smp_processor_id()).count--)
+#define XFS_STATS_ADD(count, inc)	(per_cpu(xfsstats, __smp_processor_id()).count += (inc))
 
 extern void xfs_init_procfs(void);
 extern void xfs_cleanup_procfs(void);

but what was checked in was a bit cleaner.

Re: Preempt & Xfs Question

[Matthias-Christian Ott]

Chris Wedgwood wrote:

>On Thu, Jan 27, 2005 at 05:46:54PM +0000, Matthias-Christian Ott wrote:
>
>  
>
>>How did you fix it?
>>    
>>
>
>I suggested:
>
>===== fs/xfs/linux-2.6/xfs_stats.h 1.9 vs edited =====
>Index: cw-current/fs/xfs/linux-2.6/xfs_stats.h
>===================================================================
>--- cw-current.orig/fs/xfs/linux-2.6/xfs_stats.h	2005-01-17 16:03:59.656946818 -0800
>+++ cw-current/fs/xfs/linux-2.6/xfs_stats.h	2005-01-17 16:06:50.692361597 -0800
>@@ -142,9 +142,9 @@
> 
> /* We don't disable preempt, not too worried about poking the
>  * wrong cpu's stat for now */
>-#define XFS_STATS_INC(count)		(__get_cpu_var(xfsstats).count++)
>-#define XFS_STATS_DEC(count)		(__get_cpu_var(xfsstats).count--)
>-#define XFS_STATS_ADD(count, inc)	(__get_cpu_var(xfsstats).count += (inc))
>+#define XFS_STATS_INC(count)		(per_cpu(xfsstats, __smp_processor_id()).count++)
>+#define XFS_STATS_DEC(count)		(per_cpu(xfsstats, __smp_processor_id()).count--)
>+#define XFS_STATS_ADD(count, inc)	(per_cpu(xfsstats, __smp_processor_id()).count += (inc))
> 
> extern void xfs_init_procfs(void);
> extern void xfs_cleanup_procfs(void);
>
>but what was checked in was a bit cleaner.
>
>  
>
Well calling such a internal function (__function) is not a cleaning 
coding style but works best :-) .
Combined with the current_cpu() fixes I mentioned, it looks like this:

diff -Nru linux-2.6.11-rc2/fs/xfs/linux-2.6/xfs_linux.h 
linux-2.6.11-rc2-ott/fs/xfs/linux-2.6/xfs_linux.h
--- linux-2.6.11-rc2/fs/xfs/linux-2.6/xfs_linux.h    2004-12-24 
21:35:50.000000000 +0000
+++ linux-2.6.11-rc2-ott/fs/xfs/linux-2.6/xfs_linux.h    2005-01-27 
18:13:09.000000000 +0000
@@ -144,7 +144,7 @@
 #define xfs_inherit_nosymlinks    xfs_params.inherit_nosym.val
 #define xfs_rotorstep        xfs_params.rotorstep.val
 
-#define current_cpu()        smp_processor_id()
+#define current_cpu()        __smp_processor_id()
 #define current_pid()        (current->pid)
 #define current_fsuid(cred)    (current->fsuid)
 #define current_fsgid(cred)    (current->fsgid)
diff -Nru linux-2.6.11-rc2/fs/xfs/linux-2.6/xfs_stats.h 
linux-2.6.11-rc2-ott/fs/xfs/linux-2.6/xfs_stats.h
--- linux-2.6.11-rc2/fs/xfs/linux-2.6/xfs_stats.h    2004-12-24 
21:34:29.000000000 +0000
+++ linux-2.6.11-rc2-ott/fs/xfs/linux-2.6/xfs_stats.h    2005-01-27 
18:13:44.000000000 +0000
@@ -142,9 +142,9 @@
 
 /* We don't disable preempt, not too worried about poking the
  * wrong cpu's stat for now */
-#define XFS_STATS_INC(count)        (__get_cpu_var(xfsstats).count++)
-#define XFS_STATS_DEC(count)        (__get_cpu_var(xfsstats).count--)
-#define XFS_STATS_ADD(count, inc)    (__get_cpu_var(xfsstats).count += 
(inc))
+#define XFS_STATS_INC(count)        (per_cpu(xfsstats, 
__smp_processor_id()).count++)
+#define XFS_STATS_DEC(count)        (per_cpu(xfsstats, 
__smp_processor_id()).count--)
+#define XFS_STATS_ADD(count, inc)    (per_cpu(xfsstats, 
__smp_processor_id()).count += (inc))
 
 extern void xfs_init_procfs(void);
 extern void xfs_cleanup_procfs(void);

I'll submit it to the mailinglist as a seperate patch, so Linus can 
apply it to the current Kernel.

Matthias-Christian Ott

-- 
http://unixforge.org/~matthias-christian-ott/

Re: Preempt & Xfs Question

[Chris Wedgwood]

On Thu, Jan 27, 2005 at 06:24:13PM +0000, Matthias-Christian Ott wrote:

> Well calling such a internal function (__function) is not a cleaning
> coding style but works best :-) .

__foo does NOT mean it's an internal function necessarily or that it's
unclean to use it (sadly Linux has pretty vague (nothing consistent)
rules on how to interpret __foo versus foo really.

> Combined with the current_cpu() fixes I mentioned, it looks like
> this:

(1) your patch is mangled/wrapped

(2) this is fixed in CVS already (for maybe a week or so now?)

> I'll submit it to the mailinglist as a seperate patch, so Linus can
> apply it to the current Kernel.

XFS patches/suggestions should go to linux-xfs@oss.sgi.com and the
maintainers there can comment as needed.

Re: Preempt & Xfs Question

[Matthias-Christian Ott]

Chris Wedgwood wrote:

>On Thu, Jan 27, 2005 at 06:24:13PM +0000, Matthias-Christian Ott wrote:
>
>  
>
>>Well calling such a internal function (__function) is not a cleaning
>>coding style but works best :-) .
>>    
>>
>
>__foo does NOT mean it's an internal function necessarily or that it's
>unclean to use it (sadly Linux has pretty vague (nothing consistent)
>rules on how to interpret __foo versus foo really.
>
>  
>
>>Combined with the current_cpu() fixes I mentioned, it looks like
>>this:
>>    
>>
>
>(1) your patch is mangled/wrapped
>
>(2) this is fixed in CVS already (for maybe a week or so now?)
>
>  
>
>>I'll submit it to the mailinglist as a seperate patch, so Linus can
>>apply it to the current Kernel.
>>    
>>
>
>XFS patches/suggestions should go to linux-xfs@oss.sgi.com and the
>maintainers there can comment as needed.
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>
Hi!
I didn't know about the xfs development status. But it's good to hear 
that it's already fixed. When will it be included to the Kernel?

Matthias-Christian Ott

-- 
http://unixforge.org/~matthias-christian-ott/

Re: Preempt & Xfs Question

[Nathan Scott]

On Thu, Jan 27, 2005 at 08:51:45AM -0800, Chris Wedgwood wrote:
> On Thu, Jan 27, 2005 at 06:24:13PM +0000, Matthias-Christian Ott wrote:
> > I'll submit it to the mailinglist as a seperate patch, so Linus can
> > apply it to the current Kernel.

Chris' fix for this is in Linus' mail, queued to be merged hopefully RSN.
(*prod prod*)

> XFS patches/suggestions should go to linux-xfs@oss.sgi.com and the
> maintainers there can comment as needed.

cheers.

-- 
Nathan

Re: Preempt & Xfs Question

[Matthias-Christian Ott]

Steve Lord wrote:

> Matthias-Christian Ott wrote:
>
>> Hi!
>> I have a question: Why do I get such debug messages:
>>
>> BUG: using smp_processor_id() in preemptible [00000001] code: 
>> khelper/892
>> caller is _pagebuf_lookup_pages+0x11b/0x362
>> [<c03119c7>] smp_processor_id+0xa3/0xb4
>> [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362
>> [<c02ef802>] _pagebuf_lookup_pages+0x11b/0x362
>
>
> .....
>
>>
>> Does the XFS Module avoid preemption rules? If so, why?
>
>
> It is probably coming from these macros which keep various statistics
> inside xfs as per cpu variables.
>
> in fs//xfs/linux-2.6/xfs_stats.h:
>
> DECLARE_PER_CPU(struct xfsstats, xfsstats);
>
> /* We don't disable preempt, not too worried about poking the
>  * wrong cpu's stat for now */
> #define XFS_STATS_INC(count)            (__get_cpu_var(xfsstats).count++)
> #define XFS_STATS_DEC(count)            (__get_cpu_var(xfsstats).count--)
> #define XFS_STATS_ADD(count, inc)       (__get_cpu_var(xfsstats).count 
> += (inc))
>
> So it knows about the fact that preemption can mess up the result of 
> this,
> but it does not really matter for the purpose it is used for here. The
> stats are just informational but very handy for working out what is going
> on inside xfs. Using a global instead of a per cpu variable would
> lead to cache line contention.
>
> If you want to make it go away on a preemptable kernel, then use the
> alternate definition of the stat macros which is just below the
> above code.
>
> Steve
>
> p.s. try running xfs_stats.pl -f which comes with the xfs-cmds source to
> watch the stats.
>
>
Hi!
It happens to all functions -- not only to the stat functions -- because 
of this macro:
#define current_cpu()           smp_processor_id()

But this it not my problem (setting it 0 fixes it or get_cpu()). I just 
wanted to know why it breaks the preemption rules.

Matthias-Christian

-- 
http://unixforge.org/~matthias-christian-ott/