[Xenomai-core] I-pipe 1.7 breaks mlockall safety check

[Xenomai-core] I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Hi all,

I think I found another unwanted side-effect of the no-cow changes:

With the I-pipe 1.7 patch series the test for missing mlockall no longer
works. I just - once again - wrote a test program that was lacking this
call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
get the usual error message on startup.

Jan


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

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Thu, 2007-02-15 at 14:06 +0100, Jan Kiszka wrote:
> Hi all,
> 
> I think I found another unwanted side-effect of the no-cow changes:
> 
> With the I-pipe 1.7 patch series the test for missing mlockall no longer
> works. I just - once again - wrote a test program that was lacking this
> call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
> get the usual error message on startup.
> 

I can't reproduce this with 1.7-01 here. Which Xenomai codebase are you
currently using (trunk/, 2.3.x maintenance or stock 2.3)?

> Jan
> 
-- 
Philippe.

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Thu, 2007-02-15 at 14:06 +0100, Jan Kiszka wrote:
>> Hi all,
>>
>> I think I found another unwanted side-effect of the no-cow changes:
>>
>> With the I-pipe 1.7 patch series the test for missing mlockall no longer
>> works. I just - once again - wrote a test program that was lacking this
>> call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
>> get the usual error message on startup.
>>
> 
> I can't reproduce this with 1.7-01 here. Which Xenomai codebase are you
> currently using (trunk/, 2.3.x maintenance or stock 2.3)?

Specifically trunk, but the first observation was over 2.3.x
maintenance. The test code is using the posix skin.

Jan


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

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Thu, 2007-02-15 at 14:58 +0100, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Thu, 2007-02-15 at 14:06 +0100, Jan Kiszka wrote:
> >> Hi all,
> >>
> >> I think I found another unwanted side-effect of the no-cow changes:
> >>
> >> With the I-pipe 1.7 patch series the test for missing mlockall no longer
> >> works. I just - once again - wrote a test program that was lacking this
> >> call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
> >> get the usual error message on startup.
> >>
> > 
> > I can't reproduce this with 1.7-01 here. Which Xenomai codebase are you
> > currently using (trunk/, 2.3.x maintenance or stock 2.3)?
> 
> Specifically trunk, but the first observation was over 2.3.x
> maintenance. The test code is using the posix skin.

I still don't find any explanation for that behaviour, only trivially
testing with and without mlockall() in a bare main routine though.

In relation to this, I've rolled out 1.7-02/x86, so that we could close
the last pending issue(s) holding v2.3.1 wrt the interrupt pipeline
support. Specifically, the -nocow related changes have been amended, and
we are now back to the implementation that prevailed in the 1.6 series
wrt vmalloc and ioremap memory, while still retaining the ability to
break COW eagerly for the RT threads.

Feedback welcome on this.

-- 
Philippe.

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Thu, 2007-02-15 at 14:58 +0100, Jan Kiszka wrote:
>> Philippe Gerum wrote:
>>> On Thu, 2007-02-15 at 14:06 +0100, Jan Kiszka wrote:
>>>> Hi all,
>>>>
>>>> I think I found another unwanted side-effect of the no-cow changes:
>>>>
>>>> With the I-pipe 1.7 patch series the test for missing mlockall no longer
>>>> works. I just - once again - wrote a test program that was lacking this
>>>> call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
>>>> get the usual error message on startup.
>>>>
>>> I can't reproduce this with 1.7-01 here. Which Xenomai codebase are you
>>> currently using (trunk/, 2.3.x maintenance or stock 2.3)?
>> Specifically trunk, but the first observation was over 2.3.x
>> maintenance. The test code is using the posix skin.
> 
> I still don't find any explanation for that behaviour, only trivially
> testing with and without mlockall() in a bare main routine though.
> 
> In relation to this, I've rolled out 1.7-02/x86, so that we could close
> the last pending issue(s) holding v2.3.1 wrt the interrupt pipeline
> support. Specifically, the -nocow related changes have been amended, and
> we are now back to the implementation that prevailed in the 1.6 series
> wrt vmalloc and ioremap memory, while still retaining the ability to
> break COW eagerly for the RT threads.
> 
> Feedback welcome on this.
> 

