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/
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.
> BUG: using smp_processor_id() in preemptible [00000001] code:
> khelper/892
fixed in CVS, I guess it will hit mainline soon
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/
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/
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.
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/
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 [email protected] and the
maintainers there can comment as needed.
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 [email protected] 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 [email protected]
>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/
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 [email protected] and the
> maintainers there can comment as needed.
cheers.
--
Nathan