mips allmodconfig fails with
ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
undefined!
which is due to LUSTRE using copy_from_user_page which is not exported by any
architecture. Unfortunately, LUSTRE can only be built as module, so there is no
easy fix.
MIPS, SH, and optionally XTENSA implement copy_from_user_page as unexported
functions. Disable LUSTRE for those.
Cc: Peng Tao <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/staging/lustre/lustre/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/Kconfig b/drivers/staging/lustre/lustre/Kconfig
index 4e898e4..07ce43d 100644
--- a/drivers/staging/lustre/lustre/Kconfig
+++ b/drivers/staging/lustre/lustre/Kconfig
@@ -1,6 +1,6 @@
config LUSTRE_FS
tristate "Lustre file system client support"
- depends on INET && m
+ depends on INET && m && !MIPS && !XTENSA && !SUPERH
select LNET
select CRYPTO
select CRYPTO_CRC32
--
1.7.9.7
On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> mips allmodconfig fails with
>
> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> undefined!
>
> which is due to LUSTRE using copy_from_user_page which is not exported by any
> architecture.
Any, or just these arches?
> Unfortunately, LUSTRE can only be built as module, so there is no
> easy fix.
Can't we just export the functions for those arches? Surely lutre
isn't the first/only driver that needs this?
thanks,
greg k-h
Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> mips allmodconfig fails with
>>
>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> undefined!
>>
>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>> architecture.
>
> Any, or just these arches?
Arches, dungeon masters.
>> Unfortunately, LUSTRE can only be built as module, so there is no
>> easy fix.
>
> Can't we just export the functions for those arches? Surely lutre
> isn't the first/only driver that needs this?
It's simply necessary to keep up to date: we can't keep building new
arches since the initial cost can be quite high.
On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> mips allmodconfig fails with
>>
>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> undefined!
>>
>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>> architecture.
>
> Any, or just these arches?
>
Other architectures implement it as define as far as I can see.
>> Unfortunately, LUSTRE can only be built as module, so there is no
>> easy fix.
>
> Can't we just export the functions for those arches? Surely lutre
> isn't the first/only driver that needs this?
>
That would be another option.
Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
https://lkml.org/lkml/2013/9/5/111
So please forget this patch. If sh/xtensa actually need it, we can do the same there.
Guenter
On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
> On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> > On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >> mips allmodconfig fails with
> >>
> >> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> >> undefined!
> >>
> >> which is due to LUSTRE using copy_from_user_page which is not exported by any
> >> architecture.
> >
> > Any, or just these arches?
> >
> Other architectures implement it as define as far as I can see.
Then why would it be a problem?
> >> Unfortunately, LUSTRE can only be built as module, so there is no
> >> easy fix.
> >
> > Can't we just export the functions for those arches? Surely lutre
> > isn't the first/only driver that needs this?
> >
> That would be another option.
>
> Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
>
> https://lkml.org/lkml/2013/9/5/111
>
> So please forget this patch. If sh/xtensa actually need it, we can do the same there.
Sounds good to me, consider it forgotten :)
greg k-h
On Mon, Sep 09, 2013 at 07:48:51AM +0530, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
> > On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >> Unfortunately, LUSTRE can only be built as module, so there is no
> >> easy fix.
> >
> > Can't we just export the functions for those arches? Surely lutre
> > isn't the first/only driver that needs this?
>
> It's simply necessary to keep up to date: we can't keep building new
> arches since the initial cost can be quite high.
What do you mean by this? What "initial cost"? You should be able to
cross-compile almost all arches on your desktop machine today with the
compilers we have on kernel.org. If I can get them set up and working,
they can't be that hard for anyone else :)
thanks,
greg k-h
On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
> On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
>> On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
>>> On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>>>> mips allmodconfig fails with
>>>>
>>>> ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>>>> undefined!
>>>>
>>>> which is due to LUSTRE using copy_from_user_page which is not exported by any
>>>> architecture.
>>>
>>> Any, or just these arches?
>>>
>> Other architectures implement it as define as far as I can see.
>
> Then why would it be a problem?
>
It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().
Guenter
>>>> Unfortunately, LUSTRE can only be built as module, so there is no
>>>> easy fix.
>>>
>>> Can't we just export the functions for those arches? Surely lutre
>>> isn't the first/only driver that needs this?
>>>
>> That would be another option.
>>
>> Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
>>
>> https://lkml.org/lkml/2013/9/5/111
>>
>> So please forget this patch. If sh/xtensa actually need it, we can do the same there.
>
> Sounds good to me, consider it forgotten :)
>
> greg k-h
>
>
Greg Kroah-Hartman wrote:
> What do you mean by this? What "initial cost"? You should be able to
> cross-compile almost all arches on your desktop machine today with the
> compilers we have on kernel.org. If I can get them set up and working,
> they can't be that hard for anyone else :)
Won't you need to physically explore hardware? How will the hardware
industry be driven otherwise?
On Mon, Sep 09, 2013 at 08:08:28AM +0530, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
> > What do you mean by this? What "initial cost"? You should be able to
> > cross-compile almost all arches on your desktop machine today with the
> > compilers we have on kernel.org. If I can get them set up and working,
> > they can't be that hard for anyone else :)
>
> Won't you need to physically explore hardware?
In order to do what?
> How will the hardware industry be driven otherwise?
I have no idea what you are referring to here, please explain.
Greg Kroah-Hartman wrote:
>> How will the hardware industry be driven otherwise?
>
> I have no idea what you are referring to here, please explain.
For stability, hardware needs to be present. When I started out a
couple of days ago, I blamed my monitor for the data corruption: it's
one extra point of leverage.
Ram
On 09/08/2013 07:55 PM, Ramkumar Ramachandra wrote:
> Greg Kroah-Hartman wrote:
>>> How will the hardware industry be driven otherwise?
>>
>> I have no idea what you are referring to here, please explain.
>
> For stability, hardware needs to be present. When I started out a
> couple of days ago, I blamed my monitor for the data corruption: it's
> one extra point of leverage.
>
I have no idea whatsoever what you are talking about.
Guenter
Guenter Roeck wrote:
>> For stability, hardware needs to be present. When I started out a
>> couple of days ago, I blamed my monitor for the data corruption: it's
>> one extra point of leverage.
>
> I have no idea whatsoever what you are talking about.
Sorry, my bad. I haven't done hardware driver development.
On Sun, Sep 08, 2013 at 07:31:18PM -0700, Guenter Roeck wrote:
> On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
> >On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
> >>On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
> >>>On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
> >>>>mips allmodconfig fails with
> >>>>
> >>>>ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
> >>>>undefined!
> >>>>
> >>>>which is due to LUSTRE using copy_from_user_page which is not exported by any
> >>>>architecture.
> >>>
> >>>Any, or just these arches?
> >>>
> >>Other architectures implement it as define as far as I can see.
> >
> >Then why would it be a problem?
> >
> It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().
>
> Guenter
>
> >>>>Unfortunately, LUSTRE can only be built as module, so there is no
> >>>>easy fix.
> >>>
> >>>Can't we just export the functions for those arches? Surely lutre
> >>>isn't the first/only driver that needs this?
> >>>
> >>That would be another option.
> >>
> >>Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
> >>
> >>https://lkml.org/lkml/2013/9/5/111
> >>
> >>So please forget this patch. If sh/xtensa actually need it, we can do the same there.
> >
> >Sounds good to me, consider it forgotten :)
> >
> >greg k-h
The proper "fix" seems to be to get rid of this new usage of
copy_from_user_page() in lustre.
The code in question is nothing but a copy of __access_remote_vm()
from mm/memory.c...
On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> Can't we just export the functions for those arches? Surely lutre
> isn't the first/only driver that needs this?
Lustre is. These are core mm helpers, and lustre uses them to
reimplement another core VM function. It then uses it to access
userspace environment variable.
In short all this code should be nuked, and no new symbols should be
exported.
On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > Can't we just export the functions for those arches? Surely lutre
> > isn't the first/only driver that needs this?
>
> Lustre is. These are core mm helpers, and lustre uses them to
> reimplement another core VM function. It then uses it to access
> userspace environment variable.
>
> In short all this code should be nuked, and no new symbols should be
> exported.
Ugh, you are right, the lustre code needs to be fixed here.
greg k-h
On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > > Can't we just export the functions for those arches? Surely lutre
> > > isn't the first/only driver that needs this?
> >
> > Lustre is. These are core mm helpers, and lustre uses them to
> > reimplement another core VM function. It then uses it to access
> > userspace environment variable.
> >
> > In short all this code should be nuked, and no new symbols should be
> > exported.
>
> Ugh, you are right, the lustre code needs to be fixed here.
>
Given that, should I send another patch marking it as BROKEN again ?
Guenter
On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> > > > Can't we just export the functions for those arches? Surely lutre
> > > > isn't the first/only driver that needs this?
> > >
> > > Lustre is. These are core mm helpers, and lustre uses them to
> > > reimplement another core VM function. It then uses it to access
> > > userspace environment variable.
> > >
> > > In short all this code should be nuked, and no new symbols should be
> > > exported.
> >
> > Ugh, you are right, the lustre code needs to be fixed here.
> >
> Given that, should I send another patch marking it as BROKEN again ?
Well, on those arches it's "broken", so I'll dig up your original patch
on this thread. It's just "normal" for staging drivers to duplicate
core code, it needs to be fixed up before it can be merged into the
kernel tree, so no need to do anything special.
thanks,
greg k-h
On Mon, Sep 9, 2013 at 7:22 PM, Greg Kroah-Hartman
<[email protected]> wrote:
> On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
>> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
>> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
>> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
>> > > > Can't we just export the functions for those arches? Surely lutre
>> > > > isn't the first/only driver that needs this?
>> > >
>> > > Lustre is. These are core mm helpers, and lustre uses them to
>> > > reimplement another core VM function. It then uses it to access
>> > > userspace environment variable.
>> > >
>> > > In short all this code should be nuked, and no new symbols should be
>> > > exported.
>> >
>> > Ugh, you are right, the lustre code needs to be fixed here.
>> >
>> Given that, should I send another patch marking it as BROKEN again ?
>
> Well, on those arches it's "broken", so I'll dig up your original patch
> on this thread. It's just "normal" for staging drivers to duplicate
> core code, it needs to be fixed up before it can be merged into the
> kernel tree, so no need to do anything special.
It's not only broken on MIPS, SH, and XTENSA, but also on at least parisc
and m68k[*].
It's no longer broken on sparc64, as the missing export already got
into mainline.
In light of the above, perhaps that should be reverted?
[*] Why does m68k allmodconfig still succeed on kissb???
It does fail for me, as m68k's copy_from_user_page() calls
flush_icache_user_range(), which is not exported.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Sep 09, 2013 at 09:11:25PM +0200, Geert Uytterhoeven wrote:
> On Mon, Sep 9, 2013 at 7:22 PM, Greg Kroah-Hartman
> <[email protected]> wrote:
> > On Mon, Sep 09, 2013 at 10:08:19AM -0700, Guenter Roeck wrote:
> >> On Mon, Sep 09, 2013 at 09:39:02AM -0700, Greg Kroah-Hartman wrote:
> >> > On Mon, Sep 09, 2013 at 06:40:12AM -0700, Christoph Hellwig wrote:
> >> > > On Sun, Sep 08, 2013 at 06:59:45PM -0700, Greg Kroah-Hartman wrote:
> >> > > > Can't we just export the functions for those arches? Surely lutre
> >> > > > isn't the first/only driver that needs this?
> >> > >
> >> > > Lustre is. These are core mm helpers, and lustre uses them to
> >> > > reimplement another core VM function. It then uses it to access
> >> > > userspace environment variable.
> >> > >
> >> > > In short all this code should be nuked, and no new symbols should be
> >> > > exported.
> >> >
> >> > Ugh, you are right, the lustre code needs to be fixed here.
> >> >
> >> Given that, should I send another patch marking it as BROKEN again ?
> >
> > Well, on those arches it's "broken", so I'll dig up your original patch
> > on this thread. It's just "normal" for staging drivers to duplicate
> > core code, it needs to be fixed up before it can be merged into the
> > kernel tree, so no need to do anything special.
>
> It's not only broken on MIPS, SH, and XTENSA, but also on at least parisc
> and m68k[*].
>
> It's no longer broken on sparc64, as the missing export already got
> into mainline.
> In light of the above, perhaps that should be reverted?
>
Agreed.
> [*] Why does m68k allmodconfig still succeed on kissb???
> It does fail for me, as m68k's copy_from_user_page() calls
> flush_icache_user_range(), which is not exported.
>
I don't see a build failure in m68k:allmodconfig either.
flush_icache_user_range() is called from copy_to_user_page(), not from
copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
which calls __flush_cache_030(). The first is inline, the second is
assembler, so I would expect it to work. Which doesn't answer
the question why it fails for you.
powerpc and frm export flush_icache_user_range(). Wonder if that is really
necessary or points to other abuses.
On parisc I currently only test defconfig. I'll check if allmodconfig passes
in 3.10 and/or 3.11; if yes I'll add it to my test suite.
Guenter
On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <[email protected]> wrote:
>> [*] Why does m68k allmodconfig still succeed on kissb???
>> It does fail for me, as m68k's copy_from_user_page() calls
>> flush_icache_user_range(), which is not exported.
>>
> I don't see a build failure in m68k:allmodconfig either.
>
> flush_icache_user_range() is called from copy_to_user_page(), not from
> copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
> which calls __flush_cache_030(). The first is inline, the second is
> assembler, so I would expect it to work. Which doesn't answer
> the question why it fails for you.
Sorry, I meant copy_to_user_page().
I tried with 2.6.3 from crosstool, and it succeeded, too.
Turns out cfs_access_process_vm() is called with write=0 only.
Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
Mystery solved.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Sep 10, 2013 at 10:49:05AM +0200, Geert Uytterhoeven wrote:
> On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <[email protected]> wrote:
> >> [*] Why does m68k allmodconfig still succeed on kissb???
> >> It does fail for me, as m68k's copy_from_user_page() calls
> >> flush_icache_user_range(), which is not exported.
> >>
> > I don't see a build failure in m68k:allmodconfig either.
> >
> > flush_icache_user_range() is called from copy_to_user_page(), not from
> > copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
> > which calls __flush_cache_030(). The first is inline, the second is
> > assembler, so I would expect it to work. Which doesn't answer
> > the question why it fails for you.
>
> Sorry, I meant copy_to_user_page().
>
> I tried with 2.6.3 from crosstool, and it succeeded, too.
>
Do such old versions of gcc still exist ? Just kidding :)
> Turns out cfs_access_process_vm() is called with write=0 only.
> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
> Mystery solved.
>
Still bad. Guess it would still fail with 4.6.3 if optimization is turned off.
Guenter
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
On Tue, Sep 10, 2013 at 6:44 PM, Guenter Roeck <[email protected]> wrote:
>> I tried with 2.6.3 from crosstool, and it succeeded, too.
>>
> Do such old versions of gcc still exist ? Just kidding :)
>
>> Turns out cfs_access_process_vm() is called with write=0 only.
>> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
>> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
>> Mystery solved.
>>
> Still bad. Guess it would still fail with 4.6.3 if optimization is turned off.
For the record: s/2.6.3/4.6.3/.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, Sep 9, 2013 at 1:01 PM, Heiko Carstens
<[email protected]> wrote:
> On Sun, Sep 08, 2013 at 07:31:18PM -0700, Guenter Roeck wrote:
>> On 09/08/2013 07:31 PM, Greg Kroah-Hartman wrote:
>> >On Sun, Sep 08, 2013 at 07:24:19PM -0700, Guenter Roeck wrote:
>> >>On 09/08/2013 06:59 PM, Greg Kroah-Hartman wrote:
>> >>>On Sun, Sep 08, 2013 at 06:03:00PM -0700, Guenter Roeck wrote:
>> >>>>mips allmodconfig fails with
>> >>>>
>> >>>>ERROR: "copy_from_user_page" [drivers/staging/lustre/lustre/libcfs/libcfs.ko]
>> >>>>undefined!
>> >>>>
>> >>>>which is due to LUSTRE using copy_from_user_page which is not exported by any
>> >>>>architecture.
>> >>>
>> >>>Any, or just these arches?
>> >>>
>> >>Other architectures implement it as define as far as I can see.
>> >
>> >Then why would it be a problem?
>> >
>> It isn't a problem for those other architectures. Mostly it is mapped to functions like memcpy().
>>
>> Guenter
>>
>> >>>>Unfortunately, LUSTRE can only be built as module, so there is no
>> >>>>easy fix.
>> >>>
>> >>>Can't we just export the functions for those arches? Surely lutre
>> >>>isn't the first/only driver that needs this?
>> >>>
>> >>That would be another option.
>> >>
>> >>Actually, turns out Geert submitted a patch to do this for mips already, and Ralf applied it:
>> >>
>> >>https://lkml.org/lkml/2013/9/5/111
>> >>
>> >>So please forget this patch. If sh/xtensa actually need it, we can do the same there.
>> >
>> >Sounds good to me, consider it forgotten :)
>> >
>> >greg k-h
>
> The proper "fix" seems to be to get rid of this new usage of
> copy_from_user_page() in lustre.
> The code in question is nothing but a copy of __access_remote_vm()
> from mm/memory.c...
>
The problem is access_process_vm() is not exported since certain
version of kernel including the latest. According to Christoph in the
other mail, access_process_vm() is also a core mm function that is not
supposed to be exported. Then what kind of change shall we make in
order to keep current functionality?
Thanks,
Bergwolf
On Tue, Sep 10, 2013 at 4:49 PM, Geert Uytterhoeven
<[email protected]> wrote:
> On Mon, Sep 9, 2013 at 10:06 PM, Guenter Roeck <[email protected]> wrote:
>>> [*] Why does m68k allmodconfig still succeed on kissb???
>>> It does fail for me, as m68k's copy_from_user_page() calls
>>> flush_icache_user_range(), which is not exported.
>>>
>> I don't see a build failure in m68k:allmodconfig either.
>>
>> flush_icache_user_range() is called from copy_to_user_page(), not from
>> copy_from_user_page(). copy_from_user_page() calls flush_cache_page()
>> which calls __flush_cache_030(). The first is inline, the second is
>> assembler, so I would expect it to work. Which doesn't answer
>> the question why it fails for you.
>
> Sorry, I meant copy_to_user_page().
>
> I tried with 2.6.3 from crosstool, and it succeeded, too.
>
> Turns out cfs_access_process_vm() is called with write=0 only.
> Gcc 2.6.3 optimizes away the write != 0 branch (which calls copy_to_user_page()
> and thus flush_icache_user_range()), while gcc 4.1.2 doesn't do that.
> Mystery solved.
>
Thanks for chasing down the root cause. I was also wondering why I
didn't see this when building on MIPS some time ago.
Thanks,
Tao
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
> The problem is access_process_vm() is not exported since certain
> version of kernel including the latest. According to Christoph in the
> other mail, access_process_vm() is also a core mm function that is not
> supposed to be exported. Then what kind of change shall we make in
> order to keep current functionality?
You should remove the higher level functionality, kernel modules are
not supposed to look at userspace environment variables.
On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <[email protected]> wrote:
> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>> The problem is access_process_vm() is not exported since certain
>> version of kernel including the latest. According to Christoph in the
>> other mail, access_process_vm() is also a core mm function that is not
>> supposed to be exported. Then what kind of change shall we make in
>> order to keep current functionality?
>
> You should remove the higher level functionality, kernel modules are
> not supposed to look at userspace environment variables.
>
OK. I've looked at the specific case that Lustre uses
access_process_vm() to get the jobid environment variable and package
it into the RPC requests to server. However, it turns out that in the
latest Lustre server code, the jobid in a request is not used
anywhere. So it looks like we can just get rid of it.
Andreas, could you please confirm this? Is the jobid an obsolete
parameter that can be abandoned? Or is there plan to use it somehow in
the future?
Thanks,
Tao
On Wed, Sep 11, 2013 at 10:25:57AM +0800, Peng Tao wrote:
> On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <[email protected]> wrote:
> > On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
> >> The problem is access_process_vm() is not exported since certain
> >> version of kernel including the latest. According to Christoph in the
> >> other mail, access_process_vm() is also a core mm function that is not
> >> supposed to be exported. Then what kind of change shall we make in
> >> order to keep current functionality?
> >
> > You should remove the higher level functionality, kernel modules are
> > not supposed to look at userspace environment variables.
> >
> OK. I've looked at the specific case that Lustre uses
> access_process_vm() to get the jobid environment variable and package
> it into the RPC requests to server. However, it turns out that in the
> latest Lustre server code, the jobid in a request is not used
> anywhere. So it looks like we can just get rid of it.
>
> Andreas, could you please confirm this? Is the jobid an obsolete
> parameter that can be abandoned? Or is there plan to use it somehow in
> the future?
>
"Plan to use it in the future" is not a reason or argument to keep it today,
especially if it is something you are not supposed to do to start with.
If you ever need it, you should be able to find some other means to
support a similar functionality.
Guenter
On Wed, Sep 11, 2013 at 10:30 AM, Guenter Roeck <[email protected]> wrote:
> On Wed, Sep 11, 2013 at 10:25:57AM +0800, Peng Tao wrote:
>> On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <[email protected]> wrote:
>> > On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>> >> The problem is access_process_vm() is not exported since certain
>> >> version of kernel including the latest. According to Christoph in the
>> >> other mail, access_process_vm() is also a core mm function that is not
>> >> supposed to be exported. Then what kind of change shall we make in
>> >> order to keep current functionality?
>> >
>> > You should remove the higher level functionality, kernel modules are
>> > not supposed to look at userspace environment variables.
>> >
>> OK. I've looked at the specific case that Lustre uses
>> access_process_vm() to get the jobid environment variable and package
>> it into the RPC requests to server. However, it turns out that in the
>> latest Lustre server code, the jobid in a request is not used
>> anywhere. So it looks like we can just get rid of it.
>>
>> Andreas, could you please confirm this? Is the jobid an obsolete
>> parameter that can be abandoned? Or is there plan to use it somehow in
>> the future?
>>
> "Plan to use it in the future" is not a reason or argument to keep it today,
> especially if it is something you are not supposed to do to start with.
> If you ever need it, you should be able to find some other means to
> support a similar functionality.
>
I'm not fighting against removing the piece of code. But if there is a
strong reason to keep the functionality, we need to find a way to
implement it. The convenience of using environment variables is that
job scheduler can set the environment and other existing applications
don't have to change. Are there other means to do the same? ioctl and
upcall both need application change AFAIK.
Again, if the code is just obsolete, which is quite likely but needs
Andreas' confirmation, we can just remove it.
Thanks,
Tao
On Wed, Sep 11, 2013 at 10:51:50AM +0800, Peng Tao wrote:
> I'm not fighting against removing the piece of code. But if there is a
> strong reason to keep the functionality, we need to find a way to
> implement it. The convenience of using environment variables is that
> job scheduler can set the environment and other existing applications
> don't have to change. Are there other means to do the same? ioctl and
> upcall both need application change AFAIK.
There is no use case for it, the kernel has no business looking at these
variables. Given that you think it's not even used I don't even know
why we're having this discussion.
Talking about nasty code, the whole linux-curproc.c is highly
questionable:
- cfs_curproc_groups_nr:
unused and should be removed
- cfs_cap_raise/cfs_cap_lower/cfs_cap_raised:
needs to go away, modyules must not change access permissions
on behalf of processes
- the whole cfs_cap_t handling also needs to go away, passing around
capabilities is not a concept the kernel supports for a reason
- current_is_32bit:
Code should just use is_compat_task directly.
I've just taken the time to walk through this one file, but it seems
like most of libcfs is just as bad.
On 2013/09/10 8:25 PM, "Peng Tao" <[email protected]> wrote:
>On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <[email protected]>
>wrote:
>> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>>> The problem is access_process_vm() is not exported since certain
>>> version of kernel including the latest. According to Christoph in the
>>> other mail, access_process_vm() is also a core mm function that is not
>>> supposed to be exported. Then what kind of change shall we make in
>>> order to keep current functionality?
>>
>> You should remove the higher level functionality, kernel modules are
>> not supposed to look at userspace environment variables.
>>
>OK. I've looked at the specific case that Lustre uses
>access_process_vm() to get the jobid environment variable and package
>it into the RPC requests to server. However, it turns out that in the
>latest Lustre server code, the jobid in a request is not used
>anywhere. So it looks like we can just get rid of it.
>
>Andreas, could you please confirm this? Is the jobid an obsolete
>parameter that can be abandoned? Or is there plan to use it somehow in
>the future?
The jobid code is relatively new and in use, I'm not sure why you think
it is not in use. It is actually a feature that a bunch of users
requested.
The jobid feature allows tracking IO request stats for parallel user
processes
running on possibly thousands of different client nodes onto the servers.
This is easy to do with a single node and a single process via PID/PPID
and blktrace or equivalent, but otherwise impossible in a parallel
processing
environment where there may be users running hundreds of different jobs.
The PID/PPID is not consistent across client nodes, and the server threads
will randomly handle requests from all users.
By all means, I'd prefer to just use access_process_vm() directly, instead
of making a copy in the Lustre code. Not being able to access the process
environment seems a bit ridiculous - the kernel stores and manages this for
the process, and it isn't even doing anything nasty like accessing the
environment from a different process, just its own environment variables.
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel High Performance Data Division
On 2013/09/11 10:29 AM, "Christoph Hellwig" <[email protected]> wrote:
>Talking about nasty code, the whole linux-curproc.c is highly
>questionable:
>
> - cfs_curproc_groups_nr:
> unused and should be removed
This is already removed in the upstream Lustre code, it just hasn't made
it into the kernel yet.
>- cfs_cap_raise/cfs_cap_lower/cfs_cap_raised:
> needs to go away, modyules must not change access permissions
> on behalf of processes
These are only used on the server so that threads running as user UID/GID
can write to recovery log files. There is likely a different way that
this could be done, it has probably been this way from years ago when we
used the VFS to do file IO on the server and it was doing file permission
checking again.
> - the whole cfs_cap_t handling also needs to go away, passing around
> capabilities is not a concept the kernel supports for a reason
>
> - current_is_32bit:
> Code should just use is_compat_task directly.
This was already removed in the upstream Lustre code.
>I've just taken the time to walk through this one file, but it seems
>like most of libcfs is just as bad.
Sure, and that's why Lustre is in drivers/staging and not fs/, and I don't
make any claims to the contrary. We are slowly cleaning out these macros
(added when we wanted to get Lustre working on MacOS and WinNT), but it
will take some time. XFS lived with an IRIX shim layer for years, and
still
has a whole vnode abstraction layer that nobody seems to be complaining
about, so I don't think it is unreasonable for us to take some time to
clean
up Lustre.
Cheers, Andreas
--
Andreas Dilger
Lustre Software Architect
Intel High Performance Data Division
On Thu, Sep 12, 2013 at 4:48 AM, Dilger, Andreas
<[email protected]> wrote:
> On 2013/09/10 8:25 PM, "Peng Tao" <[email protected]> wrote:
>
>>On Wed, Sep 11, 2013 at 9:44 AM, Christoph Hellwig <[email protected]>
>>wrote:
>>> On Wed, Sep 11, 2013 at 01:14:11AM +0800, Peng Tao wrote:
>>>> The problem is access_process_vm() is not exported since certain
>>>> version of kernel including the latest. According to Christoph in the
>>>> other mail, access_process_vm() is also a core mm function that is not
>>>> supposed to be exported. Then what kind of change shall we make in
>>>> order to keep current functionality?
>>>
>>> You should remove the higher level functionality, kernel modules are
>>> not supposed to look at userspace environment variables.
>>>
>>OK. I've looked at the specific case that Lustre uses
>>access_process_vm() to get the jobid environment variable and package
>>it into the RPC requests to server. However, it turns out that in the
>>latest Lustre server code, the jobid in a request is not used
>>anywhere. So it looks like we can just get rid of it.
>>
>>Andreas, could you please confirm this? Is the jobid an obsolete
>>parameter that can be abandoned? Or is there plan to use it somehow in
>>the future?
>
> The jobid code is relatively new and in use, I'm not sure why you think
> it is not in use. It is actually a feature that a bunch of users
> requested.
>
You are right. Sorry I misread the code yesterday. I accidentally
searched for callers of lustre_msg_get_jobid() in a kernel tree
instead of a Lustre tree. Now I see that jobid is used by server for
request tracking purpose. Thank you for pointing it out.
> The jobid feature allows tracking IO request stats for parallel user
> processes
> running on possibly thousands of different client nodes onto the servers.
> This is easy to do with a single node and a single process via PID/PPID
> and blktrace or equivalent, but otherwise impossible in a parallel
> processing
> environment where there may be users running hundreds of different jobs.
> The PID/PPID is not consistent across client nodes, and the server threads
> will randomly handle requests from all users.
>
> By all means, I'd prefer to just use access_process_vm() directly, instead
> of making a copy in the Lustre code. Not being able to access the process
> environment seems a bit ridiculous - the kernel stores and manages this for
> the process, and it isn't even doing anything nasty like accessing the
> environment from a different process, just its own environment variables.
>
Or, can we read from /proc/self/environ? Instead of reading from
mm->env_start/env_end, we can let proc_environ_operations do the same
and avoid calling access_process_vm() directly.
Thanks,
Tao