From first tests it looks like the BUGs on RTnet startup are resolved
with 1.7-02. I will have a look at the mlockall issue again to
understand what happens precisely there (I'm still lacking the warning).

Jan


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

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Hi Philippe,

I just verfied that the mlockall issue persists. But it doesn't appear
to be a regression anymore. This little posix demo exposes it now on
both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:

#include <stdio.h>
#include <sys/mman.h>
#include <pthread.h>

int main(int argc, char *argv[])
{
	struct sched_param param = { .sched_priority = 10 };

//	mlockall(MCL_CURRENT|MCL_FUTURE);

	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);

	printf("shouldn't be printed...\n");
	pause();
}


In contrast, the same done via the native skin (rt_task_shadow) triggers
warning and program termination as expected.

It looks to me like the temporary mlockall during libpthread_rt init is
not really reverted (but munlockall is actually called) or not
propagated to the mm state that is later checked on xnshadow_map.

Jan


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

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > Hi Philippe,
 > 
 > I just verfied that the mlockall issue persists. But it doesn't appear
 > to be a regression anymore. This little posix demo exposes it now on
 > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
 > 
 > #include <stdio.h>
 > #include <sys/mman.h>
 > #include <pthread.h>
 > 
 > int main(int argc, char *argv[])
 > {
 > 	struct sched_param param = { .sched_priority = 10 };
 > 
 > //	mlockall(MCL_CURRENT|MCL_FUTURE);
 > 
 > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
 > 
 > 	printf("shouldn't be printed...\n");
 > 	pause();
 > }
 > 
 > 
 > In contrast, the same done via the native skin (rt_task_shadow) triggers
 > warning and program termination as expected.
 > 
 > It looks to me like the temporary mlockall during libpthread_rt init is
 > not really reverted (but munlockall is actually called) or not
 > propagated to the mm state that is later checked on xnshadow_map.

The problem is that the root thread is already shadowed in this program
when pthread_setschedparam is called. So, xnshadow_map is not called and
the flag is not checked.

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > Hi Philippe,
>  > 
>  > I just verfied that the mlockall issue persists. But it doesn't appear
>  > to be a regression anymore. This little posix demo exposes it now on
>  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
>  > 
>  > #include <stdio.h>
>  > #include <sys/mman.h>
>  > #include <pthread.h>
>  > 
>  > int main(int argc, char *argv[])
>  > {
>  > 	struct sched_param param = { .sched_priority = 10 };
>  > 
>  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
>  > 
>  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
>  > 
>  > 	printf("shouldn't be printed...\n");
>  > 	pause();
>  > }
>  > 
>  > 
>  > In contrast, the same done via the native skin (rt_task_shadow) triggers
>  > warning and program termination as expected.
>  > 
>  > It looks to me like the temporary mlockall during libpthread_rt init is
>  > not really reverted (but munlockall is actually called) or not
>  > propagated to the mm state that is later checked on xnshadow_map.
> 
> The problem is that the root thread is already shadowed in this program
> when pthread_setschedparam is called. So, xnshadow_map is not called and
> the flag is not checked.
> 

Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
VM_LOCKED bit from the caller's mm default flags.

Since this behaviour is specific to the POSIX skin because it silently
shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
maybe we should check the mm state in the POSIX thread creation routine
too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.

-- 
Philippe.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Gilles Chanteperdrix]

Philippe Gerum wrote:
 > On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
 > > Jan Kiszka wrote:
 > >  > Hi Philippe,
 > >  > 
 > >  > I just verfied that the mlockall issue persists. But it doesn't appear
 > >  > to be a regression anymore. This little posix demo exposes it now on
 > >  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
 > >  > 
 > >  > #include <stdio.h>
 > >  > #include <sys/mman.h>
 > >  > #include <pthread.h>
 > >  > 
 > >  > int main(int argc, char *argv[])
 > >  > {
 > >  > 	struct sched_param param = { .sched_priority = 10 };
 > >  > 
 > >  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
 > >  > 
 > >  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
 > >  > 
 > >  > 	printf("shouldn't be printed...\n");
 > >  > 	pause();
 > >  > }
 > >  > 
 > >  > 
 > >  > In contrast, the same done via the native skin (rt_task_shadow) triggers
 > >  > warning and program termination as expected.
 > >  > 
 > >  > It looks to me like the temporary mlockall during libpthread_rt init is
 > >  > not really reverted (but munlockall is actually called) or not
 > >  > propagated to the mm state that is later checked on xnshadow_map.
 > > 
 > > The problem is that the root thread is already shadowed in this program
 > > when pthread_setschedparam is called. So, xnshadow_map is not called and
 > > the flag is not checked.
 > > 
 > 
 > Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
 > VM_LOCKED bit from the caller's mm default flags.
 > 
 > Since this behaviour is specific to the POSIX skin because it silently
 > shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
 > maybe we should check the mm state in the POSIX thread creation routine
 > too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.

The flag is checked if another thread is created, since xnshadow_map is
called. The only solution I see at the moment is to remove the call to
munlockall in the library initialization.

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Gilles Chanteperdrix wrote:
> Philippe Gerum wrote:
>  > On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
>  > > Jan Kiszka wrote:
>  > >  > Hi Philippe,
>  > >  > 
>  > >  > I just verfied that the mlockall issue persists. But it doesn't appear
>  > >  > to be a regression anymore. This little posix demo exposes it now on
>  > >  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
>  > >  > 
>  > >  > #include <stdio.h>
>  > >  > #include <sys/mman.h>
>  > >  > #include <pthread.h>
>  > >  > 
>  > >  > int main(int argc, char *argv[])
>  > >  > {
>  > >  > 	struct sched_param param = { .sched_priority = 10 };
>  > >  > 
>  > >  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
>  > >  > 
>  > >  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
>  > >  > 
>  > >  > 	printf("shouldn't be printed...\n");
>  > >  > 	pause();
>  > >  > }
>  > >  > 
>  > >  > 
>  > >  > In contrast, the same done via the native skin (rt_task_shadow) triggers
>  > >  > warning and program termination as expected.
>  > >  > 
>  > >  > It looks to me like the temporary mlockall during libpthread_rt init is
>  > >  > not really reverted (but munlockall is actually called) or not
>  > >  > propagated to the mm state that is later checked on xnshadow_map.
>  > > 
>  > > The problem is that the root thread is already shadowed in this program
>  > > when pthread_setschedparam is called. So, xnshadow_map is not called and
>  > > the flag is not checked.
>  > > 
>  > 
>  > Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
>  > VM_LOCKED bit from the caller's mm default flags.
>  > 
>  > Since this behaviour is specific to the POSIX skin because it silently
>  > shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
>  > maybe we should check the mm state in the POSIX thread creation routine
>  > too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.
> 
> The flag is checked if another thread is created, since xnshadow_map is
> called. The only solution I see at the moment is to remove the call to
> munlockall in the library initialization.
> 

So this piece of code should trigger again?

#include <stdio.h>
#include <sys/mman.h>
#include <pthread.h>

void *thread(void *arg)
{
        struct sched_param param = { .sched_priority = 10 };

        pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
        printf("shouldn't be printed...\n");
        return NULL;
}

int main(int argc, char *argv[])
{
        pthread_t th;

        pthread_create(&th, NULL, thread, NULL);
        pthread_join(th, NULL);
}


Well, it doesn't in fact, and I think I found my regression again. This
demo behaves as expected over 1.6-06, but remains silent over I-pipe 1.7-02.

Jan


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

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Jan Kiszka]

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

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Philippe Gerum wrote:
>>  > On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
>>  > > Jan Kiszka wrote:
>>  > >  > Hi Philippe,
>>  > >  > 
>>  > >  > I just verfied that the mlockall issue persists. But it doesn't appear
>>  > >  > to be a regression anymore. This little posix demo exposes it now on
>>  > >  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
>>  > >  > 
>>  > >  > #include <stdio.h>
>>  > >  > #include <sys/mman.h>
>>  > >  > #include <pthread.h>
>>  > >  > 
>>  > >  > int main(int argc, char *argv[])
>>  > >  > {
>>  > >  > 	struct sched_param param = { .sched_priority = 10 };
>>  > >  > 
>>  > >  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
>>  > >  > 
>>  > >  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
>>  > >  > 
>>  > >  > 	printf("shouldn't be printed...\n");
>>  > >  > 	pause();
>>  > >  > }
>>  > >  > 
>>  > >  > 
>>  > >  > In contrast, the same done via the native skin (rt_task_shadow) triggers
>>  > >  > warning and program termination as expected.
>>  > >  > 
>>  > >  > It looks to me like the temporary mlockall during libpthread_rt init is
>>  > >  > not really reverted (but munlockall is actually called) or not
>>  > >  > propagated to the mm state that is later checked on xnshadow_map.
>>  > > 
>>  > > The problem is that the root thread is already shadowed in this program
>>  > > when pthread_setschedparam is called. So, xnshadow_map is not called and
>>  > > the flag is not checked.
>>  > > 
>>  > 
>>  > Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
>>  > VM_LOCKED bit from the caller's mm default flags.
>>  > 
>>  > Since this behaviour is specific to the POSIX skin because it silently
>>  > shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
>>  > maybe we should check the mm state in the POSIX thread creation routine
>>  > too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.
>>
>> The flag is checked if another thread is created, since xnshadow_map is
>> called. The only solution I see at the moment is to remove the call to
>> munlockall in the library initialization.
>>
> 
> So this piece of code should trigger again?
> 
> #include <stdio.h>
> #include <sys/mman.h>
> #include <pthread.h>
> 
> void *thread(void *arg)
> {
>         struct sched_param param = { .sched_priority = 10 };
> 
>         pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
>         printf("shouldn't be printed...\n");
>         return NULL;
> }
> 
> int main(int argc, char *argv[])
> {
>         pthread_t th;
> 
>         pthread_create(&th, NULL, thread, NULL);
>         pthread_join(th, NULL);
> }
> 
> 
> Well, it doesn't in fact, and I think I found my regression again. This
> demo behaves as expected over 1.6-06, but remains silent over I-pipe 1.7-02.

May this hunk explain the behaviour?

http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/arch/i386/patches/adeos-ipipe-2.6.20-i386-1.7-02.patch?a=i386;v=SVN-2.3.x#7755

munlockall is realised via mlockall, so OR'ing here would never take
away any flag.

Jan


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

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Mon, 2007-02-19 at 13:31 +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Gilles Chanteperdrix wrote:
> >> Philippe Gerum wrote:
> >>  > On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
> >>  > > Jan Kiszka wrote:
> >>  > >  > Hi Philippe,
> >>  > >  > 
> >>  > >  > I just verfied that the mlockall issue persists. But it doesn't appear
> >>  > >  > to be a regression anymore. This little posix demo exposes it now on
> >>  > >  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
> >>  > >  > 
> >>  > >  > #include <stdio.h>
> >>  > >  > #include <sys/mman.h>
> >>  > >  > #include <pthread.h>
> >>  > >  > 
> >>  > >  > int main(int argc, char *argv[])
> >>  > >  > {
> >>  > >  > 	struct sched_param param = { .sched_priority = 10 };
> >>  > >  > 
> >>  > >  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
> >>  > >  > 
> >>  > >  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
> >>  > >  > 
> >>  > >  > 	printf("shouldn't be printed...\n");
> >>  > >  > 	pause();
> >>  > >  > }
> >>  > >  > 
> >>  > >  > 
> >>  > >  > In contrast, the same done via the native skin (rt_task_shadow) triggers
> >>  > >  > warning and program termination as expected.
> >>  > >  > 
> >>  > >  > It looks to me like the temporary mlockall during libpthread_rt init is
> >>  > >  > not really reverted (but munlockall is actually called) or not
> >>  > >  > propagated to the mm state that is later checked on xnshadow_map.
> >>  > > 
> >>  > > The problem is that the root thread is already shadowed in this program
> >>  > > when pthread_setschedparam is called. So, xnshadow_map is not called and
> >>  > > the flag is not checked.
> >>  > > 
> >>  > 
> >>  > Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
> >>  > VM_LOCKED bit from the caller's mm default flags.
> >>  > 
> >>  > Since this behaviour is specific to the POSIX skin because it silently
> >>  > shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
> >>  > maybe we should check the mm state in the POSIX thread creation routine
> >>  > too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.
> >>
> >> The flag is checked if another thread is created, since xnshadow_map is
> >> called. The only solution I see at the moment is to remove the call to
> >> munlockall in the library initialization.
> >>
> > 
> > So this piece of code should trigger again?
> > 
> > #include <stdio.h>
> > #include <sys/mman.h>
> > #include <pthread.h>
> > 
> > void *thread(void *arg)
> > {
> >         struct sched_param param = { .sched_priority = 10 };
> > 
> >         pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
> >         printf("shouldn't be printed...\n");
> >         return NULL;
> > }
> > 
> > int main(int argc, char *argv[])
> > {
> >         pthread_t th;
> > 
> >         pthread_create(&th, NULL, thread, NULL);
> >         pthread_join(th, NULL);
> > }
> > 
> > 
> > Well, it doesn't in fact, and I think I found my regression again. This
> > demo behaves as expected over 1.6-06, but remains silent over I-pipe 1.7-02.
> 
> May this hunk explain the behaviour?
> 

Yes it is; the original version does overwrite the default flags
properly, not the modified one.

> http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/arch/i386/patches/adeos-ipipe-2.6.20-i386-1.7-02.patch?a=i386;v=SVN-2.3.x#7755
> 
> munlockall is realised via mlockall, so OR'ing here would never take
> away any flag.
> 
> Jan
> 
-- 
Philippe.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > May this hunk explain the behaviour?
 > 
 > http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/arch/i386/patches/adeos-ipipe-2.6.20-i386-1.7-02.patch?a=i386;v=SVN-2.3.x#7755
 > 
 > munlockall is realised via mlockall, so OR'ing here would never take
 > away any flag.

My intent was to avoid that the VM_PINNED flag be cleared by mlockall.
So, I guess the correct pattern is:

current->mm->def_flags = current->mm->def_flags & ~VM_LOCKED | def_flags;

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Mon, 2007-02-19 at 22:24 +0100, Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > May this hunk explain the behaviour?
>  > 
>  > http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/arch/i386/patches/adeos-ipipe-2.6.20-i386-1.7-02.patch?a=i386;v=SVN-2.3.x#7755
>  > 
>  > munlockall is realised via mlockall, so OR'ing here would never take
>  > away any flag.
> 
> My intent was to avoid that the VM_PINNED flag be cleared by mlockall.
> So, I guess the correct pattern is:
> 
> current->mm->def_flags = current->mm->def_flags & ~VM_LOCKED | def_flags;
> 

In such a case, I would suggest the following, so that we don't copy
unwanted regular flags unexpectedly:

--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -162,10 +162,10 @@ asmlinkage long sys_munlock(unsigned long start, size_t len)
 static int do_mlockall(int flags)
 {
 	struct vm_area_struct * vma, * prev = NULL;
-	unsigned int def_flags = 0;
+	unsigned int def_flags = current->mm->def_flags & VM_PINNED;
 
 	if (flags & MCL_FUTURE)
-		def_flags = VM_LOCKED;
+		def_flags |= VM_LOCKED;
 	current->mm->def_flags = def_flags;
 	if (flags == MCL_FUTURE)
 		goto out;

-- 
Philippe.

Re: [Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Mon, 2007-02-19 at 01:20 +0100, Gilles Chanteperdrix wrote:
> Philippe Gerum wrote:
>  > On Mon, 2007-02-19 at 00:31 +0100, Gilles Chanteperdrix wrote:
>  > > Jan Kiszka wrote:
>  > >  > Hi Philippe,
>  > >  > 
>  > >  > I just verfied that the mlockall issue persists. But it doesn't appear
>  > >  > to be a regression anymore. This little posix demo exposes it now on
>  > >  > both 2.6.20-1.7-02 and 2.6.19-1.6-06 against latest trunk:
>  > >  > 
>  > >  > #include <stdio.h>
>  > >  > #include <sys/mman.h>
>  > >  > #include <pthread.h>
>  > >  > 
>  > >  > int main(int argc, char *argv[])
>  > >  > {
>  > >  > 	struct sched_param param = { .sched_priority = 10 };
>  > >  > 
>  > >  > //	mlockall(MCL_CURRENT|MCL_FUTURE);
>  > >  > 
>  > >  > 	pthread_setschedparam(pthread_self(), SCHED_FIFO, &param);
>  > >  > 
>  > >  > 	printf("shouldn't be printed...\n");
>  > >  > 	pause();
>  > >  > }
>  > >  > 
>  > >  > 
>  > >  > In contrast, the same done via the native skin (rt_task_shadow) triggers
>  > >  > warning and program termination as expected.
>  > >  > 
>  > >  > It looks to me like the temporary mlockall during libpthread_rt init is
>  > >  > not really reverted (but munlockall is actually called) or not
>  > >  > propagated to the mm state that is later checked on xnshadow_map.
>  > > 
>  > > The problem is that the root thread is already shadowed in this program
>  > > when pthread_setschedparam is called. So, xnshadow_map is not called and
>  > > the flag is not checked.
>  > > 
>  > 
>  > Yep. This makes sense, since kernel-wise, sys_munlockall does clear the
>  > VM_LOCKED bit from the caller's mm default flags.
>  > 
>  > Since this behaviour is specific to the POSIX skin because it silently
>  > shadows the main thread as a Xenomai thread from the SCHED_NORMAL class,
>  > maybe we should check the mm state in the POSIX thread creation routine
>  > too, when SCHED_FIFO or SCHED_RR are passed as the scheduling class.
> 
> The flag is checked if another thread is created, since xnshadow_map is
> called.

Ack, it must be so anyway.

>  The only solution I see at the moment is to remove the call to
> munlockall in the library initialization.

The main thread of a POSIX program cannot have real-time constraints
(i.e. it may be acceptable for it to take page faults, unless otherwise
_explicitely_ specified) so there is no point to trigger SIGXCPU in that
case; the user has to (re)assert mlockall explicitely before creating
real-time threads from the main one, period. I agree that compared to
the usual behaviour of skins, it's a bit disturbing, but this all comes
from the implicit initial shadowing that other skins don't do.

-- 
Philippe.

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Gilles Chanteperdrix]

[-- Attachment #1: message body and .signature --]
[-- Type: text/plain, Size: 1897 bytes --]

Philippe Gerum wrote:
 > On Thu, 2007-02-15 at 14:58 +0100, Jan Kiszka wrote:
 > > Philippe Gerum wrote:
 > > > On Thu, 2007-02-15 at 14:06 +0100, Jan Kiszka wrote:
 > > >> Hi all,
 > > >>
 > > >> I think I found another unwanted side-effect of the no-cow changes:
 > > >>
 > > >> With the I-pipe 1.7 patch series the test for missing mlockall no longer
 > > >> works. I just - once again - wrote a test program that was lacking this
 > > >> call, but only with I-pipe 1.6-06 (same Xenomai version: latest trunk) I
 > > >> get the usual error message on startup.
 > > >>
 > > > 
 > > > I can't reproduce this with 1.7-01 here. Which Xenomai codebase are you
 > > > currently using (trunk/, 2.3.x maintenance or stock 2.3)?
 > > 
 > > Specifically trunk, but the first observation was over 2.3.x
 > > maintenance. The test code is using the posix skin.
 > 
 > I still don't find any explanation for that behaviour, only trivially
 > testing with and without mlockall() in a bare main routine though.
 > 
 > In relation to this, I've rolled out 1.7-02/x86, so that we could close
 > the last pending issue(s) holding v2.3.1 wrt the interrupt pipeline
 > support. Specifically, the -nocow related changes have been amended, and
 > we are now back to the implementation that prevailed in the 1.6 series
 > wrt vmalloc and ioremap memory, while still retaining the ability to
 > break COW eagerly for the RT threads.
 > 
 > Feedback welcome on this.

Here comes yet another amendment of the nocow patch, which avoid a race
between pgd_alloc creating a new pgd, and __ipipe_pin_range_globally not
seeing the new pgd because its task is not yet in the task
list. Unfortunately, __ipipe_pin_range_globally becomes an architecture
dependent function, but at least we should have no race. The patch also
fixes the issue with munlockall not clearing the VM_LOCKED flag.

-- 


					    Gilles Chanteperdrix.

[-- Attachment #2: ipipe-avoid-race.diff --]
[-- Type: text/plain, Size: 3781 bytes --]

diff -x '*~' -x '*.orig' -x '*.rej' -Naurdp ipipe-2.6.20-x86/arch/i386/mm/fault.c ipipe-2.6.20-x86-avoid-race/arch/i386/mm/fault.c
--- ipipe-2.6.20-x86/arch/i386/mm/fault.c	2007-02-19 22:33:58.000000000 +0100
+++ ipipe-2.6.20-x86-avoid-race/arch/i386/mm/fault.c	2007-02-19 22:39:34.000000000 +0100
@@ -656,16 +656,20 @@ void vmalloc_sync_all(void)
 #endif
 
 #ifdef CONFIG_IPIPE
-int __ipipe_pin_range_mapping(struct mm_struct *mm,
-			      unsigned long start, unsigned long end)
+void __ipipe_pin_range_globally(unsigned long start, unsigned long end)
 {
 	unsigned long next, addr = start;
 
 	do {
+		unsigned long flags;
+		struct page *page;
+
 		next = pgd_addr_end(addr, end);
-		vmalloc_sync_one(mm->pgd, addr);
-	} while (addr = next, addr != end);
+		spin_lock_irqsave(&pgd_lock, flags);
+		for (page = pgd_list; page; page = (struct page *)page->index)
+			vmalloc_sync_one(page_address(page), addr);
+		spin_unlock_irqrestore(&pgd_lock, flags);
 
-	return 0;
+	} while (addr = next, addr != end);
 }
 #endif /* CONFIG_IPIPE */
diff -x '*~' -x '*.orig' -x '*.rej' -Naurdp ipipe-2.6.20-x86/include/linux/ipipe.h ipipe-2.6.20-x86-avoid-race/include/linux/ipipe.h
--- ipipe-2.6.20-x86/include/linux/ipipe.h	2007-02-19 22:33:58.000000000 +0100
+++ ipipe-2.6.20-x86-avoid-race/include/linux/ipipe.h	2007-02-19 22:39:56.000000000 +0100
@@ -337,11 +337,6 @@ void fastcall __ipipe_sync_stage(unsigne
 
 void __ipipe_pin_range_globally(unsigned long start, unsigned long end);
 
-struct mm_struct;
-
-int __ipipe_pin_range_mapping(struct mm_struct *mm,
-			      unsigned long start, unsigned long end);
-
 #ifndef __ipipe_sync_pipeline
 #define __ipipe_sync_pipeline(syncmask) __ipipe_sync_stage(syncmask)
 #endif
@@ -695,7 +690,7 @@ int ipipe_disable_ondemand_mappings(stru
 #define ipipe_cleanup_notify(mm)	do { } while(0)
 #define ipipe_trap_notify(t,r)	0
 #define ipipe_init_proc()		do { } while(0)
-#define __ipipe_update_all_pinned_mm(start, end) 0
+#define __ipipe_pin_range_globally(start, end) do { } while(0)
 
 #define local_irq_enable_hw_cond()		do { } while(0)
 #define local_irq_disable_hw_cond()		do { } while(0)
diff -x '*~' -x '*.orig' -x '*.rej' -Naurdp ipipe-2.6.20-x86/mm/memory.c ipipe-2.6.20-x86-avoid-race/mm/memory.c
--- ipipe-2.6.20-x86/mm/memory.c	2007-02-19 22:33:58.000000000 +0100
+++ ipipe-2.6.20-x86-avoid-race/mm/memory.c	2007-02-19 22:41:06.000000000 +0100
@@ -2808,21 +2808,6 @@ int ipipe_disable_ondemand_mappings(stru
 	}
 	mm->def_flags |= VM_PINNED;
 
-	read_lock(&vmlist_lock);
-	for (area = vmlist; area; area = area->next) {
-		result =  __ipipe_pin_range_mapping(mm,
-						    (unsigned long) area->addr,
-						    (unsigned long) area->addr
-						    + area->size);
-		if (result) {
-			mm->def_flags &= ~VM_PINNED;
-			read_unlock(&vmlist_lock);
-			goto done_mm;
-		}
-	}
-
-	read_unlock(&vmlist_lock);
-
   done_mm:
 	up_write(&mm->mmap_sem);
 	mmput(mm);
@@ -2830,18 +2815,4 @@ int ipipe_disable_ondemand_mappings(stru
 }
 
 EXPORT_SYMBOL(ipipe_disable_ondemand_mappings);
-
-void __ipipe_pin_range_globally(unsigned long start, unsigned long end)
-{
-	struct task_struct *p;
-
-	read_lock(&tasklist_lock);
-
-	for_each_process(p)
-		if (p->mm)
-			__ipipe_pin_range_mapping(p->mm, start, end);
-
-	read_unlock(&tasklist_lock);
-}
-
 #endif
diff -x '*~' -x '*.orig' -x '*.rej' -Naurdp ipipe-2.6.20-x86/mm/mlock.c ipipe-2.6.20-x86-avoid-race/mm/mlock.c
--- ipipe-2.6.20-x86/mm/mlock.c	2007-02-19 22:33:58.000000000 +0100
+++ ipipe-2.6.20-x86-avoid-race/mm/mlock.c	2007-02-19 23:39:41.000000000 +0100
@@ -166,6 +166,7 @@ static int do_mlockall(int flags)
 
 	if (flags & MCL_FUTURE)
 		def_flags = VM_LOCKED;
+	current->mm->def_flags &= ~VM_LOCKED;
 	current->mm->def_flags |= def_flags;
 	if (flags == MCL_FUTURE)
 		goto out;

[Xenomai-core] Re: I-pipe 1.7 breaks mlockall safety check

[Philippe Gerum]

On Mon, 2007-02-19 at 23:48 +0100, Gilles Chanteperdrix wrote:

> Here comes yet another amendment of the nocow patch, which avoid a race
> between pgd_alloc creating a new pgd, and __ipipe_pin_range_globally not
> seeing the new pgd because its task is not yet in the task
> list. Unfortunately, __ipipe_pin_range_globally becomes an architecture
> dependent function, but at least we should have no race. The patch also
> fixes the issue with munlockall not clearing the VM_LOCKED flag.

Ok, I'm queuing this one for 1.7-04, in order to play safe with 1.7-03
for the upcoming 2.3.1 release. Since the race has always been there
anyway, we have some time ahead to shake this fix over 2.4-devel.

-- 
Philippe.